1111901 - Segfault in pk12util when using -l option with certain .p12 files

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

As originally reported by Hubert Kario while testing the fix for Bug 1049435 - Importing an RSA private key fails if p < q

Upstream reproducer causes Segmentation fault in nss when using -l option (in normal mode).

Using:
nss-3.16.2.3-1.el7.x86_64
nss-softokn-3.16.2.3-3.el7.x86_64
nspr-4.10.6-2.el7.x86_64

# valgrind pk12util -l Importable.p12 -w ImportablePwd.txt 
==11611== Memcheck, a memory error detector
==11611== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==11611== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==11611== Command: pk12util -l Importable.p12 -w ImportablePwd.txt
==11611== 
==11611== Invalid read of size 1
==11611==    at 0x508522E: sec_pkcs12_convert_item_to_unicode (p12local.c:931)
==11611==    by 0x508AA00: sec_pkcs12_get_friendlyName (p12d.c:3038)
==11611==    by 0x508AC3F: SEC_PKCS12DecoderIterateNext (p12d.c:3121)
==11611==    by 0x4063FA: P12U_ListPKCS12File (pk12util.c:741)
==11611==    by 0x404D9C: main (pk12util.c:1088)
==11611==  Address 0x106fd5ddf is not stack'd, malloc'd or (recently) free'd
==11611==
==11611==
==11611== Process terminating with default action of signal 11 (SIGSEGV)
==11611==  Access not within mapped region at address 0x106FD5DDF
==11611==    at 0x508522E: sec_pkcs12_convert_item_to_unicode (p12local.c:931)
==11611==    by 0x508AA00: sec_pkcs12_get_friendlyName (p12d.c:3038)
==11611==    by 0x508AC3F: SEC_PKCS12DecoderIterateNext (p12d.c:3121)
==11611==    by 0x4063FA: P12U_ListPKCS12File (pk12util.c:741)
==11611==    by 0x404D9C: main (pk12util.c:1088)
==11611==  If you believe this happened as a result of a stack
==11611==  overflow in your program's main thread (unlikely but
==11611==  possible), you can try to increase the size of the
==11611==  main thread stack using the --main-stacksize= flag.
==11611==  The main thread stack size used in this run was 8388608.
==11611==
==11611== HEAP SUMMARY:
==11611==     in use at exit: 104,850 bytes in 731 blocks
==11611==   total heap usage: 1,733 allocs, 1,002 frees, 616,806 bytes allocated
==11611==
==11611== LEAK SUMMARY:
==11611==    definitely lost: 0 bytes in 0 blocks
==11611==    indirectly lost: 0 bytes in 0 blocks
==11611==      possibly lost: 46,353 bytes in 134 blocks
==11611==    still reachable: 58,497 bytes in 597 blocks
==11611==         suppressed: 0 bytes in 0 blocks
==11611== Rerun with --leak-check=full to see details of leaked memory
==11611==
==11611== For counts of detected and suppressed errors, rerun with: -v
==11611== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)
Segmentation fault

How reproducible: on x86_64 systems only, it's fine on i686

Comment 1

[Elio Maldonado]

Attachment: Reproducer based on the original test

Simplified version that doesn't require Beaker (or valgrind) so it's more suitable for use here. It needs needs adjustments for i686 versus x86_64.

Comment 2

[Elio Maldonado]

The results of my testing are as follows. 

Used the test data from attachment in https://bugzilla.mozilla.org/show_bug.cgi?id=1049435#c0
Tested using both Hubert's test with Beaker and my simplified version that doesn't require Beaker. 

1) On rhel-6 I can reproduce the segfault on x86_64, i686 is fine. 
2) On rhel-7 I can reproduce the segfault on x86_64, i686 is fine. 
3) On fedora f21 I can reproduce the segfault on x86_64, i686 is fine. 
3) Using my simpler test with oor upstream source tree it segfaults on x86_64 and fine on i686

The stack trace is

The stack trace is

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff79872a6 in sec_pkcs12_convert_item_to_unicode (arena=0x0, dest=0x62b420, src=0x6322d8, zeroTerm=1, 
    asciiConvert=0, toUnicode=0) at p12local.c:931
931	    if((dest->data[dest->len-1] || dest->data[dest->len-2]) && zeroTerm) {
(gdb) bt
#0  0x00007ffff79872a6 in sec_pkcs12_convert_item_to_unicode (arena=0x0, dest=0x62b420, src=0x6322d8, zeroTerm=1, 
    asciiConvert=0, toUnicode=0) at p12local.c:931
#1  0x00007ffff798c961 in sec_pkcs12_get_friendlyName (bag=bag@entry=0x62c8a8) at p12d.c:3038
#2  0x00007ffff798cba0 in SEC_PKCS12DecoderIterateNext (p12dcx=p12dcx@entry=0x62c570, ipp=ipp@entry=0x7fffffffdae8)
    at p12d.c:3121
#3  0x000000000040646b in P12U_ListPKCS12File (in_file=<optimized out>, slot=<optimized out>, slotPw=<optimized out>, 
    p12FilePw=<optimized out>) at pk12util.c:741
#4  0x0000000000404eb2 in main (argc=1, argv=0x9c) at pk12util.c:1088
(gdb) 

I traced execution on gdb for x86_64 (where it fails) and on i686 (where it doesn't) and on both the code path followed is the same. gdb shows me that dest-len = 2, but dest-data is "" (on both). That explains the segfault.

How come it doesn't segfault on i686?

With the following changes to nss/lib/pkcs12/p12local.c

-    if((dest->data[dest->len-1] || dest->data[dest->len-2]) && zeroTerm) {
+    if ((PORT_Strlen(dest->data) >= 2) &&
+	(dest->data[dest->len-1] || dest->data[dest->len-2]) &&
+	 zeroTerm) {

the segfault doesn't happen.

Comment 3

[Elio Maldonado]

Attachment: fixes segfault

Comment 4

[Elio Maldonado]

https://bugzilla.redhat.com/show_bug.cgi?id=1181614#7 explains why my previous version the patch was incorrect and caused the python-nss sanity test to fail.

Comment 5

[Elio Maldonado]

Attachment: fix segmentation fault

Comment 6

[Elio Maldonado]

https://hg.mozilla.org/projects/nss/rev/e07a62e9dd88