Last Comment Bug 718573 - (CVE-2012-0477) Potential XSS attack with ISO-2022-KR/ISO-2022-CN near 1024 bytes
(CVE-2012-0477)
: Potential XSS attack with ISO-2022-KR/ISO-2022-CN near 1024 bytes
Status: VERIFIED FIXED
[sg:moderate][qa!]
: testcase
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 797645
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-17 00:26 PST by Masato Kinugawa
Modified: 2012-10-03 17:24 PDT (History)
9 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
affected
+
verified
+
verified
12+
verified
needed
wanted


Attachments
ISO-2022-KR test case (1.02 KB, text/html; charset=iso-2022-kr)
2012-01-17 00:28 PST, Masato Kinugawa
no flags Details
ISO-2022-CN test case (1.03 KB, text/html)
2012-01-17 00:30 PST, Masato Kinugawa
no flags Details
Patch (4.55 KB, patch)
2012-01-19 11:14 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Updated patch (33.29 KB, patch)
2012-01-20 06:30 PST, Simon Montagu :smontagu
no flags Details | Diff | Review
Patch v.2 (35.67 KB, patch)
2012-02-07 03:28 PST, Simon Montagu :smontagu
VYV03354: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr10+
akeybl: approval1.9.2.28-
Details | Diff | Review
Mochitest (covering all charsets) (5.86 KB, patch)
2012-02-07 03:29 PST, Simon Montagu :smontagu
no flags Details | Diff | Review

Description Masato Kinugawa 2012-01-17 00:26:59 PST
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.
Comment 1 Masato Kinugawa 2012-01-17 00:28:51 PST
Created attachment 589126 [details]
ISO-2022-KR test case
Comment 2 Masato Kinugawa 2012-01-17 00:30:16 PST
Created attachment 589127 [details]
ISO-2022-CN test case
Comment 3 Masato Kinugawa 2012-01-17 01:05:34 PST
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.
Comment 4 Daniel Veditz [:dveditz] 2012-01-18 16:45:28 PST
Confirmed. Was not able to convince bugzilla to give me the correct charset for these attachments so comment 3 applies.
Comment 5 Simon Montagu :smontagu 2012-01-19 11:14:43 PST
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
Comment 6 Masatoshi Kimura [:emk] 2012-01-20 05:15:34 PST
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?
Comment 7 Simon Montagu :smontagu 2012-01-20 06:30:42 PST
Created attachment 590185 [details] [diff] [review]
Updated patch
Comment 8 Masatoshi Kimura [:emk] 2012-01-20 06:54:31 PST
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)
Comment 9 Simon Montagu :smontagu 2012-01-23 02:40:23 PST
(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.
Comment 10 Masatoshi Kimura [:emk] 2012-01-23 02:54:35 PST
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.)
Comment 11 Masatoshi Kimura [:emk] 2012-02-04 06:26:10 PST
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
Comment 12 Simon Montagu :smontagu 2012-02-07 03:28:22 PST
Created attachment 594972 [details] [diff] [review]
Patch v.2
Comment 13 Simon Montagu :smontagu 2012-02-07 03:29:54 PST
Created attachment 594973 [details] [diff] [review]
Mochitest (covering all charsets)
Comment 14 Masatoshi Kimura [:emk] 2012-02-07 04:18:49 PST
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.
Comment 15 Simon Montagu :smontagu 2012-02-07 07:17:25 PST
Are you OK with adding an assertion instead of the extra check?
Comment 16 Masatoshi Kimura [:emk] 2012-02-08 02:35:14 PST
(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 17 Masatoshi Kimura [:emk] 2012-02-08 02:36:26 PST
Comment on attachment 594972 [details] [diff] [review]
Patch v.2

r=me with (dest > destEnd) changed to an assertion.
Comment 19 Ed Morley [:emorley] 2012-02-10 05:26:12 PST
https://hg.mozilla.org/mozilla-central/rev/24b776d91fdb
Comment 20 Simon Montagu :smontagu 2012-03-06 01:06:54 PST
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
Comment 21 Alex Keybl [:akeybl] 2012-03-06 15:17:30 PST
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.
Comment 22 Simon Montagu :smontagu 2012-03-07 11:49:56 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/dac6479027cb
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-03-20 11:48:08 PDT
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
Comment 24 Simon Montagu :smontagu 2012-03-20 14:37:26 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/00f9152478a7
Comment 25 Al Billings [:abillings] 2012-04-23 15:14:35 PDT
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
Comment 26 Simon Montagu :smontagu 2012-05-20 02:52:09 PDT
Landed mochitest now that the bug is open: https://hg.mozilla.org/integration/mozilla-inbound/rev/58d38bc9de23. Should the test land on branches also?
Comment 27 Ed Morley [:emorley] 2012-05-20 23:42:21 PDT
https://hg.mozilla.org/mozilla-central/rev/58d38bc9de23
Comment 28 Ioana (away) 2012-06-03 05:31:05 PDT
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.

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