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

fix: der2raw sLength is s byte length #634

Merged
merged 6 commits into from
Jun 30, 2021

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Jun 29, 2021

Raw signature (also see ISO/IEC 7816-8 / IEEE P1363)
are the static size concatenation of the r and s value.
These values MUST be 0 padded to the correct length.

This fixes the occasional Invalid Signature in browsers.
This impacts ~1% of messages.
These messages are valid and can now be decrypted.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

Raw signature (also see ISO/IEC 7816-8 / IEEE P1363)
are the static size concatenation of the r and s value.
These values MUST be 0 padded to the correct length.

This fixes the occasional `Invalid Signature` in browsers.
This impacts ~1% of messages.
These messages are valid and can now be decrypted.
@seebees seebees requested a review from texastony June 29, 2021 03:22
Copy link
Contributor

@texastony texastony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably because I am foolish, but if we could address some of the adjectives we are using, this will be a better read.


// Suite AlgorithmSuiteIdentifier.ALG_AES256_GCM_IV12_TAG16_HKDF_SHA512_COMMIT_KEY_ECDSA_P384
// prettier-ignore
const derSigOnlySPadded = new Uint8Array([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only concerns me, as it raises confusion. Given all I know, I assume that derSigOnlySPadded is different from derSigSPadded, and the difference is that r = 0, is that right? If not, what is the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like to delete vectors. How about numbers? derSigSPadded derSigSPadded1 derSigSPadded2`?

)
})

it('New vector S only padded', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor: I hate/despise New in a repo. It is New today, it is old tomorrow. Is there some other adjective we can use that possible addresses the Only comment above?

Copy link
Contributor

@texastony texastony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra Groovy!

@seebees seebees merged commit 46cd178 into aws:master Jun 30, 2021
@seebees seebees deleted the raw-ecdsa-signature-padding branch June 30, 2021 00:17
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