Bug 1002204

+++ 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.