Improve Webcrypto error handling, resolve several internal errors and add clarifying comments #382
Conversation
fhanau
changed the title
fhanau
changed the title
|
Needs an internal PR to run through CI. |
| // TODO: boringssl silently uses (modulusLength & ~127) for the key size, i.e. it rounds down to | ||
| // the closest multiple of 128 bits. This can easily cause confusion when non-standard key sizes | ||
| // are requested. Ideally we would throw an error when trying to create keys where the size would | ||
| // be rounded down, but this would likely break existing scripts. |
There was a problem hiding this comment.
We could emit a log message to confirm whether we'd break anyone, like here:
workerd/src/workerd/io/worker.c++
Lines 377 to 381 in 0efe8ae
There was a problem hiding this comment.
We could also do the same for the other cases where you identify that we could theoretically be introducing a breaking change, e.g. when calling validateRsaParams() during RSA key import, and 62/63-byte RSASSA-PKCS1-v1_5 keys used with SHA-256.
There was a problem hiding this comment.
Sounds good, I'll try doing that. Can you explain why there's a lambda expression around this instance of KJ_LOG – is it related to how the warning is reported?
There was a problem hiding this comment.
Never mind, it's used to only report the warning once instead of repeating it
There was a problem hiding this comment.
Switched to log messages to detect potential breakage for key signing and validating imported keys. For the key size issue I'd expect that there likely are scripts using sizes other than multiples of 128, so I left the TODO message as-is for now.
There is an internal branch (felix/crypto-errors2), would that generally be enough or is a PR preferred? The internal tests seem to be failing for some reason though – I'll take care of the suggested changes, rebase and see if that fixes it. |
|
A branch is fine so long as CI shows that internal changes aren't also needed for this to land, and that nothing breaks there of course :-). The label can be removed once that's clear :-) |
fhanau
force-pushed
the
felix/crypto-errors2
branch
from
e4fe56e
to
8e1bd76
Compare
|
Internal branch felix/crypto-errors2 now passes all test cases. |
| static bool logOnce KJ_UNUSED = ([v] { | ||
| KJ_LOG(WARNING, "Imported RSA key has invalid publicExponent ", *v,"."); | ||
| return true; | ||
| })(); |
There was a problem hiding this comment.
No action for this PR but just noting... this is a common enough pattern now that we could probably justify adding a new LOG_WARNING_ONCE("...") macro to simplify.
There was a problem hiding this comment.
That's a good suggestion! Looks like this approach is not used that much elsewhere, but if I use it again in the future I'll try adding a macro
|
While there are some eyes on subtle crypto 🙏 #138, #48, and of course at least Ed25519 from https://wicg.github.io/webcrypto-secure-curves/ |
…necessarily keys represented by raw data
fhanau
force-pushed
the
felix/crypto-errors2
branch
from
b65f33d
to
8044842
Compare
|
@panva I'll work on the secure curves and take a look at the other issues in the coming days. |
Commit messages and comments should explain the changes reasonably well, please let me know if you find anything unclear.
One issue that merits discussion is whether to validate imported RSA keys. Calling validateRsaParams() should only reject keys that are unreasonably large or possibly insecure based on the modulus size or exponent, but may break existing scripts that import such keys. Fortunately, the other changes add some key size checks during signing and should be sufficient to prevent internal errors even if unreasonably small keys are imported.
For signing with RSASSA-PKCS1-v1_5, in very few cases errors will be reported where there previously was no error (e.g. when importing a key of exactly 62 or 63 bytes, using SHA256 as the hash digest and trying to use it for signing). I think the change – requiring the key to be at least 32 bytes larger than the hash digest when 26 or 30 would be sufficient depending on which hash is used – is sensible since there is no good reason to use keys of these specific sizes, boringssl doesn't generate keys of that size already and it is unlikely anyone is using this exact configuration.