276183 - PK11_HashBuf buffer overflow

Status: RESOLVED INVALID

(NSS :: Libraries, defect)

Description

[Jason Reid]

/* digest is too small to handle the value */                                  
digest = (unsigned char *)malloc((MD2_LENGTH + 1)*sizeof(unsigned char));      
                                                                               
secStatus = PK11_HashBuf(SEC_OID_SHA512, digest, buffer, strlen(buffer));      
                                                                               
if (SECSuccess ==  secStatus ) {                                               
        fprintf(stderr,                                                 "ERROR:
PK11_HashBuf succeeded for a SHA512 into a MD2 digest buffer\n");              
} else {                                                                       
        fprintf(stderr,                                                
"PK11_HashBuf failed for a SHA512 into a MD2 digest buffer. (expected)\n");    
        }                                                                      
        free(digest);

The above code causes a segementation fault when PK11_HashBuf overflows the
digest buffer due to the requested hash/digest operation result being too large.
PK11_HashBuf should instead return SECFailure.

> 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 154
Breakpoint 1 at 0x8048c00: file hash-tests.c, line 154.
(gdb) next
The program is not being run.
(gdb) run
Starting program: /home/jmr/work/nss/tests/Hashing/hash-tests
[New Thread 16384 (LWP 3878)]
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 3878)]

Breakpoint 1, main (argc=1, argv=0xbffff1e4) at hash-tests.c:154
154             digest = (unsigned char *)malloc((MD2_LENGTH +
1)*sizeof(unsigned char));
(gdb) next
155             secStatus = PK11_HashBuf(SEC_OID_SHA512, digest, buffer,
strlen(buffer));
(gdb) next

Program received signal SIGSEGV, Segmentation fault.
0x402732a7 in pk11_SessionFromHandle (handle=9) at pkcs11u.c:2859
2859        pk11queue_find(session,handle,slot->head,slot->sessHashSize);
(gdb) where
#0  0x402732a7 in pk11_SessionFromHandle (handle=9) at pkcs11u.c:2859
#1  0x40261744 in NSC_CloseSession (hSession=9) at pkcs11.c:3435
#2  0x400517fb in pk11_CloseSession (slot=0x8050610, session=9, owner=1)
    at pk11obj.c:373
#3  0x4004270b in PK11_DestroyContext (context=0x8058c08, freeit=1)
    at pk11cxt.c:94
#4  0x4004369c in PK11_HashBuf (hashAlg=193,
    out=0x8058c40
"\021:;Ç\203ØQü\003s!K\031ê{éú=åAì¹þ\002mRÆ\003èê\031ÁtÌ\016\227\004ø¹\r1\"\022ÀæØE=ß³ã\024\024\tÏKíÈï\0035\220´\232â\005@\020\006\005\b\220û\004\b",
in=0xbffff170 "deadbeef", len=8) at pk11cxt.c:649
#5  0x08048c2f in main (argc=1, argv=0xbffff1e4) at hash-tests.c:155
(gdb)

Comment 1

[Wan-Teh Chang]

Since PK11_HashBuf doesn't have an argument for
output buffer size, it has to require that the
caller provide an output buffer large enough to
hold the output.  (PK11_DigestFinal is the same
way; there is no way to tell it how big the 'data'
buffer is.)  All the hash algorithms have a
known output length, so it is possible to allocate
an output buffer of the right size.

There is nothing we can do about PK11_HashBuf
other than documenting this responsibility of
the caller.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Right.  It is the caller's responsibility to provide a big enough buffer.

Comment 3

[Wan-Teh Chang]

I added a comment to our public header file to document this:

/*
 * The output buffer 'out' must be big enough to hold the output of
 * the hash algorithm 'hashAlg'.
 */
SECStatus PK11_HashBuf(SECOidTag hashAlg, unsigned char *out, unsigned char *in,
                                        int32 len);

Comment 4

[Wan-Teh Chang]

It seems that what I said about PK11_DigestFinal
in comment 1 is wrong:

    PK11_DigestFinal is the same way; there is no
    way to tell it how big the 'data' buffer is.

PK11_DigestFinal has this prototype:

    SECStatus PK11_DigestFinal(PK11Context *context, unsigned char *data, 
                                    unsigned int *outLen, unsigned int length);

If I understand the code correctly, the 'length'
input argument specifies the length of the 'data'
buffer, and PK11_DigestFinal stores the length of
the actual output in *outLen.