Web Crypto API's RSA key generation goes into infinite loop

RESOLVED FIXED in 3.35

Status

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: thanhb, Assigned: jallmann)

Tracking

({good-first-bug})

trunk
3.35
good-first-bug

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8873084 [details]
RsaBug.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

- Generate a RSA key with even public exponent, such as 2.


Actual results:

It seems that the function goes into an infinite loop and doesn't return anything or throw exception.


Expected results:

It should throw an error if it's not possible to generate a key with the given public exponent.
Component: Untriaged → DOM: Security
Product: Firefox → Core
Assignee: nobody → nobody
Component: DOM: Security → Libraries
Product: Core → NSS
Version: 53 Branch → trunk
RSA_NewKey() [1] should check that (e > 2) and (e & 1 = 1) and bail out if it isn't. Rather hard to find p/q otherwise. Should also add a few tests.

[1] https://searchfox.org/nss/source/lib/freebl/rsa.c#276
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug

Comment 2

a year ago
I would like to take this up.
Assignee: nobody → jonas.allmann
(Assignee)

Comment 3

a year ago
Attached a first draft of a patch. 
Haven't been able to test it yet due to problems building the code on my computer. 
Very unsure about the tests I've written, since it's my very first try on that.
(Assignee)

Comment 4

a year ago
Created attachment 8901184 [details] [diff] [review]
First draft of a patch.
Attachment #8901184 - Flags: review?(franziskuskiefer)
Attachment #8901184 - Flags: feedback?(franziskuskiefer)
Comment on attachment 8901184 [details] [diff] [review]
First draft of a patch.

Review of attachment 8901184 [details] [diff] [review]:
-----------------------------------------------------------------

If you initialise `e` in rsa.c before you use it, it should work.
The test needs a little more work to compile.
You can create SECItems like `SECItem input = {siBuffer, number, len};` If you have to allocate it, use scoped secitems (see scoped_ptrs.h).
Also make the RSAPrivateKey scoped.

::: gtests/freebl_gtest/rsa_unittest.cc
@@ +1,1 @@
> +

License header is missing. Copy it from another file.

@@ +1,2 @@
> +
> +#include "gtest/gtest.h"

You have to add this file to `freebl_gtest.gyp` to actually compile it. You don't have to add it to the manifest as it will only work with test builds.

@@ +10,5 @@
> + protected:
> +  void TestIfEqResult(int keySizeInBits, int publicExponent,void* result) {
> +      int exp = publicExponent;
> +      SECItem* e;
> +      SECITEM_AllocItem(NULL, e, sizeof(int));

Missing `include "secitem.h"`

@@ +11,5 @@
> +  void TestIfEqResult(int keySizeInBits, int publicExponent,void* result) {
> +      int exp = publicExponent;
> +      SECItem* e;
> +      SECITEM_AllocItem(NULL, e, sizeof(int));
> +      e->type = SECItemType siUnsignedInteger;

Not sure what the SECItemType is doing here.

@@ +12,5 @@
> +      int exp = publicExponent;
> +      SECItem* e;
> +      SECITEM_AllocItem(NULL, e, sizeof(int));
> +      e->type = SECItemType siUnsignedInteger;
> +      e->data = &exp;

You can't assign the int pointer to the char* pointer (e->data).

@@ +16,5 @@
> +      e->data = &exp;
> +      RSAPrivateKey* key = NULL;
> +      key = RSA_NewKey(keySizeInBits, e);
> +      ASSERT_TRUE(key == result);
> +      SECItem_FreeItem(e);

SECITEM_

@@ +32,5 @@
> +  }
> +}
> + //Tests that expect NULL because of RSA_NewKey failing..
> +TEST_F(RSANewKeyTest, expOneTest){
> +    TestIfEqResult(512, 1, NULL)

You should probably use at least 2048. We might refuse to generate RSA keys of this size at some point. Also, the key size sholdn't really influence this. It could stay the same in all tests.

::: lib/freebl/rsa.c
@@ +290,5 @@
>          PORT_SetError(SEC_ERROR_INVALID_ARGS);
>          return NULL;
>      }
> +    /* 1.  Set the public exponent and check if it's uneven and greater than 2.*/
> +    SECITEM_TO_MPINT(*publicExponent, &e);

This will segfault because `e` hasn't been initialised.
Attachment #8901184 - Flags: review?(franziskuskiefer)
Attachment #8901184 - Flags: feedback?(franziskuskiefer)
(Assignee)

Comment 6

a year ago
Created attachment 8902341 [details] [diff] [review]
Second Version of my patch.

Tried to resolve all of the issues from the feedback comment above.

The function itself in rsa.c should be correct now. 

I was also able to build everything and actually run the tests I wrote.
The tests do compile now, but they don't deliver the desired results yet, so there's some more work needed. I'll continue trying to resolve these problems.
 
I'm almost sure it's about how I wrap the integer value of the publicExponent in the SECItem that is then passed to the function to be tested. I haven't been able to figure out how to do that correctly. 

I was also not sure how to make the RSAPrivateKey scoped, as you suggested.
Attachment #8902341 - Flags: review?(franziskuskiefer)
Attachment #8902341 - Flags: feedback?(franziskuskiefer)
(Assignee)

Comment 7

a year ago
Created attachment 8902383 [details] [diff] [review]
ThirdDraft, just cosmetic changes.

