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

RSA signatures larger than the modulus are accepted #272

Closed
randombit opened this issue Mar 9, 2023 · 2 comments · Fixed by #306
Closed

RSA signatures larger than the modulus are accepted #272

randombit opened this issue Mar 9, 2023 · 2 comments · Fixed by #306
Labels

Comments

@randombit
Copy link

raw_encryption_primitive is used for verifying signatures. It does not check that the decoded integer value is less than the modulus, thus if s is a valid signature it will also accept s+k*n where n is the public modulus and k is any positive integer.

This introduces signature malleability, which is probably not an enormous problem in most applications, but neither does it seem desirable.

A related issue is that signatures are decoded as big integers but it is never checked that the length of the signature is equal to the public modulus length. So if sig is the binary encoding of a valid signature, prefixing that signature with any number of zero bytes will also be accepted as valid.

This may affect ciphertext decryption as well, but I haven't checked this.

The following patch to the tests demonstrates the issues

diff --git a/src/pkcs1v15.rs b/src/pkcs1v15.rs
index 5d9bac4..50b6dc1 100644
--- a/src/pkcs1v15.rs
+++ b/src/pkcs1v15.rs
@@ -1152,15 +1152,23 @@ mod tests {
             (
                 "Test.\n",
                 hex!(
-                    "a4f3fa6ea93bcdd0c57be020c1193ecbfd6f200a3d95c409769b029578fa0e33"
+                    "00a4f3fa6ea93bcdd0c57be020c1193ecbfd6f200a3d95c409769b029578fa0e33"
                     "6ad9a347600e40d3ae823b8c7e6bad88cc07c1d54c3a1523cbbb6d58efc362ae"
                 ),
                 true,
             ),
             (
                 "Test.\n",
+                // signature + modulus
                 hex!(
-                    "a4f3fa6ea93bcdd0c57be020c1193ecbfd6f200a3d95c409769b029578fa0e33"
+                    "01578d09b86db9c85d997c8e8b0e34c9076782843561884f09b2969a0e83d8a88033052e71e182beb26f0d9e2363a5b64bfd7040c7a88609b7375d2e3ef3e1ffc3"
+                ),
+                true,
+            ),
+            (
+                "Test.\n",
+                hex!(
+                    "00a4f3fa6ea93bcdd0c57be020c1193ecbfd6f200a3d95c409769b029578fa0e33"
                     "6ad9a347600e40d3ae823b8c7e6bad88cc07c1d54c3a1523cbbb6d58efc362af"
                 ),
                 false,
tarcieri added a commit that referenced this issue Apr 18, 2023
This one half of #220.

Doing anything with a signature involves converting it from bytes into a
`BigUint`, so this changes the inner type the latter which is more
useful.

It should also help address #272, since it will enable doing those sort
of checks more eagerly.
tarcieri added a commit that referenced this issue Apr 18, 2023
This one half of #220.

Doing anything with a signature involves converting it from bytes into a
`BigUint`, so this changes the inner type the latter which is more
useful.

It should also help address #272, since it will enable doing those sort
of checks more eagerly.
@tarcieri
Copy link
Member

tarcieri commented Apr 24, 2023

A related issue is that signatures are decoded as big integers but it is never checked that the length of the signature is equal to the public modulus length.

I think this only applies to RSASSA-PKCS#1v1.5. It seems like the PSS implementation has always had checks for the signature length:

5d28baf#diff-22b3e2164bd21d723f22be3c3381f4f58ba9c360f2079d76b93bd50b37e43611R21

Anyway, will get them added to PKCS#1v1.5 too.

tarcieri added a commit that referenced this issue Apr 25, 2023
In both the PKCS#1v1.5 and PSS implementations, checks the signature
value to ensure it does not overflow the modulus.

In the PKCS#1v1.5 implementation, checks the signature length to ensure
it matches the public key size. The PSS implementation was already doing
this.

Closes #272
@tarcieri
Copy link
Member

I believe #306 should address these concerns

tarcieri added a commit that referenced this issue Apr 25, 2023
In both the PKCS#1v1.5 and PSS implementations, checks the signature
value to ensure it does not overflow the modulus.

In the PKCS#1v1.5 implementation, checks the signature length to ensure
it matches the public key size. The PSS implementation was already doing
this.

Closes #272
gitlab-dfinity pushed a commit to dfinity/ic that referenced this issue Aug 24, 2023
chore(crypto): CRP-2038: Bump rsa version

The previous version `rsa = "0.4.0"` in `ic-crypto-internal-basic-sig-rsa-pkcs1` depends on an older version on `simple_asn1`, which in turn uses `chrono`, which makes compiling crates to wasm difficult. The `rsa` version `0.6.1`, which is already used elsewhere, depends on a newer version of `simple_asn1`, which doesn't use `chrono`. This MR bumps the version to the latest version `0.9.2`. The `0.9` version also includes a fix for [this bug](RustCrypto/RSA#272), where RSA signatures larger than the modulus are accepted. 

See merge request dfinity-lab/public/ic!14315
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants