Error in string handling within <keygen>

RESOLVED FIXED

Status

()

Core
Security: PSM
--
critical
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Arkadiusz Goralski, Assigned: Philip K. Warren)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

763 bytes, patch
Philip K. Warren
: review+
Philip K. Warren
: superreview+
Chase Phillips
: approval-aviary1.0.6-
Chase Phillips
: approval1.7.10-
Benjamin Smedberg
: approval1.8b4+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl-PL; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl-PL; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

When you put <keygen> tag into the form then it's transformed into a
simple <SELECT> tag with two <OPTIONS> - 2048 (High grade) and 1024
(Medium grade). But when looking in DOM inspector i've noticed that it
is possible to manipulate these objects (options, to be precise)
trough JavaScript.

After manipulation, KeygenHandler (source:
security/manager/ssl/src/nsKeygenHandler.cpp) is getting a different
string (other size) than it's expecting, causing a crash. Since
KeygenHandler is depending on string values rather than option ID's,
maybe it's possible to create a buffer overflow, and inject external
code? I'm not a CPP guru, so i don't know how to exploit this bug,
please check that.

Reproducible: Always

Steps to Reproduce:
Here's the code causing the crash (in the code i've changed the first
text value of Keygen options):

<html>
<body onload="testit()">
<script language="javascript">
function testit() {
   var s;
   s = document.getElementsByTagName("option");
   s[0].text='00000000000000000000000000000000000';
   document.crashform.submit();
}
</script>
<form name="crashform">
<keygen name="ffkey">
</form>
</body>
</html>

Actual Results:  
Mozilla Firefox 1.04 under Windows XP: crash
Mozilla Firefox 1.03 under Windows XP: crash
Mozilla Firefox 1.02 under Linux: crash
Mozilla Suite 1.6 under Windows 2000: crash
Deer Park Alpha 1 under Windows 2000: crash

Expected Results:  
Display error stopping the key generation process.
(Assignee)

Comment 1

13 years ago
I can reproduce the crash.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

13 years ago
Created attachment 185215 [details] [diff] [review]
Patch v1

Fixes the crash. The code was looking for a match of one of the two strings,
and wasn't correctly exiting the loop when a match was not found.
I don't see a security issue here. The code just loops off into space comparing
the input string to chunks of random memory, eventually getting an access
violation trying to read something.
Assignee: nobody → kaie
Group: security
Component: Security → Security: PSM
Product: Firefox → Core
QA Contact: firefox
Version: unspecified → Trunk
Comment on attachment 185215 [details] [diff] [review]
Patch v1

>-    while (choice) {
>+    while (choice && choice->name) {

You don't need to preserve the check for choice, there's really no way for it
to be null. Similar pattern of checking choice for null down in the
ProvideContent method, but that one has the correct check for choice->name in
addition.

sr=dveditz

a=dveditz for the trunk once you've got r=
Attachment #185215 - Flags: superreview+
Attachment #185215 - Flags: review?(kai.engert)
Attachment #185215 - Flags: approval1.8b3+
(Assignee)

Comment 5

13 years ago
Is anyone else able to review this patch?

Comment 6

13 years ago
Comment on attachment 185215 [details] [diff] [review]
Patch v1

Thanks for the patch

r=kaie
Attachment #185215 - Flags: review?(kai.engert) → review+
(Assignee)

Comment 7

13 years ago
Created attachment 189430 [details] [diff] [review]
Patch for checkin

Patch addressing dveditz's comments. Requesting approval for current releases.
Assignee: kaie.bugs → pkwarren
Attachment #185215 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Attachment #189430 - Flags: superreview+
Attachment #189430 - Flags: review+
Attachment #189430 - Flags: approval1.8b4?
Attachment #189430 - Flags: approval1.7.10?
Attachment #189430 - Flags: approval-aviary1.0.6?

Comment 8

13 years ago
Comment on attachment 189430 [details] [diff] [review]
Patch for checkin

Per 1.0.6 meeting, minusing patch.
Attachment #189430 - Flags: approval1.7.10?
Attachment #189430 - Flags: approval1.7.10-
Attachment #189430 - Flags: approval-aviary1.0.6?
Attachment #189430 - Flags: approval-aviary1.0.6-

Updated

13 years ago
Attachment #189430 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 9

13 years ago
Fixed on trunk.

Checking in nsKeygenHandler.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsKeygenHandler.cpp,v  <-- 
nsKeygenHandler.cpp
new revision: 1.36; previous revision: 1.35
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.