fix: browser encrypt signature encoding #157
Conversation
resolves: aws#154 DER encoding is always signed. Browsers use 'raw' encoding. Where the r and s values are just concatenated. If the first bit of r or s is 1, an additional 0 byte should be added. The current implementation of raw2der does not do this. Tests and test vectors. The basic solution was to let the asn1 library handle the encoding and not pass bytes.
| * This turns out the be very tricky. | ||
| * Test vectors are the path to your success. | ||
| * In case you need to create more, | ||
| * I am reproducing the parts needed to copy/past yourself to victory. |
| const derSigNoPadding = new Uint8Array([48, 100, 2, 48, 125, 32, 154, 168, 112, 11, 187, 171, 135, 119, 83, 66, 69, 164, 226, 80, 39, 176, 112, 210, 72, 159, 201, 242, 110, 212, 158, 170, 99, 155, 80, 29, 99, 77, 158, 81, 170, 46, 116, 246, 137, 197, 82, 112, 70, 36, 196, 49, 2, 48, 117, 43, 254, 192, 131, 207, 80, 1, 152, 238, 154, 139, 42, 81, 244, 230, 42, 114, 137, 98, 127, 86, 166, 26, 172, 80, 132, 251, 97, 249, 4, 39, 47, 250, 132, 44, 187, 235, 197, 157, 56, 216, 39, 130, 69, 46, 185, 150]) | ||
| const rawSigNoPadding = new Uint8Array([125,32,154,168,112,11,187,171,135,119,83,66,69,164,226,80,39,176,112,210,72,159,201,242,110,212,158,170,99,155,80,29,99,77,158,81,170,46,116,246,137,197,82,112,70,36,196,49,117,43,254,192,131,207,80,1,152,238,154,139,42,81,244,230,42,114,137,98,127,86,166,26,172,80,132,251,97,249,4,39,47,250,132,44,187,235,197,157,56,216,39,130,69,46,185,150]) | ||
|
|
||
| // length == 103, R is padded |
There was a problem hiding this comment.
why is r and s lower case above, but upper case in this section.
There was a problem hiding this comment.
Sigh, so r and s are the variable names, but camel case it looks stupid.
I'm guessing that I copy/past ;) myself to an error, will fix
| * Otherwise you need to interpret this padding yourself. | ||
| */ | ||
| const r = new asn.bignum.BN(rawSignature.slice(0, _keyLengthBytes)) | ||
| const s = new asn.bignum.BN(rawSignature.slice(_keyLengthBytes)) |
There was a problem hiding this comment.
Nitpick: Consider an explicit "end" param to the slice call to extract for s. Would make the code more readable, might be useful in catching issues if the string is larger than it should be.
i.e. const s = ... rawSignature.slice(_keyLengthBytes, howeverLongThisShouldBe)
There was a problem hiding this comment.
I can, but there is a precondition at line 91. :) . it says that the signature MUST be 2 * _keyLengthBytes...
In many cases I would agree with you, but here, if I receive any amount of data other than what I'm expecting, it should fail.
| @@ -50,6 +127,58 @@ const validSuite = new WebCryptoAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES1 | |||
| const rawSignature = new Uint8Array([22, 77, 187, 192, 175, 104, 2, 240, 55, 2, 6, 138, 103, 148, 214, 240, 244, 65, 224, 254, 60, 52, 218, 22, 250, 245, 216, 228, 151, 151, 220, 234, 125, 9, 97, 8, 132, 123, 79, 193, 216, 207, 214, 0, 73, 183, 149, 173, 26, 173, 251, 132, 140, 139, 44, 122, 11, 50, 163, 105, 138, 221, 223, 29]) | |||
| const derSignature = new Uint8Array([48, 68, 2, 32, 22, 77, 187, 192, 175, 104, 2, 240, 55, 2, 6, 138, 103, 148, 214, 240, 244, 65, 224, 254, 60, 52, 218, 22, 250, 245, 216, 228, 151, 151, 220, 234, 2, 32, 125, 9, 97, 8, 132, 123, 79, 193, 216, 207, 214, 0, 73, 183, 149, 173, 26, 173, 251, 132, 140, 139, 44, 122, 11, 50, 163, 105, 138, 221, 223, 29]) | |||
|
|
|||
| /* | |||
| // All signatures should be verified with this public key (spki in bytes) | |||
| const publicKeyBytes = new Uint8Array([48,118,48,16,6,7,42,134,72,206,61,2,1,6,5,43,129,4,0,34,3,98,0,4,182,142,16,181,73,22,77,38,171,216,20,142,20,154,218,20,31,70,13,88,242,169,247,248,184,238,221,100,191,26,82,176,137,96,163,242,244,215,25,250,144,157,65,246,201,220,219,188,122,115,129,227,74,201,236,240,93,173,108,36,49,249,149,224,67,76,66,192,255,173,90,184,124,191,154,165,137,251,173,181,109,109,75,167,34,202,198,114,192,197,215,224,199,45,105,126]) | |||
There was a problem hiding this comment.
Super minor nitpick: Maybe base64 encoded would be cleaner for all of these vectors? In general it feels like there might be some way to do these tests that provides an easier way to verify that they're doing what we think they're doing, but b64 doesn't do that and I'm not sure what would, so I wanted to put it in your head as something to ponder.
There was a problem hiding this comment.
This is actually intentional. Browsers don't have a built in base64 encoder. And the function takes and returns Uint8Array. This format makes the notes above, re: verifying this in a browser, much more portable :)
See GHSA-h45p-w933-jxh3 Co-authored-by: Robin Salkeld <salkeldr@amazon.com>
See GHSA-h45p-w933-jxh3 Co-authored-by: Robin Salkeld <salkeldr@amazon.com>
resolves: #154
DER encoding is always signed.
Browsers use 'raw' encoding.
Where the r and s values are just concatenated.
If the first bit of r or s is 1,
an additional 0 byte should be added.
The current implementation of raw2der does not do this.
Tests and test vectors.
The basic solution was to let the asn1 library handle the encoding
and not pass bytes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.