276311 - public functions in pk11cxt.c don't check for NULL arguments

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Jason Reid]

secStatus = PK11_DigestFinal(NULL, digest, &counter, (MD2_LENGTH + 1));

The above line causes a segmentation violation.

> 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 3159)]
deadbeef hash tests with PK11_HashBuf
MD2= 17682ba795d60a2e740ba5f4539007a4
MD5= 4f41243847da693a4f356c0486114bc6
SHA1= f49cf6381e322b147053b74e4500af8533ac1e4c
SHA256= 2baf1f40105d9501fe319a8ec463fdf4325a2a5df445adf3f572f626253678c9
SHA384=
906d9fe92a5e0ad7e020842127f7970b7d2adff3a91106927b592cf157633d86d0be6ab78f5d398e53f705643ccbfb78
SHA512=
113a3bc783d851fc0373214b19ea7be9fa3de541ecb9fe026d52c603e8ea19c174cc0e9705f8b90d312212c0c3a6d8453ddfb3e3141409cf4bedc8ef033590b4
ERROR: PK11_HashBuf succeeded for a NULL digest
PK11_HashBuf failed for an invalide SEC_OID. (expected)
deadbeef hash tests with PK11_CreateDigestContext
MD2= 17682ba795d60a2e740ba5f4539007a4
PK11_CreateDigestContext failed to return a valid context with -1. (expected)
ERROR: PK11_DigestFinal returned success for a digest without PK11_DigestOp ever
being called.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 3159)]
0x40042652 in PK11_EnterContextMonitor (cx=0x0) at pk11cxt.c:68
68          if ((cx->ownSession) && (cx->slot->isThreadSafe)) {
(gdb) where
#0  0x40042652 in PK11_EnterContextMonitor (cx=0x0) at pk11cxt.c:68
#1  0x40044090 in PK11_DigestFinal (context=0x0, data=0x8059170 "",
    outLen=0xbffff16c, length=17) at pk11cxt.c:1003
#2  0x08049090 in main (argc=1, argv=0xbffff1e4) at hash-tests.c:282
(gdb) quit
The program is running.  Exit anyway? (y or n) y

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Neil, please add the missing input parameter validation checks to all
the public PK11 funcs in the same source file as PK11_DigestFinal.
Suggest you use the code in PK11_DigestKey as the example.  Thanks.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm combining all the bugs about PK11_Digest* functions not checking null
pointers into this one bug.  One patch will cure them all.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 276312 has been marked as a duplicate of this bug. ***

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 276313 has been marked as a duplicate of this bug. ***

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 276314 has been marked as a duplicate of this bug. ***

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 276181 has been marked as a duplicate of this bug. ***

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I'm broadening the description of this bug to include all the public
functions in pk11cxt.c.
Neil, when these are fixed, please set the "target Milestone" to 3.10
in all the bugs that are dups of this one.  Thanks.

Comment 8

[Wan-Teh Chang]

I suggest that we only do null pointer checking
where it makes the most bang for the buck.

For example, this sequence is common (code checking
secStatus for failure is omitted for brevity):

        digestContext = PK11_CreateDigestContext(...);
        secStatus = PK11_DigestBegin(digestContext);
        secStatus = PK11_DigestOp(digestContext, ...);
        secStatus = PK11_DigestFinal(digestContext, ...);
        PK11_DestroyContext(digestContext, PR_TRUE);

I think that we derive the most benefit from checking
digestContext for null in PK11_DigestBegin.  Checking
digestContext for null in PK11_DigestOp, PK11_DigestFinal,
and PK11_DestroyContext is redundant in the above code
sequence.

Null pointer checking increases code size and takes
time.  It is not free.  We should do it to help detect
programming errors in real programs, not in contrived
test programs.

Comment 9

[Neil Williams]

Changing this back to new since we don't have agreement on the fix yet.

Comment 10

[Julien Pierre]

Really reopening the bug.

Comment 11

[Wan-Teh Chang]

We can try to fix this in NSS 3.11.  I reiterate my opinion
in comment 8 that we only need to do NULL pointer checking
in PK11_DigestBegin.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:beurdouche, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 13

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.