Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic on blob deserialization #1008

Open
mzabaluev opened this issue Jan 17, 2025 · 2 comments
Open

Panic on blob deserialization #1008

mzabaluev opened this issue Jan 17, 2025 · 2 comments
Labels
Source: Audit Issues discovered by audit. suzuka:security

Comments

@mzabaluev
Copy link
Collaborator

The .into() call in try_verify can panic if the signature field inside the blob is not the correct length. This occurs before signature verification, so anyone can post such a blob on Celestia and cause movement nodes to panic.
Here’s a small unit test demonstrating the panic:

	#[test]
	fn poc_verify_can_panic() -> Result<(), anyhow::Error> {
		let s = InnerSignedBlobV1 {
			data: InnerSignedBlobV1Data::try_new(vec![1, 2, 3], 123).unwrap(),
			signature: vec![],
			signer: vec![
				2, 130, 130, 130, 130, 130, 130, 130, 82, 130, 130, 130, 130, 255, 255, 130, 130,
				130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130, 130,
			],
			id: Id(vec![1, 2, 3, 4]),
		};

		s.try_verify::<k256::Secp256k1>()?;

		Ok(())
	}
@mzabaluev mzabaluev added Source: Audit Issues discovered by audit. suzuka:security labels Jan 17, 2025
@mzabaluev
Copy link
Collaborator Author

mzabaluev commented Jan 17, 2025

The ecdsa crate uses an old version of generic-array, where the From<&[u8]> impl is panicky where by righs it should not exist: the TryFrom trait is the one for potentially fallible conversions. Unfortunately, the authors did't seem to believe in fallible conversions until later versions. Thanks for spotting this; such subtle ergonomic faults somewhat negate the value of using generic-array in rust-crypto.

@mzabaluev
Copy link
Collaborator Author

Fixed in commit dd3757b of #937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source: Audit Issues discovered by audit. suzuka:security
Projects
None yet
Development

No branches or pull requests

1 participant