The default bug view has changed. See this FAQ.

Potential XSS attack using zwnbsp on UTF-8 page

RESOLVED FIXED

Status

()

Core
Internationalization
--
critical
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
fixed1.8.1, verified1.8.0.4
Points:
---
Bug Flags:
blocking1.7.14 +
blocking-aviary1.0.9 +
blocking1.9a1 +
blocking1.8.0.4 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Reproducable: Always
Steps to reproduce:
Just load a testcase.
Actual result:
Script is executed.
Expected result:
Script should not be executed.
(Assignee)

Comment 1

11 years ago
Created attachment 220127 [details]
testcase
(Assignee)

Comment 2

11 years ago
Created attachment 220128 [details] [diff] [review]
proposed patch
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)
(Assignee)

Updated

11 years ago
Severity: normal → critical
(Assignee)

Comment 3

11 years ago
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+

Comment 4

11 years ago
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.
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.
(Assignee)

Comment 8

11 years ago
(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.
(Assignee)

Comment 10

11 years ago
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+
(Assignee)

Comment 11

11 years ago
Created attachment 220218 [details] [diff] [review]
changing comment

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+
(Assignee)

Comment 13

11 years ago
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
Last Resolved: 11 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
Keywords: fixed1.8.0.4 → verified1.8.0.4
This affects older branches also.
Flags: blocking1.7.14+
Flags: blocking-aviary1.0.9+
(Assignee)

Comment 19

11 years ago
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?

Comment 20

11 years ago
Chris, can you approve/checkin for aviary?
Group: security

Updated

9 years ago
Flags: in-testsuite?
Checked in intl/uconv/tests/test_bug335816.html
Flags: in-testsuite? → in-testsuite+
(Assignee)

Updated

a year ago
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.