276180 - PK11_HashBuf will crash if given a NULL buffer or a negative length

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jason Reid]

/* handling a NULL buffer */
        digest = (unsigned char *)malloc((MD5_LENGTH + 1)*sizeof(unsigned char));
        secStatus = PK11_HashBuf(SEC_OID_MD5, digest, NULL, strlen(buffer));
        if (SECSuccess ==  secStatus ) {
                fprintf(stderr,
"ERROR: PK11_HashBuf succeeded for a NULL buffer\n");
        } else {
                fprintf(stderr,
"PK11_HashBuf failed for a NULL buffer. (expected)\n");
        }
        free(digest);


The above code causes a segmentation violation in PK11_HashBuf. PK11_HashBuf
should return SECFailure not crash if given bad input.

> gdb ./hash-tests
GNU gdb 5.3.92
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i586-suse-linux"...
(gdb) run
Starting program: /home/jmr/work/nss/tests/Hashing/hash-tests
[New Thread 16384 (LWP 3100)]
0xdeadbeef hash tests
MD2= 17682ba795d60a2e740ba5f4539007a4
MD5= 4f41243847da693a4f356c0486114bc6
SHA1= f49cf6381e322b147053b74e4500af8533ac1e4c
SHA256= 2baf1f40105d9501fe319a8ec463fdf4325a2a5df445adf3f572f626253678c9
SHA384=
906d9fe92a5e0ad7e020842127f7970b7d2adff3a91106927b592cf157633d86d0be6ab78f5d398e53f705643ccbfb78
SHA512=
113a3bc783d851fc0373214b19ea7be9fa3de541ecb9fe026d52c603e8ea19c174cc0e9705f8b90d312212c0c3a6d8453ddfb3e3141409cf4bedc8ef033590b4

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 3100)]
0x401441bc in memcpy () from /lib/i686/libc.so.6
(gdb) where
#0  0x401441bc in memcpy () from /lib/i686/libc.so.6
#1  0x4027ae6a in MD5_Update (cx=0x8057bb0, input=0x0, inputLen=8) at md5.c:500
#2  0x402663d7 in NSC_DigestUpdate (hSession=8, pPart=0x0, ulPartLen=8)
    at pkcs11c.c:1179
#3  0x40043b0e in PK11_DigestOp (context=0x8057978, in=0x0, inLen=8)
    at pk11cxt.c:801
#4  0x40043611 in PK11_HashBuf (hashAlg=SEC_OID_MD5,
    out=0x80579b0 "¬\232\037@¬\232\037@\200ó\004\b\006", in=0x0, len=8)
    at pk11cxt.c:636
#5  0x08048bb8 in main (argc=1, argv=0xbffff1e4) at hash-tests.c:129

Comment 1

[Jason Reid]

Giving a negative buffer length will also cause a crash, SECFailure should be
returned.
secStatus = PK11_HashBuf(SEC_OID_MD5, digest, buffer, -1);
> gdb ./hash-tests
GNU gdb 5.3.92
Copyright 2003 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i586-suse-linux"...
(gdb) break 169
Breakpoint 1 at 0x8048c00: file hash-tests.c, line 169.
(gdb) run
Starting program: /home/jmr/work/nss/tests/Hashing/hash-tests
[New Thread 16384 (LWP 3995)]
0xdeadbeef hash tests
MD2= 17682ba795d60a2e740ba5f4539007a4
MD5= 4f41243847da693a4f356c0486114bc6
SHA1= f49cf6381e322b147053b74e4500af8533ac1e4c
SHA256= 2baf1f40105d9501fe319a8ec463fdf4325a2a5df445adf3f572f626253678c9
SHA384=
906d9fe92a5e0ad7e020842127f7970b7d2adff3a91106927b592cf157633d86d0be6ab78f5d398e53f705643ccbfb78
SHA512=
113a3bc783d851fc0373214b19ea7be9fa3de541ecb9fe026d52c603e8ea19c174cc0e9705f8b90d312212c0c3a6d8453ddfb3e3141409cf4bedc8ef033590b4
ERROR: PK11_HashBuf succeeded for a NULL digest
[Switching to Thread 16384 (LWP 3995)]

Breakpoint 1, main (argc=1, argv=0xbffff1e4) at hash-tests.c:169
169             digest = (unsigned char *)malloc((MD5_LENGTH +
1)*sizeof(unsigned char));
(gdb) next
170             secStatus = PK11_HashBuf(SEC_OID_MD5, digest, buffer, -1);
(gdb) next

Program received signal SIGSEGV, Segmentation fault.
0x4027a1b6 in md5_compress (cx=0x8058c98, wBuf=0xbffffff0) at md5.c:382
382             FF(a, b, c, d, wBuf[R1B4 ], S1_0, T1_4);
(gdb) where
#0  0x4027a1b6 in md5_compress (cx=0x8058c98, wBuf=0xbffffff0) at md5.c:382
#1  0x4027ae3e in MD5_Update (cx=0x8058c98, input=0xbffffff0 "/hash-tests",
    inputLen=4294963583) at md5.c:493
#2  0x402663d7 in NSC_DigestUpdate (hSession=9, pPart=0xbffff170 "deadbeef",
    ulPartLen=4294967295) at pkcs11c.c:1179
#3  0x40043b0e in PK11_DigestOp (context=0x8058ac8, in=0xbffff170 "deadbeef",
    inLen=4294967295) at pk11cxt.c:801
#4  0x40043611 in PK11_HashBuf (hashAlg=SEC_OID_MD5, out=0x8058b00 "",
    in=0xbffff170 "deadbeef", len=-1) at pk11cxt.c:636
#5  0x08048c1f in main (argc=1, argv=0xbffff1e4) at hash-tests.c:170
(gdb) quit
The program is running.  Exit anyway? (y or n) y

Comment 2

[Wan-Teh Chang]

It is the application program's responsibility
to ensure that the arguments they pass to NSS
functions are valid.  (The Standard C library
takes this stance, which is why memcpy crashes
on a null buffer input.)  So the severity of this
bug should not be critical.

However, NSS functions should also check for
invalid arguments.  This probably should be
done at all levels, from PK11_HashBuf down to
MD5_Update, but I will attach a patch that
just adds the defense at the top-level PK11
functions.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

Jason, please test this patch.

I defend against a null 'in' buffer in PK11_DigestOp,
which is called by PK11_HashBuf.

I can only check for a negative buffer size in PK11_HashBuf
because the buffer size for PK11_DigestOp is unsigned.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 170198 [details] [diff] [review]
Proposed patch

r=nelson

Comment 5

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (NSS 3.10).

Comment 6

[Robert Relyea]

Comment on attachment 170198 [details] [diff] [review]
Proposed patch

This patch is fine,
though I disagree with the assertion that NSS should not crash on bad input...
It's very difficult to verify bogus pointers, and NSS does not work on handles.

That was something we thought about as a Stan feature, and there was some
pointer tracker code in Stan, but it became pretty unweildy when you through in
arenas, so we have effective abandoned that idea.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Setting priorities on unprioritized bugs resolved fixed for NSS 3.10.
P1 since it's a crash fix.