Bug 718573 (CVE-2012-0477)

Potential XSS attack with ISO-2022-KR/ISO-2022-CN near 1024 bytes

VERIFIED FIXED in Firefox 12

Status

()

Core
Internationalization
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Masato Kinugawa, Assigned: smontagu)

Tracking

({testcase})

Trunk
mozilla13
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 affected, firefox11+ affected, firefox12+ verified, firefox13+ verified, firefox-esr1012+ verified, blocking1.9.2 needed, status1.9.2 wanted)

Details

(Whiteboard: [sg:moderate][qa!])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Firefox has risk of potential XSS on ISO-2022-KR/ISO-2022-CN page.


Actual results:

On ISO-2022-KR page, characters near 1024 bytes is copied on back. On ISO-2022-CN page, characters near 1024 bytes is deleted.


Expected results:

Firefox should not copy or delete.
(Reporter)

Comment 1

5 years ago
Created attachment 589126 [details]
ISO-2022-KR test case
(Reporter)

Comment 2

5 years ago
Created attachment 589127 [details]
ISO-2022-CN test case
(Reporter)

Updated

5 years ago
Attachment #589127 - Attachment mime type: text/plain → text/html
(Reporter)

Updated

5 years ago
Attachment #589126 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 3

5 years ago
Oops, test case does not work from this link. So far, I don't know this reason. Please save and test. You will be able to reproduce this.
Confirmed. Was not able to convince bugzilla to give me the correct charset for these attachments so comment 3 applies.
Assignee: nobody → smontagu
Status: UNCONFIRMED → NEW
Component: Untriaged → Internationalization
Ever confirmed: true
Product: Firefox → Core
QA Contact: untriaged → i18n
Whiteboard: [sg:moderate]
(Assignee)

Updated

5 years ago
Attachment #589126 - Attachment mime type: text/html → text/html; charset=iso-2022-kr
(Assignee)

Comment 5

5 years ago
Created attachment 589920 [details] [diff] [review]
Patch

I included ISO-2022-CN in the patch, even though we may prefer to remove the decoder anyway, as suggested in bug 470523; and also ISO-2022-JP for consistency
Attachment #589920 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #589920 - Flags: review? → review?(VYV03354)
Comment on attachment 589920 [details] [diff] [review]
Patch

> I included ISO-2022-CN in the patch, even though we may prefer to remove the 
> decoder anyway, as suggested in bug 470523; and also ISO-2022-JP for consistency
Where is ISO-2022-JP in the patch?
(Assignee)

Comment 7

5 years ago
Created attachment 590185 [details] [diff] [review]
Updated patch
Attachment #589920 - Attachment is obsolete: true
Attachment #589920 - Flags: review?(VYV03354)
Attachment #590185 - Flags: review?(VYV03354)
Comment on attachment 590185 [details] [diff] [review]
Updated patch

> -           if(dest+1 >= destEnd)
> +           if(dest+1 > destEnd)
Is this legal per C spec? If |dest+1| pass over the end of the array, the behavior will be undefined. It should be:
           if (destEnd - dest < 1)
(Assignee)

Comment 9

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #8)
> > -           if(dest+1 >= destEnd)
> > +           if(dest+1 > destEnd)
> Is this legal per C spec? If |dest+1| pass over the end of the array, the
> behavior will be undefined. It should be:
>            if (destEnd - dest < 1)

I don't understand this: dest and destEnd are just PRUnichar* pointers, and aren't being dereferenced here.
Assignee: smontagu → nobody
Component: Internationalization → Installer: XPInstall Engine
QA Contact: i18n → xpi-engine
(Assignee)

Updated

5 years ago
Component: Installer: XPInstall Engine → Internationalization
OS: Windows Vista → All
QA Contact: xpi-engine → i18n
Hardware: x86 → All
Version: 9 Branch → Trunk
(Assignee)

Updated

5 years ago
Assignee: nobody → smontagu
It causes undefined behavior even if it is not dereferenced per C spec.
Consider about what happens if dest == destEnd == 65535 on MS-DOS small model. (I know we don't support 16-bits environments, but I think we shouldn't violate the spec without a good reason.)
Quotation from C99 6.5.6 Additive operators:
> If both the pointer operand and the result point to elements of the same array
> object, or one past the last element of the array object, the evaluation shall
> not produce an overflow; otherwise, the behavior is undefined.

See also C FAQ 6.17.
http://c-faq.com/aryptr/non0based.html
(Assignee)

Comment 12