This patch is just an intermediary step towards the fourth patch which actually solves the problems regarding the tests. 
I wasn't sure how to create an overall patch including all the changes I've made since I started to work on this patch.
(Assignee)

Comment 8

a year ago
Created attachment 8902384 [details] [diff] [review]
FourthDraft.patch

With this version, the tests compile and work as I expected.
Comment on attachment 8902341 [details] [diff] [review]
Second Version of my patch.

Review of attachment 8902341 [details] [diff] [review]:
-----------------------------------------------------------------

The rsa.c change looks good now.
Attachment #8902341 - Flags: review?(franziskuskiefer)
Attachment #8902341 - Flags: feedback?(franziskuskiefer)
Jonas, can you squash your commits and upload them as a single patch?

For the tests;
* you don't need two functions. The easiest would be to return the key from the test function and compare the result outside in `TEST_F`.
* You should make it `pubExp[32] = {0}` as you'll get random memory otherwise (it's not actually used but it might be by accident). But you could simplify the function by passing in a byte array with the encoded numbers instead of doing it generically. The tests use only 4 different (easy to encode) numbers.
Flags: needinfo?(jonas.allmann)
(Assignee)

Comment 11

a year ago
Created attachment 8902634 [details] [diff] [review]
collapsed_changes1to5.patch

I collapsed all my patches into one and fixed the uninitialized array in the test function. 
I will work on your suggestions to simplify the tests later.
Flags: needinfo?(jonas.allmann)
Attachment #8902634 - Flags: feedback?(franziskuskiefer)
Comment on attachment 8902634 [details] [diff] [review]
collapsed_changes1to5.patch

Review of attachment 8902634 [details] [diff] [review]:
-----------------------------------------------------------------

Please mark the old patches as obsolete.

::: gtests/freebl_gtest/rsa_unittest.cc
@@ +9,5 @@
> +
> +#include "blapi.h"
> +#include "secitem.h"
> +
> +// RSAPrivateKey* RSA_NewKey(int keySizeInBits, SECItem *publicExponent)

remove comment

@@ +21,5 @@
> +      unsigned char pubExp[32] = {0};
> +      int expLen = 0;
> +      unsigned int i;
> +
> +      for (i = 0; i < sizeof(int); i++) {

Please consider the suggestion to simplify this and pass in the public exponent as byte array.

@@ +65,5 @@
> +  
> +};
> +
> + //Tests that expect NULL because of RSA_NewKey failing..
> +TEST_F(RSANewKeyTest, expOneTest){

If you use TEST_P you can make this more concise.
Attachment #8902634 - Flags: feedback?(franziskuskiefer)
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 13

a year ago
Created attachment 8908364 [details] [diff] [review]
RSA_NewKey_Patch_09_14.patch

Hi Franziskus,

this is my latest Version of the Patch for this bug. 
I simplified the tests, just as you suggested. They seem to be working just fine like this. 

Are there still any improvements needed?
Attachment #8901184 - Attachment is obsolete: true
Attachment #8902341 - Attachment is obsolete: true
Attachment #8902383 - Attachment is obsolete: true
Attachment #8902384 - Attachment is obsolete: true
Attachment #8902634 - Attachment is obsolete: true
Attachment #8908364 - Flags: review?(franziskuskiefer)
Comment on attachment 8908364 [details] [diff] [review]
RSA_NewKey_Patch_09_14.patch

Review of attachment 8908364 [details] [diff] [review]:
-----------------------------------------------------------------

Let's use this patch to do some administrative stuff.
Can you push the next version of this patch to try [1] (request level 1 commit access first as described, Tim or I can vouch for you)?
NSS also uses Phabricator for reviews nowadays and it's integrated with bugzilla now. So you can go to https://phabricator.services.mozilla.com/ and link the account there with your bugzilla account. To upload your next version of the patch to phabricator you use arcanist (here's a guide on how to that [2], let me know if you have any questions).

[1] https://wiki.mozilla.org/NSS:TryServer
[2] https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/

::: gtests/freebl_gtest/rsa_unittest.cc
@@ +1,4 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +// You can obtain one at http://mozilla.org/MPL/2.0/.
> +

If you run ./mach clang-format (this requires docker), it will format the code for you and get rid of this empty line.

@@ +15,5 @@
> +  RSAPrivateKey* CrateKeyWithExponent(int keySizeInBits, unsigned char publicExponent) {
> +
> +      SECItem exp = {siBuffer, 0, 0};
> +      unsigned char pubExp[1] = {0};
> +      pubExp[0] = publicExponent;

You can put the publicExponent in pubExp directly.

@@ +20,5 @@
> +      exp.data = pubExp;
> +      exp.len = 1;
> +
> +      RSAPrivateKey* key = NULL;
> +      key = RSA_NewKey(keySizeInBits, &exp);

You don't need the temp key variable here.

But more importantly the key leaks (at least in the one test where it's not NULL). The best thing to do would be to use a scoped pointer for RSAPrivateKey. You can do it similarly to here [1].

[1] https://searchfox.org/nss/rev/9626433e0e5ebe830bbd6a29a621e3c23745ecaa/gtests/freebl_gtest/blake2b_unittest.cc#31
Attachment #8908364 - Flags: review?(franziskuskiefer)
Priority: P3 → P1
Comment on attachment 8922298 [details]
Patch for bug 1369091

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D165#4976
Attachment #8922298 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/de569c50ac57ceef36f84cfe9298e182af24623d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → 3.35
You need to log in before you can comment on or make changes to this bug.