Bug 896651

Summary: PEM module trashes private keys if login fails
Product: [Fedora] Fedora Reporter: Nalin Dahyabhai <nalin>
Component: nssAssignee: Elio Maldonado Batiz <emaldona>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 18CC: emaldona, kdudka, kengert, rrelyea
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: nss-3.14.3-1.fc18 nss-3.14.3-1.fc17 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1002204 1002205 (view as bug list) Environment:
Last Closed: 2013-02-28 07:04:23 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1002204, 1002205    
Attachments:
Description Flags
patch to only replace the encrypted data with decrypted data after it's vetted
none
the same patch, with redundant assignment removed none

Description Nalin Dahyabhai 2013-01-17 16:54:39 UTC 
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
Comment 1 Nalin Dahyabhai 2013-01-17 16:55:54 UTC 
Created attachment 680373 [details]
patch to only replace the encrypted data with decrypted data after it's vetted
Comment 2 Kamil Dudka 2013-01-21 11:33:33 UTC 
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.
It would also help to put there a comment saying which error case is being
handled by the condition.
Comment 3 Nalin Dahyabhai 2013-01-23 16:30:49 UTC 
(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.
Comment 4 Nalin Dahyabhai 2013-02-13 18:33:47 UTC 
Created attachment 696984 [details]
the same patch, with redundant assignment removed
Comment 5 Fedora Update System 2013-02-22 19:33:30 UTC 
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
Comment 6 Fedora Update System 2013-02-24 08:41:59 UTC 
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).
Comment 7 Fedora Update System 2013-02-25 18:50:01 UTC 
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
Comment 8 Fedora Update System 2013-02-28 07:04:25 UTC 
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.
Comment 9 Fedora Update System 2013-03-14 02:40:07 UTC 
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.