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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Thanh Bui]

Attachment: 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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 2

[Aastha]

I would like to take this up.

Comment 3

[Jonas Allmann [:jallmann]]

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.

Comment 4

[Jonas Allmann [:jallmann]]

Attachment: First draft of a patch.

Comment 5

[Franziskus Kiefer [:franziskus]]

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.

Comment 6

[Jonas Allmann [:jallmann]]

Attachment: 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.

Comment 7

[Jonas Allmann [:jallmann]]

Attachment: 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.

Comment 8

[Jonas Allmann [:jallmann]]

Attachment: FourthDraft.patch

With this version, the tests compile and work as I expected.

Comment 9

[Franziskus Kiefer [:franziskus]]

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.

Comment 10

[Franziskus Kiefer [:franziskus]]

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.

Comment 11

[Jonas Allmann [:jallmann]]

Attachment: 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.

Comment 12

[Franziskus Kiefer [:franziskus]]

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.

Comment 13

[Jonas Allmann [:jallmann]]

Attachment: 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?

Comment 14

[Franziskus Kiefer [:franziskus]]

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

Comment 15

[Phabricator Automation]

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

Comment 16

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/de569c50ac57ceef36f84cfe9298e182af24623d