401240 - crash if I try to backup a PKCS#12 certificate from a PKCS#11 provider [@ nsPKCS12Blob::~nsPKCS12Blob()]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Stefan Kaeser]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; Arcor 5.006; .NET CLR 1.1.4322; InfoPath.2; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

During backup of a PKCS#12 certificate, Mozilla Firefox crashes with an unhandled exception. 

Reproducible: Always

Steps to Reproduce:
Preconditions:
1. Install Firefox
2. Install Infineon TPM Professional Package 3.0 (includes PKCS#11 provider)
3. Add PKCS#11 provider to Firefox (Tools->Options->Advanced->Encryption->Security Devices) 
4. Request a certificate using PKCS#11 provider and install the certificate.

Steps to reproduce: 
1. Start Firefox.
2. Click Tools->Options->Advanced->Encryption->View Certificates.
3. Firefox' "Password required" dialog pops up for the PKCS#11 provider. Click "Cancel". 
4. Firefox "Certificate manager" dialog pops up. Select certificate from PKCS#11 provider and click "Backup".
5. Choose filename. Click "Save".
6. Choose a certificate backup password. Click "OK".
7. Password dialog from PKCS#11 provider pops up. Click "Cancel".
8. Firefox crashes with an unhandled exception. 

Actual Results:  
Firefox crashed with an unhandled exception.

Expected Results:  
Firefox should not crash.

Comment 1

[Stefan Kaeser]

Attachment: Crash analysis with WinDBG

Comment 2

[Wayne Mery (:wsmwk)]

Stefan, can this still be reproduced?

this is probably nsPKCS12Blob::~nsPKCS12Blob().
only a handful per month.

an example from FF 3.5.5 bp-096d9d2f-40e3-4ac3-9f6a-1ed642091216
"Tried exporting a certificate to PKCS 12 file."

0	xul.dll	nsPKCS12Blob::~nsPKCS12Blob	 security/manager/ssl/src/nsPKCS12Blob.cpp:102
1	xul.dll	nsPKCS12Blob::~nsPKCS12Blob	security/manager/ssl/src/nsPKCS12Blob.cpp:104
2	xul.dll	nsPKCS12Blob::`scalar deleting destructor'	
3	xul.dll	nsProxyObjectManager::GetProxyForObject	xpcom/proxy/src/nsProxyObjectManager.cpp:210
4	xul.dll	NS_GetProxyForObject	xpcom/proxy/src/nsProxyObjectManager.cpp:340
5	xul.dll	PK11PasswordPrompt	security/manager/ssl/src/nsNSSCallbacks.cpp:794
6	nss3.dll	PK11_DoPassword	security/nss/lib/pk11wrap/pk11auth.c:594
7	nss3.dll	pk11_findKeyObjectByDERCert	security/nss/lib/pk11wrap/pk11cert.c:2057 

other nsPKCS crashes...
nsPKCS12Blob::newPKCS12FilePassword(SECItemStr*)
nsPKCS12Blob::getPKCS12FilePassword(SECItemStr*)

Comment 3

[Stefan Kaeser]

Wayne, I can reproduce this with FF 3.5.7.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Attachment: bug 401240 - part 1/2 - run ./mach clang-format on nsPKCS12Blob

Review commit: https://reviewboard.mozilla.org/r/243546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243546/

Comment 6

[Dana Keeler (she/her) [:keeler]]

Attachment: bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

Review commit: https://reviewboard.mozilla.org/r/243548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243548/

Comment 7

[Dana Keeler (she/her) [:keeler]]

I still want to figure out a testcase that exercises the exact issue in this bug, but in the meantime I thought I'd ask for an initial review.

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8975198 [details]
bug 401240 - part 1/2 - run ./mach clang-format on nsPKCS12Blob

https://reviewboard.mozilla.org/r/243546/#review249496

Comment 9

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

https://reviewboard.mozilla.org/r/243548/#review249498

Looks pretty good already. A couple nits.
Clearing r? while waiting for a test.

::: security/manager/ssl/nsNSSComponent.cpp:2648
(Diff revision 1)
>    }
>  
>    return NS_OK;
>  }
>  
> +// required to be set by NSS (to do PKCS#12), but since we've already got

Required

::: security/manager/ssl/nsNSSHelper.h:14
(Diff revision 1)
>  
>  #include "nsIInterfaceRequestor.h"
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "pk11func.h"
>  
> -//
> +// Implementation of an nsIInterfaceRequestor for use as context for NSS calls

.

::: security/manager/ssl/nsPKCS12Blob.h:74
(Diff revision 1)
> +
>    nsresult ImportFromFileHelper(nsIFile* file,
>                                  ImportMode aImportMode,
>                                  RetryReason& aWantRetry);
>  
> -  // NSPR file I/O for export file
> +  static SECItem* nicknameCollision(SECItem*, PRBool*, void*);

All other functions have argument names.

::: security/manager/ssl/nsPKCS12Blob.cpp:64
(Diff revision 1)
> -nsresult
> -nsPKCS12Blob::ImportFromFileHelper(nsIFile* file,
> -                                   nsPKCS12Blob::ImportMode aImportMode,
> -                                   nsPKCS12Blob::RetryReason& aWantRetry)
> -{
> -  nsresult rv = NS_OK;
> +void
> +nsPKCS12Blob::handleImportError(PRErrorCode nssError, RetryReason& retryReason,
> +                                uint32_t passwordLengthInBytes)
> +{
> +  if (nssError == SEC_ERROR_BAD_PASSWORD) {
> +    if (passwordLengthInBytes == 2) {

Maybe add a comment that `length == 2` implies that the password is empty and contains only the null terminator.

::: security/manager/ssl/nsPKCS12Blob.cpp:202
(Diff revision 1)
> -    // internal token.  Most, if not all, smart card vendors
> -    // won't let you extract the private key (in any way
> +    // token. Most, if not all, smart card vendors won't let you extract the
> +    // private key (in any way shape or form) from the card. So let's punt if
> -    // shape or form) from the card.  So let's punt if
>      // the cert is not in the internal db.
>      if (nssCert->slot && !PK11_IsInternal(nssCert->slot)) {
>        // we aren't the internal token, see if the key is extractable.

We

::: security/manager/ssl/nsPKCS12Blob.cpp:222
(Diff revision 1)
>      if (!SEC_PKCS12IsEncryptionAllowed() || PK11_IsFIPS()) {
>        certSafe = keySafe;
>      } else {
>        certSafe = SEC_PKCS12CreatePasswordPrivSafe(
> -        ecx, &unicodePw, SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_40_BIT_RC2_CBC);
> +        ecx.get(), &unicodePw,
> +        SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_40_BIT_RC2_CBC);

This always makes me sad :(

::: security/manager/ssl/nsPKCS12Blob.cpp:250
(Diff revision 1)
> +
> +  UniquePRFileDesc prFile;
> +  PRFileDesc* rawPRFile;
>    rv = file->OpenNSPRFileDesc(
> -    PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE, 0664, &mTmpFile);
> -  if (NS_FAILED(rv) || !this->mTmpFile)
> +    PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE, 0664, &rawPRFile);
> +  if (NS_FAILED(rv)) {

`|| !rawPRFile`?

::: security/manager/ssl/nsPKCS12Blob.cpp:275
(Diff revision 1)
> -//  private members
> -//
> -///////////////////////////////////////////////////////////////////////
> -
> -// unicodeToItem
> -//
> +UniquePtr<uint8_t>
> +nsPKCS12Blob::unicodeToBytes(const nsString& uni, uint32_t& bytesLength)
> +{
> +  uint32_t wideLength = uni.Length() + 1; // +1 for the null terminator.
> +  auto buffer = MakeUnique<uint8_t>(wideLength * 2);
> +  bytesLength = wideLength * 2;

Moving this one up would safe you computing `wideLength*2` twice.

::: security/manager/ssl/nsPKCS12Blob.cpp:325
(Diff revision 1)
> -                       NS_CERTIFICATEDIALOGS_CONTRACTID);
> +                                NS_CERTIFICATEDIALOGS_CONTRACTID);
> -  if (NS_FAILED(rv))
> +  if (NS_FAILED(rv)) {
>      return rv;
> +  }
> +  nsAutoString password;
>    bool pressedOK;

I'd probably initialise this to false just to be safe.

::: security/manager/ssl/nsPKCS12Blob.cpp:371
(Diff revision 1)
> +  }
>    return NS_OK;
>  }
>  
> -// nickname_collision
>  // what to do when the nickname collides with one already in the db.

What

::: security/manager/ssl/nsPKCS12Blob.cpp:425
(Diff revision 1)
> -  if (!newNick)
> +                                          nickname.Length() + 1));
> +  if (!newNick) {
>      return nullptr;
> +  }
> +  memcpy(newNick->data, nickname.get(), nickname.Length());
> +  newNick->data[nickname.Length()] = 0;

`SECITEM_AllocItem` uses `PORT_ZAlloc` so this shouldn't be necessary. But I'm fine with keeping it if you want to protect against changes in `SECITEM_AllocItem`.

::: security/manager/ssl/nsPKCS12Blob.cpp:434
(Diff revision 1)
> -// write_export_file
>  // write bytes to the exported PKCS#12 file
>  void
> -nsPKCS12Blob::write_export_file(void* arg, const char* buf, unsigned long len)
> +nsPKCS12Blob::writeExportFile(void* arg, const char* buf, unsigned long len)
>  {
> -  nsPKCS12Blob* cx = (nsPKCS12Blob*)arg;
> +  PRFileDesc* file = static_cast<PRFileDesc*>(arg);

Maybe assert that `file` isn't null.

Comment 10

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

https://reviewboard.mozilla.org/r/243548/#review249498

Thanks!

> This always makes me sad :(

Yeah, no kidding :(

> Moving this one up would safe you computing `wideLength*2` twice.

Good call.

> `SECITEM_AllocItem` uses `PORT_ZAlloc` so this shouldn't be necessary. But I'm fine with keeping it if you want to protect against changes in `SECITEM_AllocItem`.

It looks like it calls PORT_ZAlloc to allocate the SECItem itself if necessary, but the data gets allocated with PORT_Alloc here, so I think we do need it.

Comment 11

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975198 [details]
bug 401240 - part 1/2 - run ./mach clang-format on nsPKCS12Blob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243546/diff/1-2/

Comment 12

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243548/diff/1-2/

Comment 13

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

https://reviewboard.mozilla.org/r/243548/#review250198

Thanks! Looks good.

::: security/manager/ssl/tests/unit/test_certDB_export_pkcs12_with_master_password.js:6
(Diff revision 2)
>  // -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
>  // Any copyright is dedicated to the Public Domain.
>  // http://creativecommons.org/publicdomain/zero/1.0/
>  "use strict";
>  
>  // Tests that a CA certificate can still be imported if the user has a master

Update comment.

::: security/manager/ssl/tests/unit/test_certDB_export_pkcs12_with_master_password.js:64
(Diff revision 2)
> -    }
>      equal(text,
>            "Please enter your master password.",
>            "password prompt text should be as expected");
>      equal(checkMsg, null, "checkMsg should be null");
> -    ok(this.passwordToTry, "passwordToTry should be non-null");
> +    password.value = this.password;

Given that most people don't have a master password wouldn't it be nice to have an export test without master password as well?

Comment 14

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

https://reviewboard.mozilla.org/r/243548/#review250198

Thanks!

> Update comment.

Heh - good catch.

> Given that most people don't have a master password wouldn't it be nice to have an export test without master password as well?

Sure - sounds good.

Comment 15

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975198 [details]
bug 401240 - part 1/2 - run ./mach clang-format on nsPKCS12Blob

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243546/diff/2-3/

Comment 16

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8975199 [details]
bug 401240 - part 2/2 - reimplement PKCS#12 import/export without goto

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243548/diff/2-3/

Comment 17

[Dana Keeler (she/her) [:keeler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ede7c80ee3881156bb76505c9aa66e369215beaf

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a2224a146ec
part 1/2 - run ./mach clang-format on nsPKCS12Blob r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/da8d4cad05b3
part 2/2 - reimplement PKCS#12 import/export without goto r=fkiefer

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7a2224a146ec
https://hg.mozilla.org/mozilla-central/rev/da8d4cad05b3