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

Implement RSA verification #4952

Merged
merged 39 commits into from
Jun 11, 2024
Merged

Implement RSA verification #4952

merged 39 commits into from
Jun 11, 2024

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 12, 2024

Fix LIB-1238

Work in progress. Based on https://github.com/adria0/SolRsaVerify

Tests:

For now, only sha256 is supported. @nobles/hash provides all the function we need to generate the digest, and SigVer15_186-3.rsp contains more tests.

Do we want to support more?

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Mar 12, 2024

🦋 Changeset detected

Latest commit: f50d89a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

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

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Managed to navigate through RFC 8017 and mostly looks good.

unchecked {
// cache and check length
uint256 length = mod.length;
if (length < 0x40 || length != sig.length) {
Copy link
Member

Choose a reason for hiding this comment

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

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

contracts/utils/cryptography/RSA.sol Outdated Show resolved Hide resolved
contracts/utils/cryptography/RSA.sol Outdated Show resolved Hide resolved
@ernestognw
Copy link
Member

ernestognw commented Jun 4, 2024

I took the freedom of renaming the functions since one that receives a bytes32 digest is not exclusively sha256 as far as I can tell. Theoretically, a user can sign with RSA using any digest they like.

Similarly, I renamed the function arguments to match those referenced in RFC 8017. I think it's cleaner

For now, only sha256 is supported. @nobles/hash provides all the function we need to generate the digest, and SigVer15_186-3.rsp contains more tests.

Do we want to support more?

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.

@ernestognw ernestognw marked this pull request as ready for review June 4, 2024 03:41
@ernestognw ernestognw requested a review from cairoeth June 4, 2024 03:41
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

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

.changeset/curvy-crabs-repeat.md Outdated Show resolved Hide resolved
Comment on lines 97 to 104
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It feels like the only way for modExp to fail is if mod = 0. If we check that s < n, then we know that n > 0, and there may be no way for modExp to fail.

test/utils/cryptography/RSA.test.js Outdated Show resolved Hide resolved
ernestognw
ernestognw previously approved these changes Jun 11, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Added some docs as well. LGTM.

I'd wait for a second approval.

Copy link
Contributor

@cairoeth cairoeth left a comment

Choose a reason for hiding this comment

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

lgtm -- @ernestognw: I added a replay protection notice on the docs page

@ernestognw
Copy link
Member

lgtm -- @ernestognw: I added a replay protection notice on the docs page

Thanks, looking good. Approving again

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.

4 participants