1016811 - Softoken does not ensure the consistency of RSA private keys being imported

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ryan Sleevi]

Softoken, particularly when backed by the SQLite DB, does not ensure the consistency of the RSA private key values. When the legacydb is used, however, NSS does check the consistency.

This is measured by looking for callsites to RSA_PrivateKeyCheck. Only the legacydb calls it.

Presumably, Softoken should handle this as part of sftk_handlePrivateKeyObject for RSA keys ( http://mxr.mozilla.org/nss/source/lib/softoken/pkcs11.c#998 ), much in the same way it ensures consistency in sftk_fillRSAPrivateKey ( http://mxr.mozilla.org/nss/source/lib/softoken/pkcs11.c#1948 )

Comment 1

[Ryan Sleevi]

Attachment: Patch to always perform consistency check for RSA private keys in softoken

The use case for this is handling import of 'hostile' data (eg: JWK, SPKIs) as part of Web Crypto. NSS should reject inconsistent parameters, as otherwise the application would need to perform these checks.

Comment 2

[Robert Relyea]

Comment on attachment 8429839 [details] [diff] [review]
Patch to always perform consistency check for RSA private keys in softoken

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

Good patch. I have a minor comment in PKCS #11.

r+

::: lib/softoken/pkcs11.c
@@ +1055,5 @@
> +	    rv = sftk_verifyRSAPrivateKey(object, PR_TRUE);
> +	    if (rv != SECSuccess) {
> +		return CKR_TEMPLATE_INCOMPLETE;
> +	    }
> +	} else {

even though I've r+ this, I would like this section fixed. It would be cleaner and easier to read if the structure:

if (condition) {
        .
        .
        .
    do_something(true)
    bail if fail
} else {
    do_something(false)
    bail if fail
}

is changed to 
bool fill = false;


if (condition) {
   .
   .
   .
   fill = true;
}

do_something(fill)
bail if fail

Comment 3

[Ryan Sleevi]

Attachment: Restructured per Bob's request

Carrying over r+ because it's just style shifting as requested.

Comment 4

[Robert Relyea]

Comment on attachment 8432007 [details] [diff] [review]
Restructured per Bob's request

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

r+ I concure:).

bob

Comment 5

[Ryan Sleevi]

Fixed in https://hg.mozilla.org/projects/nss/rev/3b3baf9909f2

Comment 6

[Wan-Teh Chang]

Attachment: Fix copy-and-paste typos. Fix apparent (but probably not real) SFTKAttribute leaks

I noticed these issues while investigating bug 1021102 (SECKEY_CopyPrivateKey
failure). These issues are not the cause of that bug.

sftk_FreeAttribute only frees certain sftk_FreeAttribute objects. So the
missing sftk_FreeAttribute calls are not necessarily leaks. But it's good
to add them.

Comment 7

[Ryan Sleevi]

Comment on attachment 8436363 [details] [diff] [review]
Fix copy-and-paste typos. Fix apparent (but probably not real) SFTKAttribute leaks

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

Dang it. I had fixed these, but then screwed it up when using MQ. LGTM.

Comment 8

[Robert Relyea]

Comment on attachment 8436363 [details] [diff] [review]
Fix copy-and-paste typos. Fix apparent (but probably not real) SFTKAttribute leaks

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

Actually I think this is a real leak fix. (we are at least leaking an attribute reference.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8436363 [details] [diff] [review]
Fix copy-and-paste typos. Fix apparent (but probably not real) SFTKAttribute leaks

Patch checked in: https://hg.mozilla.org/projects/nss/rev/bfff77d7ae78