Use SSB-Validation-Dataset #22
Conversation
christianbundy
force-pushed
the
use-validation-dataset
branch
2 times, most recently
from
6c6b536
to
cdd3ec2
Compare
Problem: Maintaining one validation dataset per module means that we can't build the dataset collaboratively and the validation dataset tends toward stagnation. Solution: Use a collaborative validation dataset, which provides a handful of new and interesting test failures. This branch includes seven new failing tests that we should aim to resolve.
christianbundy
force-pushed
the
use-validation-dataset
branch
from
cdd3ec2
to
0f6ac12
Compare
Problem: The signature regex has ignored backslashes, but this bug is masked by another bug where a function is returning an error on failure (when it *should* return something falsey on error), meaning that the signature checking code doesn't work at all. Solution: Fix the signature checker regex and the backward return value.
Problem: We aren't validating the HMAC key anywhere, so we're taking junk values as input and truncating them. We should insist that HMAC keys are valid (either `null` or a 32-byte buffer encoded as a base64 string). Solution: Add checks for valid HMAC key.
Problem: Like the HMAC key, we're not actually checking the length of the signature, which means that we're allowing signatures of arbitrary length and then just using the first 64 bytes for verification. Solution: Check the signature length before asserting that the signature is valid.
Problem: Some existing tests were failing because of the new validation rules, which were from `hmac_key` being `null` (versus `undefined`) and trying to write to a directory that no longer exists. Solution: Fix the tests.
Problem: The hardcoded test was using 32 X characters as the HMAC key, which isn't valid base64. Solution: Use a random HMAC key instead of an invalid hardcoded one.
| var isEncryptedRx = isCanonicalBase64('','\.box.*') | ||
| var isSignatureRx = isCanonicalBase64('','\.sig.\w+') | ||
| var isEncryptedRx = isCanonicalBase64('','\\.box.*') | ||
| var isSignatureRx = isCanonicalBase64('','\\.sig.\\w+') |
There was a problem hiding this comment.
They're totally broken. 🙃
We want /\.box.*/, but the previous code accidentally gave us /.box.*/ because the backslash is interpreted as an escape character and ignored (because . doesn't need to be escaped in JavaScript strings). The . gives us a greedy match that gets a bunch of characters and actually breaks the regex. If you run either of these regexes, they always return false because they fail to match. It's just that we have another bug that's been masking the issue.
(I'll comment on the other bug.)
| @@ -78,10 +78,16 @@ var isSupportedHash = exports.isSupportedHash = function (msg) { | |||
| } | |||
|
|
|||
| var isSigMatchesCurve = exports.isSigMatchesCurve = function (msg) { | |||
| if(!isSignatureRx.test(msg.signature)) return new Error('signature was not canonical base64') | |||
| if(!isSignatureRx.test(msg.signature)) return | |||
There was a problem hiding this comment.
Other functions expect isSigMatchesCurve to return truthy if the signature is valid, or falsey if the signature is invalid. Returning an error here is wrong because it's interpreted as truthy, which means "this is valid" even though the value we're returning is an error. 🤦
Combined with the previous error that I've described, this function has historically always returned truthy because:
- The regex is broken and
isSignatureRx.test(msg.signature)is always false. - The error handling is broken and when
!isSignatureRx.test(msg.signature)happens we return a truthy value.
The two bugs cancel each other out so that we don't notice them, but isSigMatchesCurve() has always returned truthy, even when it shouldn't.
There was a problem hiding this comment.
Also briefly documented in my commit message: d3e2dac
|
Lets get this merged, really nice detective work there @christianbundy :-) |
Problem: Although the timestamp check in `state` was meant to be stripped out a long time ago, some code is still checking for the existence of `state.timestamp` and doing validation on it. Solution: Remove that vestigial code so that we pass new SSB-Validation-Dataset tests.
|
Done! Thanks for taking the time to review. |
Problem: Maintaining one validation dataset per module means that we
can't build the dataset collaboratively and the validation dataset tends
toward stagnation.
Solution: Use a collaborative validation dataset, which provides a
handful of new and interesting test failures. This branch includes eight
new failing tests that we should aim to resolve.