Bug 1002204 - PEM module trashes private keys if login fails
PEM module trashes private keys if login fails
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: nss (Show other bugs)
7.0
Unspecified Unspecified
medium Severity medium
: rc
: ---
Assigned To: Elio Maldonado Batiz
Hubert Kario
: TestCaseNeeded
Depends On: 896651 1002205
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-28 11:36 EDT by Elio Maldonado Batiz
Modified: 2014-06-16 06:26 EDT (History)
7 users (show)

See Also:
Fixed In Version: nss-3.15.2-4.el7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 896651
Environment:
Last Closed: 2014-06-16 06:26:53 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Elio Maldonado Batiz 2013-08-28 11:36:09 EDT
+++ This bug was initially created as a clone of Bug #896651 +++

Description of problem:
When an encrypted private key is imported into a slot in the PEM module, the module sets the slot's needsLogin flag to signal that a password is required.

When the application attempts to log in to the token, the module attempts to decrypt the private key, but after decryption, it overwrites the encrypted key data with the decrypted data before verifying that the decrypted data isn't just the garbage that you get when you decrypt with an incorrect key.  After that, there's no way to recover the data.

It really should wait until after SEC_QuickDERDecodeItem() succeeds before modifying the contents of io->u.key.key.privateKey.

Version-Release number of selected component (if applicable):
nss-3.14.1-2.fc18

--- Additional comment from Nalin Dahyabhai on 2013-01-17 11:55:54 EST ---



--- Additional comment from Kamil Dudka on 2013-01-21 06:33:33 EST ---

> --- mozilla/security/nss/lib/ckfw/pem/psession.c
> +++ mozilla/security/nss/lib/ckfw/pem/psession.c
> @@ -230,6 +230,7 @@ pem_mdSession_Login
>      unsigned int len = 0;
>      NSSLOWKEYPrivateKey *lpk = NULL;
>      PLArenaPool *arena;
> +    SECItem plain;
>      int i;
>  
>      fwSlot = NSSCKFWToken_GetFWSlot(fwToken);
> @@ -306,23 +321,31 @@ pem_mdSession_Login
>      lpk->keyType = NSSLOWKEYRSAKey;
>      prepare_low_rsa_priv_key_for_asn1(lpk);
>  
> -    nss_ZFreeIf(io->u.key.key.privateKey->data);
> -    io->u.key.key.privateKey->len = len - output[len - 1];
> -    io->u.key.key.privateKey->data =
> -        (void *) nss_ZAlloc(NULL, io->u.key.key.privateKey->len);
> -    memcpy(io->u.key.key.privateKey->data, output, len - output[len - 1]);
> +    if (output[len - 1] > 8) {
> +        rv = CKR_GENERAL_ERROR;
> +        /* goto loser; */
> +    }

Why is the goto statement commented out?  It makes the assignment redundant.
It would also help to put there a comment saying which error case is being
handled by the condition.

--- Additional comment from Nalin Dahyabhai on 2013-01-23 11:30:49 EST ---

