Closed Bug 335816 Opened 19 years ago Closed 19 years ago

Potential XSS attack using zwnbsp on UTF-8 page

Categories

(Core :: Internationalization, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: emk, Assigned: emk)

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:high])

Attachments

(3 files, 1 obsolete file)

Reproducable: Always Steps to reproduce: Just load a testcase. Actual result: Script is executed. Expected result: Script should not be executed.
Attached file testcase
Attached patch proposed patch (obsolete) — Splinter Review
Assignee: smontagu → VYV03354
Status: NEW → ASSIGNED
Attachment #220128 - Flags: superreview?(blizzard)
Attachment #220128 - Flags: review?(smontagu)
Attachment #220128 - Flags: approval-branch-1.8.1?(smontagu)
Severity: normal → critical
I know 1.8.0 branch will be freezed very soon, however I think this is highly critical.
Flags: blocking1.8.0.4?
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:high]
Flags: blocking1.8.0.4? → blocking1.8.0.4+
not simon or blizzard are around for quick turn around on reviews. can other help?
Flags: blocking1.9a1+
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?
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.
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.
(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.
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.
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
Attachment #220128 - Flags: superreview?(blizzard) → superreview+
Attached patch changing commentSplinter Review
Comment only change. Carrying over sr.
Attachment #220128 - Attachment is obsolete: true
Attachment #220218 - Flags: superreview+
Attachment #220218 - Flags: review?(smontagu)
Attachment #220218 - Flags: approval-branch-1.8.1?(smontagu)
Attachment #220128 - Flags: review?(smontagu)
Attachment #220128 - Flags: approval-branch-1.8.1?(smontagu)
Comment on attachment 220218 [details] [diff] [review] changing comment r+a = smontagu, but please file a follow-up bug to investigate Reset()
Attachment #220218 - Flags: review?(smontagu)
Attachment #220218 - Flags: review+
Attachment #220218 - Flags: approval-branch-1.8.1?(smontagu)
Attachment #220218 - Flags: approval-branch-1.8.1+
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.
Attachment #220218 - Flags: approval1.8.0.4?
Checked into trunk and MOZILLA_1_8_BRANCH. Thank you very much for the patch!
Keywords: fixed1.8.1
Comment on attachment 220218 [details] [diff] [review] changing comment approved for 1.8.0 branch, a=dveditz
Attachment #220218 - Flags: approval1.8.0.4? → approval1.8.0.4+
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8.0.4
Resolution: --- → FIXED
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
This affects older branches also.
Flags: blocking1.7.14+
Flags: blocking-aviary1.0.9+
Comment on attachment 220218 [details] [diff] [review] changing comment I tested this patch on 1.7branch and aviary branch. It works fine :-)
Attachment #220218 - Flags: approval1.7.14?
Attachment #220218 - Flags: approval-aviary1.0.9?
Chris, can you approve/checkin for aviary?
Group: security
Flags: in-testsuite?
Checked in intl/uconv/tests/test_bug335816.html
Flags: in-testsuite? → in-testsuite+
Attachment #220218 - Flags: approval1.7.14?
Attachment #220218 - Flags: approval-aviary1.0.9?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: