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

RESOLVED FIXED

Status

()

Core
Security: PSM
--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Mikolaj J. Habryn, Assigned: kaie)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
fixed1.8.1, verified1.8.0.4
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], URL)

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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
Assignee: dveditz → kengert
Component: Security → Security: PSM
Product: Mozilla Application Suite → Core
QA Contact: seamonkey
Version: unspecified → Trunk
Created attachment 215465 [details] [diff] [review]
reporter's patch

The provided patch looks correct to me.
Attachment #215465 - Flags: superreview+
Attachment #215465 - Flags: review?(kengert)
Attachment #215465 - Flags: approval1.8.0.3?
Attachment #215465 - Flags: approval-branch-1.8.1?(kengert)
(Although I'd probably have written the code differently to start with, using a loop over pointers rather than indices.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

12 years ago
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.
Attachment #215465 - Flags: review?(kengert)
Attachment #215465 - Flags: review+
Attachment #215465 - Flags: approval-branch-1.8.1?(kengert)
Attachment #215465 - Flags: approval-branch-1.8.1+
Checked in to trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Keywords: fixed1.8 → fixed1.8.1
Flags: blocking1.8.0.3+
Whiteboard: [sg:critical?]

Comment 5

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

approved for 1.8.0 branch, a=darin for drivers
Attachment #215465 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fix checked in to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.3

Comment 7

11 years ago
v.fixed based on comments, discussion with Darin during triage meeting, and code inspection.
Keywords: fixed1.8.0.4 → verified1.8.0.4
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Group: security

Comment 8

11 years ago
Chris, can you please check-in for aviary?
You need to log in before you can comment on or make changes to this bug.