Last Comment Bug 335816 - Potential XSS attack using zwnbsp on UTF-8 page
: Potential XSS attack using zwnbsp on UTF-8 page
Status: RESOLVED FIXED
[sg:high]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Masatoshi Kimura [:emk]
: Yuying Long
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-28 08:13 PDT by Masatoshi Kimura [:emk]
Modified: 2016-03-15 17:35 PDT (History)
12 users (show)
dveditz: blocking1.7.14+
dveditz: blocking‑aviary1.0.9+
chofmann: blocking1.9a1+
dveditz: blocking1.8.0.4+
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (314 bytes, text/html; charset=UTF-8)
2006-04-28 08:14 PDT, Masatoshi Kimura [:emk]
no flags Details
proposed patch (4.02 KB, patch)
2006-04-28 08:17 PDT, Masatoshi Kimura [:emk]
dbaron: superreview+
Details | Diff | Splinter Review
variant, splitting the word script (314 bytes, text/html)
2006-04-28 16:57 PDT, Daniel Veditz [:dveditz]
no flags Details
changing comment (4.07 KB, patch)
2006-04-28 18:55 PDT, Masatoshi Kimura [:emk]
smontagu: review+
VYV03354: superreview+
smontagu: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2006-04-28 08:13:50 PDT
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] 2006-04-28 08:14:37 PDT
Created attachment 220127 [details]
testcase
Comment 2 Masatoshi Kimura [:emk] 2006-04-28 08:17:02 PDT
Created attachment 220128 [details] [diff] [review]
proposed patch
Comment 3 Masatoshi Kimura [:emk] 2006-04-28 09:35:28 PDT
I know 1.8.0 branch will be freezed very soon, however I think this is highly critical.
Comment 4 chris hofmann 2006-04-28 15:23:20 PDT
not simon or blizzard are around for quick turn around on reviews.  can other help?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-28 15:46:20 PDT
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-7 (review requests must explain patch) 2006-04-28 15:49:39 PDT
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] 2006-04-28 16:57:30 PDT
Created attachment 220213 [details]
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] 2006-04-28 18:19:17 PDT
(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-7 (review requests must explain patch) 2006-04-28 18:34:53 PDT
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] 2006-04-28 18:38:47 PDT
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] 2006-04-28 18:55:17 PDT
Created attachment 220218 [details] [diff] [review]
changing comment

Comment only change. Carrying over sr.
Comment 12 Simon Montagu :smontagu 2006-04-29 14:20:56 PDT
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] 2006-04-29 14:45:10 PDT
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 2006-04-29 23:53:06 PDT
Checked into trunk and MOZILLA_1_8_BRANCH. Thank you very much for the patch!
Comment 15 Daniel Veditz [:dveditz] 2006-04-30 11:33:34 PDT
Comment on attachment 220218 [details] [diff] [review]
changing comment

approved for 1.8.0 branch, a=dveditz
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-04-30 22:57:23 PDT
-> FIXED
Comment 17 Tracy Walker [:tracy] 2006-05-04 08:44:04 PDT
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] 2006-05-30 15:57:59 PDT
This affects older branches also.
Comment 19 Masatoshi Kimura [:emk] 2006-05-31 14:57:05 PDT
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 2006-06-13 15:10:41 PDT
Chris, can you approve/checkin for aviary?
Comment 21 Simon Montagu :smontagu 2007-12-06 09:06:47 PST
Checked in intl/uconv/tests/test_bug335816.html

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