Bug 1174527

Summary: Segfault in pk12util when using -l option with certain .p12 files
Product: Red Hat Enterprise Linux 7 Reporter: Elio Maldonado Batiz <emaldona>
Component: nssAssignee: Elio Maldonado Batiz <emaldona>
Status: CLOSED ERRATA QA Contact: Hubert Kario <hkario>
Severity: high Docs Contact:
Priority: unspecified    
Version: 7.2CC: hkario, ksrot, ovasik, pkis, rrelyea
Target Milestone: rc   
Target Release: ---   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: nss-3.16.2.3-5.el7 Doc Type: Bug Fix
Doc Text:
Cause: The pkcs #12 decoder didn't properly check the destination buffer length when decoding. Consequence: A segmentation fault resulted in the pk12util tool when using -l option to list the contents of certain pkcs 12 encoded files Fix: The decoder has been fixed to perform the needed check. Result: The pk12util can now list the encoded files.
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-03-05 08:29: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: 1180596, 1182933    
Attachments:
Description Flags
simplified version of Hubert's test
none
Gets rid of the segfault on x86_64
none
Gets rid of the segfault on x86_64
none
Fix the segfault - corrected version
rrelyea: review+
all changes including nss.spec changes none

Description Elio Maldonado Batiz 2014-12-16 01:37:50 UTC
Description of problem: Originally reprted by Hubert Kario while testing teh fix for Bug 1150645 - 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

Version-Release number of selected component (if applicable):


How reproducible: on x86_64 system


Steps to Reproduce:
You need the test data from http://pkgs.devel.redhat.com/cgit/tests/nss-softokn/tree/Regression/bz1150645-Importing-an-RSA-private-key-fails-if-p-q

1. In a convinient directory execute rhpkg tests nss-softokn to get them
This will bring the test files for nss-softokn
2. cd nss-softokn/Regression/bz1150645-Importing-an-RSA-private-key-fails-if-p-q
3. If you have the beaker test supprt exceute make run
If not execute pk12util -l Importable.p12 -w ImportablePwd.txt


Actual results:
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) {

Expected results: No segfault and subsewuently the .p12 filescan be imprted

Additional info:
Run pk12util -l Importable.p12 -w ImportablePwd.txt in the debugger
When the sefault occurs get a backtrace
(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) 

nss-softokn is doesn't appear on the stacktrace. The problem is not with pk12util tool but rather withe the nss dedoding code for p12.

I traced execution on the gdb for x86_64 (where it fails) and on i686 (where it doesn't) and on both the code patch followed is the same.

gdb shows me that dest-len = 2, but dest-data is "" (on both). That causes the segfault.

Comment 1 Elio Maldonado Batiz 2014-12-16 01:40:04 UTC
Created attachment 969386 [details]
simplified version of Hubert's test

It can be used in system without Beaker and works wiithout using valgrind.

Comment 2 Elio Maldonado Batiz 2014-12-16 01:44:10 UTC
The results of my additiobal testing are as follows. 

I tested using Hubert's test with Beaker and my simplified version that doesn't require Beaker. 

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

The stack trace is the one in Comment 0

I traced execution on the 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 Batiz 2014-12-16 01:47:34 UTC
Created attachment 969387 [details]
Gets rid of the segfault on x86_64

Comment 5 Elio Maldonado Batiz 2014-12-16 03:40:38 UTC
Created attachment 969405 [details]
Gets rid of the segfault on x86_64

identical to the one submitted upstream

Comment 6 Bob Relyea 2014-12-16 21:32:34 UTC
If the crash is because data->len-2 is negative, the better test is

if ((data->len < 2) && ..... )

Rather than
if ((strlen(data->data) <=2) && ... )

bob

NOTE: if you want this in 7.1, you'd need to ask and justify it as a blocker.

Comment 8 Elio Maldonado Batiz 2015-01-07 23:42:42 UTC
(In reply to Bob Relyea from comment #6)
> If the crash is because data->len-2 is negative, the better test is
> 
> if ((data->len < 2) && ..... )

I think you meant to type
> if ((dest->len < 2) && ..... )

Comment 10 Elio Maldonado Batiz 2015-01-18 17:19:04 UTC
and see my comment at: https://bugzilla.redhat.com/show_bug.cgi?id=1181614#c7

Comment 11 Elio Maldonado Batiz 2015-01-18 17:54:20 UTC
Created attachment 981212 [details]
Fix the segfault - corrected version

without breaking the python-nss sanity check as previous one did as explained in https://bugzilla.redhat.com/show_bug.cgi?id=1181614#c7

Comment 12 Elio Maldonado Batiz 2015-01-19 19:16:17 UTC
Created attachment 981584 [details]
all changes including nss.spec changes

Comment 13 Bob Relyea 2015-01-19 19:34:33 UTC
Comment on attachment 981212 [details]
Fix the segfault - corrected version

r+ rrelyea

Comment 14 Suzanne Forsberg 2015-01-19 21:30:24 UTC
*** Bug 1181614 has been marked as a duplicate of this bug. ***

Comment 18 errata-xmlrpc 2015-03-05 08:29:23 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-0364.html