494087 - Passing NULL as the value of cert_pi_trustAnchors causes a crash in cert_pkixSetParam

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

According to documentation of cert_pi_trustAnchors, we can
just pass NULL to use the default trusted roots:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certt.h&rev=1.50&mark=943-944#942

When the Chromium browser tried that on Ubuntu 8.04 (which uses
NSS 3.12.0.3), it resulted in a crash by SIGSEGV in cert_pkixSetParam
in mozilla/security/nss/lib/certhigh/certvfypkix.c because it
doesn't handle the NULL case as advertised.

I verified that the cert_pi_trustAnchors case in cert_pkixSetParam
hasn't changed since NSS_3_12_RTM, so you should be able to
reproduce this crash with the current NSS trunk.  I guess
it crashes here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.42&mark=1698#1690

because CERT_LIST_HEAD deferences its argument:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certt.h&rev=1.50&mark=378#378

Comment 1

[Wan-Teh Chang]

Alexei, will we get the desired behavior if we pass a pointer
to an empty CERTCertList as the value of cert_pi_trustAnchors?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,
Please confirm that this behavior is still present in 3.12.3 or 3.12.4 RC0.
It may have been fixed already.

Comment 3

[Wan-Teh Chang]

By code inspection, this bug should be still present in the current
trunk because the code in question hasn't changed since NSS_3_12_RTM:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.42&mark=1691-1723#1690

Comment 4

[Wan-Teh Chang]

I verified by patching vfychain.c that this bug is still present
in the current trunk:

wtc@fips:~/nss-tip/mozilla/security/nss/cmd/vfychain/Linux2.6_x86_glibc_PTH_DBG
.OBJ$ gdb ./vfychain
GNU gdb 6.8-gg14
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux"...
FAQ: http://go/gdb, Wiki: http://wiki/Main/GnuDebugger, Email: gdb-team
(gdb) run -u 3 -p -p -a verisign.pem
Starting program: /home/wtc/nss-tip/mozilla/security/nss/cmd/vfychain/Linux2.6_x
86_glibc_PTH_DBG.OBJ/vfychain -u 3 -p -p -a verisign.pem
warning: Lowest section in system-supplied DSO at 0xffffe000 is .hash at ffffe0b
4
[Thread debugging using libthread_db enabled]
[New Thread 0xf7dbd6c0 (LWP 12195)]
Warning: guessed host libthread_db library "/usr/grte/v1/lib64/libthread_db.so.1
".
Using host libthread_db library "/usr/grte/v1/lib64/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf7dbd6c0 (LWP 12195)]
0xf7e64cda in cert_pkixSetParam (procParams=0x8075018, param=0x8060ea0,
    plContext=0x8073d50) at certvfypkix.c:1698
1698                for(node = CERT_LIST_HEAD(certList); !CERT_LIST_END(node, ce
rtList);
(gdb) where
#0  0xf7e64cda in cert_pkixSetParam (procParams=0x8075018, param=0x8060ea0,
    plContext=0x8073d50) at certvfypkix.c:1698
#1  0xf7e6518f in CERT_PKIXVerifyCert (cert=0x8076428, usages=8,
    paramsIn=0x8060ea0, paramsOut=0x8060f40, wincx=0x8060e20)
    at certvfypkix.c:2125
#2  0x0804c7b9 in main (argc=7, argv=0xffffd8a4, envp=0xffffd8c4)
    at vfychain.c:720
(gdb) print certList
$1 = (const CERTCertList *) 0x0
(gdb)

I found that I got the desired behavior (use all default trusted
roots) if I don't specify cert_pi_trustAnchors or I pass a pointer
to an empty cert list (CERT_NewCertList()).  Perhaps all we need
to do is to fix the comment in certt.h.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Setting to P1, at least until problem analysis is complete.

Comment 6

[Wan-Teh Chang]

I added some info in bug 494489 comment 4 that can help
us document cert_pi_trustAnchors better.

Comment 7

[Alexei Volkov]

(In reply to comment #1)
> Alexei, will we get the desired behavior if we pass a pointer
> to an empty CERTCertList as the value of cert_pi_trustAnchors?
You will. To make sure that libpkix uses default trusted roots you need either not to pass cert_pi_trustAnchors parameter at all or to pass an empty list.

The code definitely require to check the list argument before dereferencing the pointer. I think the correct way to fix it will be to return an error if CERTValInParam structure that passed to the function has cert_pi_trustAnchors with the chain pointing to NULL object.

Comment 8

[Alexei Volkov]

Attachment: Patch v1 - check list value before deref it(committed).

Comment 9

[Wan-Teh Chang]

Comment on attachment 380307 [details] [diff] [review]
Patch v1 - check list value before deref it(committed).

r=wtc.  This patch is not complete without updating the documentation
in certt.h:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certt.h&rev=1.50&mark=943-944#942

Please see comment 6 for the points that need to be clarified.

Comment 10

[Alexei Volkov]

thanks for the review. I have changed the comment as well.