330897 - crypto.signText writing off end-of-array, leading to SEGV, with patch

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Mikolaj J. Habryn]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/Debian-1.8.0.1-5 Galeon/2.0.1 (Debian package 2.0.1-3)
Build Identifier: 

There is an array indexing bug in the implementation of crypto.signText. The wrong index is used (starting from 2 instead of 0) when copying varargs caNames parameters from the Javascript call into an argument array.

Since this is overwriting random memory with a pointer to user-supplied data, there's a remote possibility that it's somehow exploitable. Mitigating it is that only the first two pointers after the array end are overwritten, the browser is guaranteed to crash inside the CERT_FilterListByCANames call on the next code line, and the code is only called for browsers that have client certificates loaded. I'll flag it as 'security' just in case.

Two-character fix is:

dichro@amida:~/.c$ diff -u mozilla/security/manager/ssl/src/nsCrypto.cpp{.orig,}
--- mozilla/security/manager/ssl/src/nsCrypto.cpp.orig  2006-03-18 12:14:57.000000000 +1100
+++ mozilla/security/manager/ssl/src/nsCrypto.cpp       2006-03-18 12:15:59.000000000 +1100
@@ -2167,7 +2167,7 @@

         return NS_OK;
       }
-      caNames[i] = JS_GetStringBytes(caName);
+      caNames[i - 2] = JS_GetStringBytes(caName);
     }

     if (certList &&
dichro@amida:~/.c$


Reproducible: Always

Steps to Reproduce:
1. Ensure there is at least one client certificate loaded in browser (firefox release, firefox HEAD, and mozilla known to crash)
2. Visit any URL with a crypto.signText call containing optional caNames arguments (eg, http://mikolaj.tv/crashdemo.html)


Actual Results:  
PORT_Strcmp (presumably) SEGVs, killing the browser.

(gdb) bt
#0  0xb7f84508 in raise () from /lib/tls/libpthread.so.0
#1  0xb7f64eca in nsProfileLock::FatalSignalHandler () from /usr/local/lib/firefox-1.6a1/libxul.so
#2  <signal handler called>
#3  0xb75e5bf0 in strcmp () from /lib/tls/libc.so.6
#4  0xb324dbde in CERT_FilterCertListByCANames () from /usr/local/lib/firefox-1.6a1/libnss3.so
#5  0xb331d0a0 in nsCrypto::SignText () from /usr/local/lib/firefox-1.6a1/components/libpipnss.so


Expected Results:  
Not a crash! In the case of the example URL, the page is expected to display:

This is an HTML page

error:noMatchingCert

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's patch

The provided patch looks correct to me.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Although I'd probably have written the code differently to start with, using a loop over pointers rather than indices.)

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 215465 [details] [diff] [review]
reporter's patch

Thanks a lot for reporting this bug and providing a fix.

Sigh, I looked at bug 29152, where we added this code. The initial revision of my patch did not have this bug, but in future revisions I failed to see this error when I reviewed.

r=kengert

Feel free to check this patch in, or I can do it Monday.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk and MOZILLA_1_8_BRANCH.

Comment 5

[Darin Fisher]

Comment on attachment 215465 [details] [diff] [review]
reporter's patch

approved for 1.8.0 branch, a=darin for drivers

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_0_BRANCH.

Comment 7

[Jay Patel [:jay]]

v.fixed based on comments, discussion with Darin during triage meeting, and code inspection.

Comment 8

[Alexander Sack]

Chris, can you please check-in for aviary?