5 years ago
Created attachment 594972 [details] [diff] [review]
Patch v.2
Attachment #590185 - Attachment is obsolete: true
Attachment #590185 - Flags: review?(VYV03354)
Attachment #594972 - Flags: review?(VYV03354)
(Assignee)

Comment 13

5 years ago
Created attachment 594973 [details] [diff] [review]
Mochitest (covering all charsets)
Comment on attachment 594973 [details] [diff] [review]
Mochitest (covering all charsets)

> #define CHECK_OVERRUN(dest, destEnd, length) ((dest > destEnd) || ((destEnd - dest) < length))
Is |(dest > destEnd)| required? It will never hold unless the program has a bug.
I do not want to add extra checks because the ISO-2022-JP decoder is still heavily used in Japan.
(Assignee)

Comment 15

5 years ago
Are you OK with adding an assertion instead of the extra check?
(In reply to Simon Montagu from comment #15)
> Are you OK with adding an assertion instead of the extra check?
I'm fine with that.
Comment on attachment 594972 [details] [diff] [review]
Patch v.2

r=me with (dest > destEnd) changed to an assertion.
Attachment #594972 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b776d91fdb
blocking1.9.2: --- → ?
status-firefox-esr10: --- → affected
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/24b776d91fdb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox13: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

5 years ago
tracking-firefox-esr10: ? → .1+
tracking-firefox11: ? → +
tracking-firefox12: ? → +
tracking-firefox13: ? → +

Updated

5 years ago
tracking-firefox-esr10: .1+ → 13+
blocking1.9.2: ? → needed
status1.9.2: --- → wanted
(Assignee)

Comment 20

5 years ago
Comment on attachment 594972 [details] [diff] [review]
Patch v.2

[Approval Request Comment]
Regression caused by (bug #): Not a regression.
User impact if declined: Possibility of XSS caused by omitted or doubled bytes when decoding
Testing completed (on m-c, etc.): Baked on trunk for 3 weeks.
Risk to taking this patch (and alternatives if risky): No known risk.
String changes made by this patch: None
Attachment #594972 - Flags: approval1.9.2.28?
Attachment #594972 - Flags: approval-mozilla-esr10?
Attachment #594972 - Flags: approval-mozilla-beta?
Attachment #594972 - Flags: approval-mozilla-aurora?
Comment on attachment 594972 [details] [diff] [review]
Patch v.2

[Triage Comment]
Approving for Aurora 12, but we're too late in FF11's release too take an sg:moderate bug. Also clearing the approval‑mozilla‑esr10 and adding the ESR tracking flag instead - we'll approve once 12 is on Beta.
Attachment #594972 - Flags: approval1.9.2.28?
Attachment #594972 - Flags: approval1.9.2.28-
Attachment #594972 - Flags: approval-mozilla-esr10?
Attachment #594972 - Flags: approval-mozilla-beta?
Attachment #594972 - Flags: approval-mozilla-beta-
Attachment #594972 - Flags: approval-mozilla-aurora?
Attachment #594972 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox-esr10: 13+ → 12+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/dac6479027cb
status-firefox12: affected → fixed
Comment on attachment 594972 [details] [diff] [review]
Patch v.2

[Triage Comment]
12 is on Beta now, please go ahead and land this to ESR branch as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #594972 - Flags: approval-mozilla-esr10+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/00f9152478a7
status-firefox-esr10: affected → fixed
Whiteboard: [sg:moderate] → [sg:moderate][qa+]
Keywords: testcase
Alias: CVE-2012-0477

Updated

5 years ago
status-firefox12: fixed → verified
Verified in trunk using nightly with attached testcases.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120420 Firefox/14.0a1
Status: RESOLVED → VERIFIED

Updated

5 years ago
status-firefox13: fixed → verified
Group: core-security
(Assignee)

Comment 26

5 years ago
Landed mochitest now that the bug is open: https://hg.mozilla.org/integration/mozilla-inbound/rev/58d38bc9de23. Should the test land on branches also?
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/58d38bc9de23

Comment 28

5 years ago
Mozilla/5.0 (Windows NT 6.1; rv:10.0.5) Gecko/20100101 Firefox/10.0.5
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.5) Gecko/20100101 Firefox/10.0.5
Mozilla/5.0 (X11; Linux i686; rv:10.0.5) Gecko/20100101 Firefox/10.0.5

Verified as fixed using the attached testcases.
status-firefox-esr10: fixed → verified
Whiteboard: [sg:moderate][qa+] → [sg:moderate][qa!]
Depends on: 797645
You need to log in before you can comment on or make changes to this bug.