Implement RSA verification #4952
Conversation
🦋 Changeset detectedLatest commit: 56eac45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Managed to navigate through RFC 8017 and mostly looks good.
contracts/utils/cryptography/RSA.sol
Outdated
| unchecked { | ||
| // cache and check length | ||
| uint256 length = mod.length; | ||
| if (length < 0x40 || length != sig.length) { |
There was a problem hiding this comment.
I guess we reject length less than 0x40 because it wouldn't be secure. I wonder if 0x40 was arbitrarily chosen. If so, we need to evaluate it carefully, as far as I remember, RSA's security is p * q so a 512 bits signature is crackable in reasonable time.
Found this as a reference, but seems like 512 bits (0x40 bytes) signatures are pretty much broken.
https://github.com/tomrittervg/cloud-and-control/blob/master/gnfs-info/factoring-howto.txt
RFC 3447 is from 2003 and was superseded by RFC 8017, though, I couldn't find a recommendation for the mod length. Allegedly, 512 bits security was first broken in 1999, so my estimations say that we might increase this to 0x80 at least.
Needs discussion
|
I took the freedom of renaming the functions since one that receives a Similarly, I renamed the function arguments to match those referenced in RFC 8017. I think it's cleaner
Honestly, I don't think so, we'd require to adjust the param checks according to the length of the digest since we're counting backward to get the digest and allow optional digest info parameters. I'm marking this as ready for review since I think it's already good enough and I already feel comfortable with its security. |
There was a problem hiding this comment.
In my opinion, I think the last thing to resolve is whether to restrict n to have a minimum length or not. I added a note (probably should be a warning) suggesting that no key with length less than 2048 bits should be used.
If we don't find a reason to continue allowing smaller lengths, then I'd advocate for restricting the function.
Aside from that, we need to cover the missing test cases (should be trivial) and it'll be good to go
|
Line 106 is pretty difficult to reach as it requires to set up an attack where we forge two messages such that once signed we can derive a third signature that decrypts wrongly to the message. (Note by "decrypt" I'm referring to the I think it's ok if we don't have a test case for that rn. In the future we should look for adding fuzzing, however, we don't have an easy way of fuzzing keys unless we use Also, I marked |
There was a problem hiding this comment.
Looks solid and I feel comfortable with the result, I'll leave it to your consideration @Amxx.
test/utils/cryptography/RSA.test.js
Outdated
| it('returns false if the modexp operation fails', async function () { | ||
| const data = ethers.toUtf8Bytes('hello world!'); | ||
| const sig = | ||
| '0xa0073057133ff3758e7e111b4d7441f1d8cbe4b2dd5ee4316a14264290dee5ed7f175716639bd9bb43a14e4f9fcb9e84dedd35e2205caac04828b2c053f68176d971ea88534dd2eeec903043c3469fc69c206b2a8694fd262488441ed8852280c3d4994e9d42bd1d575c7024095f1a20665925c2175e089c0d731471f6cc145404edf5559fd2276e45e448086f71c78d0cc6628fad394a34e51e8c10bc39bfe09ed2f5f742cc68bee899d0a41e4c75b7b80afd1c321d89ccd9fe8197c44624d91cc935dfa48de3c201099b5b417be748aef29248527e8bbb173cab76b48478d4177b338fe1f1244e64d7d23f07add560d5ad50b68d6649a49d7bc3db686daaa7'; | ||
| const exp = '0x03'; | ||
| const mod = | ||
| '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'; | ||
| expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.false; |
There was a problem hiding this comment.
this test is actually not reverting at the modExp step, but instead here because of sp > np. is there a way we can have more accurate checks when the return data is a boolean? maybe by measuring gas?
test/utils/cryptography/RSA.test.js
Outdated
| }); | ||
|
|
||
| it('returns false if the modexp operation fails', async function () { | ||
| const data = ethers.toUtf8Bytes('hello world!'); |
There was a problem hiding this comment.
if module is 0, then it will revert at the s < n check, so for this to revert we need to revert the precompile directly
| const data = ethers.toUtf8Bytes('hello world!'); | |
| setCode("0x0500000000000000000000000000000000000000", "0x60006000fd"); | |
| const data = ethers.toUtf8Bytes('hello world!'); |
Fix LIB-1238
Work in progress. Based on https://github.com/adria0/SolRsaVerify
Tests:
SigVer15_186-3.rspfrom here and automatically run all test examples.For now, only sha256 is supported.
@nobles/hashprovides all the function we need to generate the digest, andSigVer15_186-3.rspcontains more tests.Do we want to support more?
PR Checklist
npx changeset add)