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

RESOLVED FIXED in 3.16.2

Status

P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ryan.sleevi, Assigned: ryan.sleevi)

Tracking

3.16.1
3.16.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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 )
(Assignee)

Updated

5 years ago
Assignee: nobody → ryan.sleevi
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 1

5 years ago
Created attachment 8429839 [details] [diff] [review]
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.
Attachment #8429839 - Flags: review?(rrelyea)

Comment 2

5 years ago
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
Attachment #8429839 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 8432007 [details] [diff] [review]
Restructured per Bob's request

Carrying over r+ because it's just style shifting as requested.
Attachment #8429839 - Attachment is obsolete: true
Attachment #8432007 - Flags: review+

Comment 4

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

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

r+ I concure:).

bob
Attachment #8432007 - Flags: review+
(Assignee)

Comment 5

5 years ago
Fixed in https://hg.mozilla.org/projects/nss/rev/3b3baf9909f2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 6

5 years ago
Created attachment 8436363 [details] [diff] [review]
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.
Attachment #8436363 - Flags: superreview?(rrelyea)
Attachment #8436363 - Flags: review?(ryan.sleevi)
(Assignee)

Comment 7

5 years ago
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.
Attachment #8436363 - Flags: review?(ryan.sleevi) → review+

Comment 8

5 years ago
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.
Attachment #8436363 - Flags: superreview?(rrelyea) → superreview+

Comment 9

5 years ago
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
Attachment #8436363 - Flags: checked-in+
You need to log in before you can comment on or make changes to this bug.