Closed
Bug 494087
Opened 12 years ago
Closed 12 years ago
Passing NULL as the value of cert_pi_trustAnchors causes a crash in cert_pkixSetParam
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.4
People
(Reporter: wtc, Assigned: alvolkov.bgs)
Details
(Whiteboard: PKIX MOZ)
Attachments
(1 file)
773 bytes,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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.
Reporter | ||
Comment 3•12 years ago
|
||
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
Reporter | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Setting to P1, at least until problem analysis is complete.
Priority: -- → P1
Target Milestone: --- → 3.12.4
Updated•12 years ago
|
Whiteboard: PKIX MOZ
Reporter | ||
Comment 6•12 years ago
|
||
I added some info in bug 494489 comment 4 that can help us document cert_pi_trustAnchors better.
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #380307 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #380307 -
Flags: review? → review?(wtc)
Reporter | ||
Updated•12 years ago
|
Attachment #380307 -
Flags: review?(wtc) → review+
Reporter | ||
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
thanks for the review. I have changed the comment as well.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Attachment #380307 -
Attachment description: Patch v1 - check list value before deref it. → Patch v1 - check list value before deref it(committed).
You need to log in
before you can comment on or make changes to this bug.
Description
•