1224505 - Possible XSS via ISO-2022-JP encoding

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Takeshi Terada]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Serves a web page in ISO-2022-JP charset contains the following byte sequence.

<p>[0x1B][0x1B]$B<SCRIPT>eval("al\ert\(document\.domain\)")</SCRIPT>X[0x1B](B</p>

"[0x1B]$B" in the bytes above is an escape sequence of JIS X 0208 in ISO-2022-JP, and "[0x1B](B" is an escape sequence of ASCII.



Actual results:

The bytes from "[0x1B]$B" to "[0x1B](B" should be treated as JIS X 0208, not ASCII. Major browsers other than Firefox treat it as such, but firefox interprets it as an ASCII data and execute the JavaScript (it shows a popup window.)

This happens in Firefox, because, in the HTML above, ESC ([0x1B]) is placed immediately before the escape sequence "[0x1B]$B". Firefox seems to ignore a byte occurs right after a bare ESC char that does not constitute a valid escape sequence.

Note that Some ISO-2022-JP encoders, including one used in Firefox, seem to produce a string containing a bare ESC when encoding. Thus, maybe decoders should assume a bare ESC occurence while decoding.

P.S.
The severity of the bug may not be high, as ISO-2022-JP (as web page encoding) isn't common.

Comment 1

[:Gijs (he/him)]

Henri, I remember you did some work with our charset code. Are you the right person to look at this or do you want to nominate someone else?

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Test case

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Test case, for real

Comment 4

[Henri Sivonen (:hsivonen)]

I was hoping we could rewrite the ISO-2022-JP decoder in Rust some time in the future in a relaxed way. Now it looks like we need to do a partial rewrite as a security bug. :-(

Basically all the 'else' branches in the escape states are bogus and need to be rewritten to decrement src, then readjust mState and then either signal error or emit U+FFFD. Additionally, for the decrement to work, we need to read src to a variable and increment src at the top of the loop. Instead of reading *src all over and incrementing at the end, since it's not done if the output buffer is found to be full first.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Basically all the 'else' branches in the escape states are bogus and need to
> be rewritten to decrement src, then readjust mState and then either signal
> error or emit U+FFFD. Additionally, for the decrement to work, we need to
> read src to a variable and increment src at the top of the loop. Instead of
> reading *src all over and incrementing at the end, since it's not done if
> the output buffer is found to be full first.

Looks like a middle-ground fix like that won't work. Either we need to rewrite the loop and states according to the Encoding Standard or we need to add checks for 0x1B to each state and make transitions to mState_ESC. If we do the former, we might as well get rid of the ISO-2022-JP-2 stuff while at it. If the we do the latter, getting the error reporting right is going to be annoying.

Comment 6

[Henri Sivonen (:hsivonen)]

emk, smontagu, are you OK with removing the ISO-2022-JP-2 features in a security fix?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to :Gijs Kruitbosch from comment #1)
> Henri, I remember you did some work with our charset code. Are you the right
> person to look at this or do you want to nominate someone else?

I probably won't have the time to *fix* this in the coming days, so if emk or smontagu wants to take the bug, please go ahead.

Comment 8

[Masatoshi Kimura [:emk]]

Let's file a non-security bug and send "Intent to unship" to dev-platform for the removal of ISO-2022-JP-2 support.

Comment 9

[Simon Montagu :smontagu]

See also bug 715833 comment 2 which implies that we still need iso-2022-jp-2 for email. That was three years ago -- if Masatoshi is happy removing it today, I have no objection.

Comment 10

[Anne (:annevk)]

Fixing bug 715833 seems like the way to go.

Note also https://github.com/whatwg/encoding/issues/15 about the iso-2022-jp encoder which I have not addressed yet.

Comment 11

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

Triage committee is trying to figure out a security severity rating for this. What are the implications of this problem?

Comment 12

[Anne (:annevk)]

Well, when a site uses iso-2022-jp (unlikely) and accepts user input, it may be susceptible to XSS if a) they accept user input and output that somewhere on their site and b) their server iso-2022-jp decoder/encoder pair is different from the one in Firefox (likely).

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Anne (:annevk) from comment #12)
> Well, when a site uses iso-2022-jp (unlikely) and accepts user input, it may
> be susceptible to XSS if a) they accept user input and output that somewhere
> on their site and b) their server iso-2022-jp decoder/encoder pair is
> different from the one in Firefox (likely).

More precisely, it seems to me this requires the site not to decode and re-encode.

So:

If all the following hold

 1) Site use ISO-2022-JP
 2) Site accepts user input
 3) Site serves input from one user to another user
 4) Site serves the bytes the bytes that it received from a user (as opposed to performing some processing that converts from ISO-2022-JP into something else and then back to ISO-2022-JP resulting in freshly-encoded bytes)

...the site may be vulnerable to one user being able to inject scripts that run on the origin of the site in another user's Firefox.

The outcome qualifies as sec-high, but the preconditions limit exploitability. For starters, only 0.05% of release channel sessions instantiate the ISO-2022-JP decoder at least once during the session.

Comment 14

[Henri Sivonen (:hsivonen)]

Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/dt87ijrusmk

Comment 15

[Henri Sivonen (:hsivonen)]

(to unship the -2 parts, that is.)

Comment 16

[Henri Sivonen (:hsivonen)]

Fixed by bug 1261841. (The test case used to have the wrong HTTP charset. I verified after changing the attachment properties moments ago.)

Comment 17

[Daniel Veditz [:dveditz]]

I assume we won't implement the rewrite in bug 1261841 on the 52ESR branch.