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

Truncate digests to the left most bits when signing with ECDSA #298

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

mwielgoszewski
Copy link
Contributor

In Golang's ecdsa signing implementation, digests are truncated to the left most bits to match the bit length of the order of the curve.

If this digest is not truncated, the TPM will return an error parameter 1, error code 0x15 : structure is the wrong size.

This PR updates the signECDSA function to match what Golang does under the hood.

@brandonweeks
Copy link
Member

I believe we're using this code and it works for us. How did you encounter the error?

@mwielgoszewski
Copy link
Contributor Author

I wrote an implementation for certstore for linux that used TPM2 backed keys, and the certstore unit tests caught it:

From https://github.com/github/smimesign/blob/main/certstore/certstore_test.go#L230

sha384Digest := sha512.Sum384([]byte("hello"))
sig, err = signer.Sign(rand.Reader, sha384Digest[:], crypto.SHA384)
if err != nil {
	t.Fatal(err)
}
if err = leafEC.Certificate.CheckSignature(x509.ECDSAWithSHA384, []byte("hello"), sig); err != nil {
	t.Fatal(err)
}

@brandonweeks
Copy link
Member

For posterity: this isn't an issue with p256 + sha256, which explains why the bug was not previously discovered.

@brandonweeks
Copy link
Member

Thanks!

@brandonweeks brandonweeks merged commit 5238453 into google:master Nov 15, 2022
@mjg59
Copy link
Collaborator

mjg59 commented Nov 8, 2023

This breaks P-256 sha256 signing if the leading byte is 0 - the digest loses the leftmost bit (or bits, if there's multiple 0s in a row)

@mjg59
Copy link
Collaborator

mjg59 commented Nov 8, 2023

I think the correct fix is probably just to re-add as many leading 0s as are necessary to restore the original digest length?

@mjg59
Copy link
Collaborator

mjg59 commented Nov 8, 2023

(Although I don't know if this is correct in the case of a digest longer than the curve length)

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.

3 participants