341455 - Crash in pk12util on Windows; pk12util and certutil test failures on other platforms

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Julien Pierre]

The Windows tinderbox started showing crashes in pk12util today. This is a regression, probably from one of the coverity fixes.

I did a local build and ran sh -X tools.sh  . I got the following stack :

MSVCRT! 78001a66()
SECITEM_ZfreeItem(SECItemStr * 0x0012fe84, int 0) line 275 + 19 bytes
sec_Pkcs12FinishMac(sec_PKCS12EncoderContextStr * 0x00e87dd8) line 2052 + 11 bytes
SEC_PKCS12Encode(SEC_PKCS12ExportContextStr * 0x00e7d750, void (void *, const char *, unsigned long)* 0x00401d75 p12u_WriteToExportFile(void *, const char *, unsigned long), void * 0x00e76e50) line 2188 + 9 bytes
P12U_ExportPKCS12Object(char * 0x00484221, char * 0x00484270, PK11SlotInfoStr * 0x00e6a4d8, secuPWData * 0x0012ff30, secuPWData * 0x0012ff44) line 687 + 18 bytes
main(int 11, char * * 0x004841d8) line 957 + 28 bytes
PK12UTIL! mainCRTStartup + 227 bytes
KERNEL32! 7c598989()

The line in question is 
	SECITEM_ZfreeItem(&hmac, PR_FALSE);

The hmac SECItem has a len field of 1244748, so I think it's bogus.

Comment 1

[Julien Pierre]

This is a regression caused by the fix for bug 337110 .

Comment 2

[Julien Pierre]

New blocker regression for 3.11.2 .

Comment 3

[Julien Pierre]

Attachment: Fix regression

This fixes the regression from attachment 225313 [details] [diff] [review] in bug 337110 .
The problem was that 
PORT_Memcpy does not copy pbe_params->pPassword or pbe_params->pSalt .
These actually get destroyed and zero'ed.

The patch restores the old behavior and the 2 different exit paths.
Note that this produces a crash on Windows because the free() call unzeroes (for lack of a better term) the memory.
On other platforms, the memory stays zero, and many tests fail.
But our tinderboxes apparently still all think it's green !!!!!!!!!!!!!

Comment 4

[Julien Pierre]

I have checked in the fix on the NSS_3_11_BRANCH so that this regression will not stop us from producing 3.11.2.

Checking in pk11pbe.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.11.24.3; previous revision: 1.11.24.2
done

I will hold off the checkin to the tip until review, and until our tinderbox environment is no longer lying and reporting green when the tests fail.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I backed out Julien's checkin for this bug on the branch.
I also backed out the checkin for bug 337110 on the branch, which fixes
this bug (bug 341455), so I will resolve this bug as fixed, and reopen 
bug 337110.

This all needs to get fixed on the trunk, too, but Julien wants to leave
the trunk broken until Tinderbox properly shows it as broken.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 225521 [details] [diff] [review]
Fix regression

This patch leaks the buffer allocated by SECITEM_AllocItem.

Comment 7

[Alexei Volkov]

Comment on attachment 225521 [details] [diff] [review]
Fix regression

fix is in attachment 225525 [details] [diff] [review]

Comment 8

[Slavomir Katuscak]

In last (20060614.1) nightly tests on securitytip is pk12util crashing on all architectures - it causes failures of 6 tests:

Importing Alice's email cert & key (pk12util -i)
Listing Alice's pk12 file (pk12util -l)
Importing Alice's email EC cert & key (pk12util -i)
Listing Alice's pk12 EC file (pk12util -l)
Import the certificate and key from the PKCS#12 file (pk12util -i)
List the FIPS module keys (certutil -K)

tools.sh: Tools Tests with ECC ===============================
tools.sh: Exporting Alice's email cert & key------------------
pk12util -o Alice.p12 -n "Alice" -d ../alicedir -k ../tests.pw.922 \
         -w ../tests.pw.922
pk12util: PKCS12 EXPORT SUCCESSFUL
tools.sh: Importing Alice's email cert & key -----------------
pk12util -i Alice.p12 -d ../tools/copydir -k ../tests.pw.922 -w ../tests.pw.922
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
tools.sh: Listing Alice's pk12 file -----------------
pk12util -l Alice.p12 -w ../tests.pw.922
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
tools.sh: Exporting Alice's email EC cert & key---------------
pk12util -o Alice-ec.p12 -n "Alice-ec" -d ../alicedir -k ../tests.pw.922 \
         -w ../tests.pw.922
pk12util: PKCS12 EXPORT SUCCESSFUL
tools.sh: Importing Alice's email EC cert & key --------------
pk12util -i Alice-ec.p12 -d ../tools/copydir -k ../tests.pw.922 -w ../tests.pw.922
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
tools.sh: Listing Alice's pk12 EC file -----------------
pk12util -l Alice-ec.p12 -w ../tests.pw.922
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
tools.sh: Create objsign cert -------------------------------
signtool -G "objectsigner" -d ../alicedir -p "nss"
using certificate directory: ../alicedir

pk12util -d ../fips -i fips140.p12 -w ../tests.fipsp12pw.922 -k ../tests.fipspw.922
pk12util: PKCS12 decode not verified: Unable to import.  Invalid MAC.  Incorrect password or corrupt file.
fips.sh: List the FIPS module certificates -----------------
certutil -d ../fips -L
fips.sh: List the FIPS module keys --------------------------
certutil -d ../fips -K -f ../tests.fipspw.922
certutil: no keys found

Comment 9

[Slavomir Katuscak]

On nightly testing: securitytip 20060614.1 nssamdrhel3.1
also failed tests:

Exporting Alice's email cert & key (pk12util -o)
Exporting Alice's email EC cert & key (pk12util -o)

Core should be accessible.

Comment 10

[Slavomir Katuscak]

On nightly testing: securitytip 20060614.1 nssamdrhel3.2
also failed tests:

Export the certificate and key as a PKCS#12 file (pk12util -o)

Core should be accessible.