(In reply to comment #2)
> Comment on attachment 680373 [details]
> patch to only replace the encrypted data with decrypted data after it's
> vetted
> 
> > --- mozilla/security/nss/lib/ckfw/pem/psession.c
> > +++ mozilla/security/nss/lib/ckfw/pem/psession.c
> > @@ -230,6 +230,7 @@ pem_mdSession_Login
> >      unsigned int len = 0;
> >      NSSLOWKEYPrivateKey *lpk = NULL;
> >      PLArenaPool *arena;
> > +    SECItem plain;
> >      int i;
> >  
> >      fwSlot = NSSCKFWToken_GetFWSlot(fwToken);
> > @@ -306,23 +321,31 @@ pem_mdSession_Login
> >      lpk->keyType = NSSLOWKEYRSAKey;
> >      prepare_low_rsa_priv_key_for_asn1(lpk);
> >  
> > -    nss_ZFreeIf(io->u.key.key.privateKey->data);
> > -    io->u.key.key.privateKey->len = len - output[len - 1];
> > -    io->u.key.key.privateKey->data =
> > -        (void *) nss_ZAlloc(NULL, io->u.key.key.privateKey->len);
> > -    memcpy(io->u.key.key.privateKey->data, output, len - output[len - 1]);
> > +    if (output[len - 1] > 8) {
> > +        rv = CKR_GENERAL_ERROR;
> > +        /* goto loser; */
> > +    }
> 
> Why is the goto statement commented out?  It makes the assignment redundant.

My early assumption that the size of the padding would always be smaller than the cipher block size proved to be incorrect.  Feel free to drop that part.

--- Additional comment from Nalin Dahyabhai on 2013-02-13 13:33:47 EST ---



--- Additional comment from Fedora Update System on 2013-02-22 14:33:30 EST ---

nss-3.14.3-1.fc18,nss-softokn-3.14.3-1.fc18,nss-util-3.14.3-1.fc18,nspr-4.9.5-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/nss-3.14.3-1.fc18,nss-softokn-3.14.3-1.fc18,nss-util-3.14.3-1.fc18,nspr-4.9.5-2.fc18

--- Additional comment from Fedora Update System on 2013-02-24 03:41:59 EST ---

Package nss-3.14.3-1.fc18, nss-softokn-3.14.3-1.fc18, nss-util-3.14.3-1.fc18, nspr-4.9.5-2.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing nss-3.14.3-1.fc18 nss-softokn-3.14.3-1.fc18 nss-util-3.14.3-1.fc18 nspr-4.9.5-2.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-2929/nss-3.14.3-1.fc18,nss-softokn-3.14.3-1.fc18,nss-util-3.14.3-1.fc18,nspr-4.9.5-2.fc18
then log in and leave karma (feedback).

--- Additional comment from Fedora Update System on 2013-02-25 13:50:01 EST ---

nss-3.14.3-1.fc17,nss-softokn-3.14.3-1.fc17,nss-util-3.14.3-1.fc17,nspr-4.9.5-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/nss-3.14.3-1.fc17,nss-softokn-3.14.3-1.fc17,nss-util-3.14.3-1.fc17,nspr-4.9.5-2.fc17

--- Additional comment from Fedora Update System on 2013-02-28 02:04:25 EST ---

nss-3.14.3-1.fc18, nss-softokn-3.14.3-1.fc18, nss-util-3.14.3-1.fc18, nspr-4.9.5-2.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

--- Additional comment from Fedora Update System on 2013-03-13 22:40:07 EDT ---

nss-3.14.3-1.fc17, nss-softokn-3.14.3-1.fc17, nss-util-3.14.3-1.fc17, nspr-4.9.5-2.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 2 Elio Maldonado Batiz 2013-10-29 19:08:00 EDT
This bug was cloned from Bug #896651 and fixed there via this patch
https://bugzilla.redhat.com/attachment.cgi?id=696984 to ...../pem/psession.c
The changes in question show up in pem/pssesion.c. By updating nss-pem to nss-pem-20130405.tar.bz2 we picked up the fix.
Comment 7 Elio Maldonado Batiz 2014-01-15 15:52:16 EST
(In reply to Karel Srot from comment #6)
> Hi Elio,
> could you please provide a bit more details HowToTest instructions? I do not
> understand how to reproduce the bug and verify the fix. Thank you.

Nalin, could you advise Karel on how to design a simple reproducer.
Comment 8 Elio Maldonado Batiz 2014-01-15 15:57:13 EST
Also see https://bugzilla.redhat.com/show_bug.cgi?id=1048713#c11 and a few comments before. I suspect that fix for this bug may help with the fix for that other one.
Comment 14 Ludek Smid 2014-06-16 06:26:53 EDT
This request was resolved in Red Hat Enterprise Linux 7.0.

Contact your manager or support representative in case you have further questions about the request.

Note You need to log in before you can comment on or make changes to this bug.