335816 - Potential XSS attack using zwnbsp on UTF-8 page

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Masatoshi Kimura [:emk]]

Reproducable: Always
Steps to reproduce:
Just load a testcase.
Actual result:
Script is executed.
Expected result:
Script should not be executed.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: testcase

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: proposed patch

Comment 3

[Masatoshi Kimura [:emk]]

I know 1.8.0 branch will be freezed very soon, however I think this is highly critical.

Comment 4

[chris hofmann]

not simon or blizzard are around for quick turn around on reviews.  can other help?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 220128 [details] [diff] [review]
proposed patch

>+  mFirst = PR_TRUE;
>+

It seems like it would be better not to touch mFirst in Reset().  The current semantics of Reset are that it resets all the per-character state.  I'd rather leave it that way and set mFirst in the constructor and where you do now, and leave the call to Reset.

>+  // Check whether it is US-ASCII first not to check in the loop.
>+  if (mFirst && aSrcLen && (0 == (0x80 & (*aSrc))))
>+    mFirst = PR_FALSE;
>+

I'm having trouble parsing the comment.  Is this just an optimization?  Optimizing for what?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, I think Reset is used by the callers to handle encoding errors (advance and continue decoding), and I don't think you want it to reset mFirst.

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: variant, splitting the word script

Should anyone doubt it, the zero-width character can split the word script, too. The displayed output on IE and Opera differ a bit between the two cases.
  <&#65279;script>alert('hello');   // in the original
  alert('hello');           // in this one.

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to comment #4)
> not simon or blizzard are around for quick turn around on reviews.  can other
> help?
jshin is also out until May 2nd per "real name" field :(
Can anyone review the I18N Library?

(In reply to comment #5)
> >+  mFirst = PR_TRUE;
> It seems like it would be better not to touch mFirst in Reset().  The current
> semantics of Reset are that it resets all the per-character state.  I'd rather
> leave it that way and set mFirst in the constructor and where you do now, and
> leave the call to Reset.
Hm... a comment in nsIDecoder.h is now out of date?
http://lxr.mozilla.org/seamonkey/source/intl/uconv/public/nsIUnicodeDecoder.h
| Resets the charset converter so it may be recycled for a completely 
| different and urelated buffer of data.

> >+  // Check whether it is US-ASCII first not to check in the loop.
> >+  if (mFirst && aSrcLen && (0 == (0x80 & (*aSrc))))
> >+    mFirst = PR_FALSE;
> I'm having trouble parsing the comment.  Is this just an optimization? 
Yes. Please correct my poor English.
> Optimizing for what?
Uconv is performance critical. This will be used all UTF-8 web page. I do not want to test mFirst with each ASCII character.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd make the comment say:

Set mFirst to PR_FALSE now so we don't have to every time through the ASCII branch within the loop

Have you looked to see if there are callers of Reset doing what's originally documented?  It's probably much less risky the way you wrote it, actually, so we should probably leave the Reset behavior as you suggested.

Comment 10

[Masatoshi Kimura [:emk]]

I didn't seek callers, but the UTF16 decoder also resets the state regarding the BOM.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/intl/uconv/ucvlatin/nsUCS2BEToUnicode.cpp&rev=1.19#125
| nsUTF16ToUnicodeBase::Reset()
| {
|   mState = 2;
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/intl/uconv/ucvlatin/nsUCS2BEToUnicode.cpp&rev=1.19#215
| nsUTF16ToUnicode::Convert(const char * aSrc, PRInt32 * aSrcLength,
|                           PRUnichar * aDest, PRInt32 * aDestLength)
| {
|   if(2 == mState) // first time called

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: changing comment

Comment only change. Carrying over sr.

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 220218 [details] [diff] [review]
changing comment

r+a = smontagu, but please file a follow-up bug to investigate Reset()

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 220218 [details] [diff] [review]
changing comment

> r+a = smontagu, but please file a follow-up bug to investigate Reset()
I've filed bug 335944.
Please check-in the patch.

Comment 14

[Simon Montagu :smontagu]

Checked into trunk and MOZILLA_1_8_BRANCH. Thank you very much for the patch!

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 220218 [details] [diff] [review]
changing comment

approved for 1.8.0 branch, a=dveditz

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> FIXED

Comment 17

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060504 Firefox/1.5.0.4

Comment 18

[Daniel Veditz [:dveditz]]

This affects older branches also.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 220218 [details] [diff] [review]
changing comment

I tested this patch on 1.7branch and aviary branch.
It works fine :-)

Comment 20

[Alexander Sack]

Chris, can you approve/checkin for aviary?

Comment 21

[Simon Montagu :smontagu]

Checked in intl/uconv/tests/test_bug335816.html