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 if unexpected length in PassthroughDigest #335

Closed
wants to merge 1 commit into from

Conversation

clehner
Copy link
Contributor

@clehner clehner commented Oct 26, 2021

PassthroughDigest represents a workaround for the requirement for a Digest implementation for use with the k256 and p256 crates.
Even though it it is only meant to be used internally for these specific uses, this PR updates it to add an additional requirement for security/robustness. This change is to require that the input data length is exactly 32 bytes, rather than allowing other lengths. This change enables panic to happen if an incorrect length is passed. An incorrect length would not be passed in the current code but could occur if code is changed resulting in incorrect use of PassthroughDigest. In case of such a change, we could consider it better to panic and reveal the bug during development, rather than potentially allow execution to proceed in a bad state.

Edit: test failure suggests this is not a correct change.

@clehner clehner requested a review from sbihel October 26, 2021 20:04
@clehner clehner marked this pull request as draft October 27, 2021 14:05
@sbihel
Copy link
Member

sbihel commented Oct 28, 2021

Summary from our discussion:

  • The reason why the digest handles the cases for length of 0 and >0 is because the digest is read past the first 32 bytes block. Still haven't found the piece of code that was the inspiration for this implementation.
  • It is only meant to be a temporary measure until Update crates to digest v0.10 RustCrypto/hashes#217 is merged.
  • The naming PassthroughDigest should probably reflect the purpose of being only used for Tezos signatures.

@clehner
Copy link
Contributor Author

clehner commented Jan 6, 2022

@sbihel the PR you mentioned was merged. Do you have ideas how to proceed?

I added another use of PassthroughDigest in #351, for ES256K-R with SHA-256 (that is the correct ES256K-R; the one using Keccak is now called ESKeccakK-R internally) - Maybe that should be using a Sha256 Digest implementation with try_sign_digest directly?

ssi/src/jws.rs

Lines 359 to 360 in f18763a

let hash = crate::hash::sha256(data)?;
let digest = Digest::chain(<PassthroughDigest as Digest>::new(), &hash);

@sbihel
Copy link
Member

sbihel commented Jan 6, 2022

I had a go last month but not all crates had been updated, I'll try again.

And yes, either try_sign_digest or try_sign directly I believe

@sbihel
Copy link
Member

sbihel commented Jan 7, 2022

Actually, not quite, the latest version of ecdsa still relies on a old version of signature (meaning an old version of digest) so I think it's best to wait a bit more.

@clehner
Copy link
Contributor Author

clehner commented Jan 27, 2022

Closing in favor of #385

@clehner clehner closed this Jan 27, 2022
@sbihel sbihel deleted the fix/passthrough-panic branch October 6, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants