718573 - (CVE-2012-0477) Potential XSS attack with ISO-2022-KR/ISO-2022-CN near 1024 bytes

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Masato Kinugawa]

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]

Attachment: ISO-2022-KR test case

Comment 2

[Masato Kinugawa]

Attachment: ISO-2022-CN test case

Comment 3

[Masato Kinugawa]

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]]

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]

Attachment: 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]]

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]

Attachment: Updated patch

Comment 8

[Masatoshi Kimura [:emk]]

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]

(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]]

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]]

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]

Attachment: Patch v.2

Comment 13

[Simon Montagu :smontagu]

Attachment: Mochitest (covering all charsets)

Comment 14

[Masatoshi Kimura [:emk]]

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]

Are you OK with adding an assertion instead of the extra check?

Comment 16

[Masatoshi Kimura [:emk]]

(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]]

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

r=me with (dest > destEnd) changed to an assertion.

Comment 18

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24b776d91fdb

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24b776d91fdb

Comment 20

[Simon Montagu :smontagu]

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]]

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]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dac6479027cb

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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]

https://hg.mozilla.org/releases/mozilla-esr10/rev/00f9152478a7

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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]

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]]

https://hg.mozilla.org/mozilla-central/rev/58d38bc9de23

Comment 28

[Ioana (away)]

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.