Last Comment Bug 330897 - crypto.signText writing off end-of-array, leading to SEGV, with patch
: crypto.signText writing off end-of-array, leading to SEGV, with patch
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Kai Engert (:kaie) (on vacation)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
http://mikolaj.tv/crashdemo.html
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-17 18:06 PST by Mikolaj J. Habryn
Modified: 2006-06-13 14:59 PDT (History)
5 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reporter's patch (1.00 KB, patch)
2006-03-17 18:33 PST, David Baron :dbaron: ⌚️UTC-10
kaie: review+
dbaron: superreview+
kaie: approval‑branch‑1.8.1+
darin.moz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Mikolaj J. Habryn 2006-03-17 18:06:50 PST
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-10 2006-03-17 18:33:02 PST
Created attachment 215465 [details] [diff] [review]
reporter's patch

The provided patch looks correct to me.
Comment 2 David Baron :dbaron: ⌚️UTC-10 2006-03-17 18:33:39 PST
(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) (on vacation) 2006-03-18 07:29:24 PST
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-10 2006-03-21 23:42:08 PST
Checked in to trunk and MOZILLA_1_8_BRANCH.
Comment 5 Darin Fisher 2006-04-07 12:56:57 PDT
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-10 2006-04-07 14:03:18 PDT
Fix checked in to MOZILLA_1_8_0_BRANCH.
Comment 7 Jay Patel [:jay] 2006-05-11 13:31:11 PDT
v.fixed based on comments, discussion with Darin during triage meeting, and code inspection.
Comment 8 Alexander Sack 2006-06-13 14:59:34 PDT
Chris, can you please check-in for aviary?

Note You need to log in before you can comment on or make changes to this bug.