Closed
Bug 228856
(CVE-2008-5510)
Opened 21 years ago
Closed 16 years ago
[FIX] \0 in CSS is ignored
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b2
People
(Reporter: llo8or4sf, Assigned: zwol)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [sg:low] xss hazard)
Attachments
(3 files, 10 obsolete files)
10.67 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
20.68 KB,
patch
|
dveditz
:
approval1.9.0.5+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
dveditz
:
approval1.8.1.19+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031213
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031213
In CSS, \0 means a null character. But it is ignored by Mozilla.
(ex. "c\0olor:red;" is treated as "color:red;".)
BTW, a raw null character in CSS is treated correctly as null
by Mozilla.
(ex. "c*olor:red;" (*=null character(x00)) is not treated as
"color:red".)
Reproducible: Always
Steps to Reproduce:
Actual Results:
\0 in CSS is ignored.
Expected Results:
\0 in CSS is treated as a null character. (equal to a raw null
character)
This \0 ignoring has possibility to avoid sanitizing of web application (webmail
etc.).
Internet Explorer executes JScript in CSS so \0 can be a vulnerability.
http://altba.com/bakera/bug/nul-test.html
http://www.st.ryukoku.ac.jp/%7Ekjm/security/memo/2003/12.html#20031216_IE
Though Mozilla does not executes JavaScript in CSS (so far as I know),
it can use for web bug easily. (ex. @i\0mport )
Comment 3•21 years ago
|
||
http://www.w3.org/TR/CSS21/syndata.html#q6 says (third bullet):
"Third, backslash escapes allow authors to refer to characters
they can't easily put in a document. In this case, the backslash
is followed by at most six hexadecimal digits (0..9A..F), which
stand for the ISO 10646 ([ISO10646]) character with that number.
If a character in the range [0-9a-zA-Z] follows the hexadecimal
number, the end of the number needs to be made clear. There are
two ways to do that:
1. with a space (or other whitespace character): "\26 B" ("&B").
In this case, user agents should treat a "CR/LF" pair (13/10)
as a single whitespace character.
2. by providing exactly 6 hexadecimal digits: "\000026B" ("&B")
"
AFAICT, the two ways mentioned above to write zero is:
c\0 olor: red;
c\000000olor: red;
which means that your example has an illegal unicode escape sequence.
The question is, shouldn't this property name fail to parse in this case?
Comment 4•21 years ago
|
||
I think I remember seeing a comment in www-style: there's a typo in the CSS21
spec. That paragraph should read:
"If a character in the range [0-9a-fA-F] follows the hexadecimal
number, the end of the number needs to be made clear"
In other words, you only need a space if the end of the escape is ambiguous
(e.g., 'eat\0food'). 'c\0olor' is unambigous and (as far as I can see) valid.
Comment 5•21 years ago
|
||
Here's the comment I was thinking of, for completeness:
http://lists.w3.org/Archives/Public/www-style/2003Oct/0075.html
Comment 6•21 years ago
|
||
Sounds reasonable to me. Confirming bug.
legal backslash escape suggested by Mats.
\0 is ignored also. (same result attachment 137652 [details])
Mats:
> The question is, shouldn't this property name fail to parse in this case?
Yes. I think not to ignore \0 or \000000 specially.
\0 or \000000 should be unescaped to a null character as well as other
backslash escapes, even if it breaks the property name.
Comment 9•21 years ago
|
||
I don't fully understand the previous comment, but I'd note that CSS 2.1 (even
the current WD) doesn't place any restrictions on identifier names,
specifically: "In CSS 2.1, identifiers ... can also contain ... any ISO 10646
character as a numeric code".
Updated•21 years ago
|
Hardware: PC → All
Reporter | ||
Comment 10•21 years ago
|
||
To Malcom.
I'm sorry that I confused you. I could not explain my thought in
English well.
(Also, I may misunderstand the opinion of Mats.)
It is certain that I misused the word, "specially".
My opinion is only that \0 and \000000 in CSS should not be ignored
by Mozilla.
Other backslash escaped characters aren't ignored by Mozilla.
A raw null character isn't ignored too.
Only \0 and \000000 are ignored. It is strange behavior.
I used "specially" to intend to point out the difference.
Hardware: All → PC
Comment 11•21 years ago
|
||
yes, nobody is arguing this is not a bug. :-)
Comment 12•21 years ago
|
||
I think the problem is basically here:
http://lxr.mozilla.org/mozilla/source/content/html/style/src/nsCSSScanner.cpp#75
9
746 PRBool nsCSSScanner::GatherIdent(nsresult& aErrorCode, PRInt32 aChar,
747 nsString& aIdent)
...
758 if (aChar == CSS_ESCAPE) {
759 aChar = ParseEscape(aErrorCode);
760 if (0 < aChar) {
761 aIdent.Append(PRUnichar(aChar));
762 }
We don't append the unescaped character if it's NUL. AFAICS, this is only
because ParseEscape overloads its return value so that zero means either 'no
character' or 'ASCII NUL'.
In ParseEscape, 'No character' is used only once (line 730), which deals with
<backslash><newline>. Oddly, <backslash> at the end of the stream will return
<backslash> (line 674). I can't see that ParseEscape will ever return zero
otherwise, except where a legitimate NUL is escaped.
So, it would help if we changed ParseEscape to return -1 for 'no character' in
the <bs><nl> case (unless Ian has any better ideas) -- line 730 of
nsCSSScanner.cpp.
However, the output is written to an nsString, which (IIRC) is a zero-
terminated string. I think it would be 'unhappy' if it contained a NUL
character.
So to fix this, we'd need to:
1. Change lines that check the return value of ParseEscape to allow a zero
return to mean 'NUL' -- lines 632, 752, 760 (as above), 954.
2. Write the output into something that can accept NULs.
3. Change consumers to use that 'something'.
As well as identifiers, this also affects \0 in url's and quoted strings. I
don't know what the effects might be of allowing NUL's into strings further up
the chain, though.
Comment 13•21 years ago
|
||
Ian: any chance you could suggest that the NUL character (escaped or unescaped)
should be disallowed (ignored?) in CSS files entirely? :)
Seriously, I can't see any good reason for allowing it, and it sounds like a
nightmare to implement. Backwards compatibility with CSS1/CSS2 might be one
reason, but I doubt any existing CSS parsers handle the situation correctly.
Comment 14•21 years ago
|
||
I don't really see that it should be disallowed, although admittedly I see very
few reasons for allowing it.
Comment 15•21 years ago
|
||
Actually, maybe an nsString can contain NUL characters after all?
http://www.mozilla.org/projects/xpcom/string-guide.html#Concrete_Classes says
it's NUL-terminated, but the implementation has a length count, so maybe it can.
It can't contain null characters. We should probably just put in 0xfffd.
Comment 17•21 years ago
|
||
Not that it would matter in the real world, but that would technically make us
non-compliant since the following two rules would become equivalent:
.t\0 est { ... }
.t\fffd est { ... }
...both matching:
<div class="t�est"></div>
...and neither matching:
<div class="t�est"></div>
But on the other hand, that would never matter in the real world.
Comment 18•21 years ago
|
||
Any fix for this should probably also take into account the embedded NUL case
(attachment 137653 [details]). At the moment, I suspect we parse '.c<NUL>olor' as '.c'.
I don't think there's any way to allow NUL characters (escaped or not) without
also breaking something else. dbaron's comment 16 sounds like a sensible
compromise.
Comment 19•21 years ago
|
||
Comment 20•21 years ago
|
||
David, I think you're wrong regarding nsString -
it handles embedded NUL characters just fine.
This bug is caused by three separate problems.
1. nsCSSScanner::ParseEscape returns zero to indicate it
found an escaped newline (that the caller should ignore).
2. nsCSSValue and nsStyleContentData uses PRUnichar* which
results in truncation at the first embedded NUL.
3. CSS class and property names are Atomized but nsAtomTable
and pldhash.c doesn't handle strings/keys with embedded NULs
(in fact truncates them so "col\0or" matches "col").
This patch fixes 1. and 2.
Comment 21•21 years ago
|
||
I could take at stab at 3. as well but I'm not sure if there are other plans
(or bugs) for Atoms that should be considered at the same time...
Updated•21 years ago
|
Attachment #137906 -
Flags: review?(dbaron)
Comment 22•21 years ago
|
||
Comment on attachment 137905 [details]
Testcase #4, 'content' with embedded NUL
Just a note to point out that this testcase doesn't actually include any
embedded NUL characters (by which I meant an actual, literal, binary ASCII zero
character).
Comment 23•21 years ago
|
||
Comment on attachment 137906 [details] [diff] [review]
Patch rev. 1
> PRBool nsCSSScanner::ParseNumber(nsresult& aErrorCode, PRInt32 c,
>@@ -947,11 +947,8 @@
> }
> if (ch == CSS_ESCAPE) {
> ch = ParseEscape(aErrorCode);
>- if (ch < 0) {
>- return PR_FALSE;
>- }
> }
>- if (0 < ch) {
>+ if (0 <= ch) {
> aBuffer.Append(PRUnichar(ch));
> }
> }
This changes the return value of ParseNumber from PR_FALSE to PR_TRUE in the
<bs><nl> case. Was that intentional?
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrameManager.cpp,v
>RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,v
Why the changes from nsAutoString to nsString here? You should be able to
initialise an nsAutoString from a nsAString.
Comment 24•21 years ago
|
||
> 3. CSS class and property names are Atomized but nsAtomTable
> and pldhash.c doesn't handle strings/keys with embedded NULs
FYI: Nothing inherent in pldhash.c precludes counted strings rather than
terminated strings as keys.
/be
Updated•21 years ago
|
Attachment #137906 -
Attachment is obsolete: true
Attachment #137906 -
Flags: review?(dbaron)
Comment 25•21 years ago
|
||
The fact is that pldhash.c does not _support_ counted strings natively
and working around that limitation was tricky...
Taking bug since I have a complete patch now.
Assignee: dbaron → mats.palmgren
Summary: \0 in CSS is ignored → [FIX] \0 in CSS is ignored
Updated•21 years ago
|
Status: NEW → ASSIGNED
Comment 26•21 years ago
|
||
Re: comment 23
> This changes the return value of ParseNumber from PR_FALSE to
> PR_TRUE in the <bs><nl> case.
No, it doesn't. The old ParseEscape() returns zero for this case and it
was rightly ignored by both the cited removed tests. The new version
of ParseEscape() now returns -1 for this case and it's still ignored.
(Please note that the old ParseEscape() never returned negative values
at all, so the removed |return PR_FALSE| is just dead code elimination.)
Also note that the cited code is in nsCSSScanner::GatherString() and not
in nsCSSScanner::ParseNumber() - this is probably "cvs diff -pu" not being
very good at parsing C++. (I have not changed ParseNumber at all).
You are right about the behaviour though - both the old and new version
fails on <bs><nl> when a number is expected but AFAICT this is not a bug.
This <bs><nl> syntax is only allowed for strings, see:
http://www.w3.org/TR/CSS21/syndata.html#q6
> You should be able to initialise an nsAutoString from a nsAString.
Yes, but I believe nsString(nsString& x) is more efficient than
nsAutoString(nsString& x).
Comment 27•21 years ago
|
||
Attachment #137652 -
Attachment is obsolete: true
Attachment #137653 -
Attachment is obsolete: true
Attachment #137699 -
Attachment is obsolete: true
Comment 28•21 years ago
|
||
There are two changes since rev. 1:
1. changed nsAString* to nsString* in the nsStyleContentData struct.
2. I discovered an additional embedded NUL problem; in the special parsing
of the "class" HTML attribute value. (function ParseClasses() in
file content/html/style/src/nsHTMLAttributes.cpp)
Comment 29•21 years ago
|
||
This is the XPCOM part of the fix.
The changes to nsStaticCaseInsensitiveNameTable are straightforward,
(we save a pointer in NameTableEntry).
The nsAtomTable changes are trickier, mostly because I wanted to keep
the type nsStaticAtom as is. I used the same ptr|0x1 trick that is
already used to mark a "wrapped atom". I had to add a 32 bit length
field to nsAtomImpl though.
I also want to point out an unrelated bug I found while experimenting
with different solutions. At one point I had added a member to
nsStaticAtomWrapper and that caused crashes because data was overwritten:
- PL_ARENA_ALLOCATE(mem, gStaticAtomArena, sizeof(nsStaticAtom));
+ PL_ARENA_ALLOCATE(mem, gStaticAtomArena, sizeof(nsStaticAtomWrapper));
If anyone can give me some feedback on the performance of this patch
that would be appreciated. I do see a couple of things that can be
done to improve performance a bit more if this patch is accepted.
Technically nsString doesn't allow embedded null characters, even if they work,
since nsString is-a nsAFlatString, which has a get method.
Updated•21 years ago
|
Attachment #138244 -
Flags: review?(dbaron)
Updated•21 years ago
|
Attachment #138245 -
Flags: review?(brendan)
Comment 31•21 years ago
|
||
Regarding the <bs><nl> syntax; We support it for identifiers as
well as for strings, eg .na\
me is parsed as .name but I can't find anything in the CSS2.1 spec. regarding
this, is this just an oversight in the spec. or is it a bug?
http://www.w3.org/TR/2004/CR-CSS21-20040225/syndata.html#q6 forbids character
escapes for the zero-character.
Comment 33•21 years ago
|
||
So should \0 then cause the whole declaration/rule/whatever to be ignored?
I think the \ should tokenize as DELIM just like it would if it were followed by
something that wasn't a hex digit.
Comment 35•21 years ago
|
||
(In reply to comment #34)
> I think the \ should tokenize as DELIM just like it would if it were followed
> by something that wasn't a hex digit.
If it were followed by something that wasn't a hex digit, it would escape that
character, not tokenise as DELIM.
Comment on attachment 138244 [details] [diff] [review]
Patch1 rev. 2 (Style System)
Per recent changes to CSS, this isn't what we want to do. I'm not entirely
sure what is, though...
Attachment #138244 -
Flags: review?(dbaron) → review-
Updated•21 years ago
|
Attachment #138245 -
Flags: review?(brendan) → review?(dbaron)
Comment on attachment 138245 [details] [diff] [review]
Patch2 rev. 1 (XPCOM)
I'm not sure how much of this patch is still relevant, but a few comments:
* have you thought about the effects of this change on performance? Some of
this code is used a lot.
>Index: xpcom/ds/nsStaticNameTable.h
>-class NS_COM nsStaticCaseInsensitiveNameTable
>+class NS_COM nsStaticCaseInsensitiveNameTable :
>+ public PLDHashTable
> {
> public:
> enum { NOT_FOUND = -1 };
>@@ -69,7 +70,6 @@ public:
>
> private:
> nsDependentCString* mNameArray;
>- PLDHashTable mNameTable;
> nsDependentCString mNullStr;
> };
Why did you need to do this? Could you avoid making this public?
The callers of GatherIdent as well and the printing of error tokens need a good
bit of additional work.
Updated•19 years ago
|
Attachment #138245 -
Flags: review?(dbaron)
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:want]
QA Contact: ian → style-system
Comment 39•16 years ago
|
||
Anything we can do to poke this bug back into life?
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Whiteboard: [sg:want] → [sg:low] xss hazard
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Assignee | ||
Comment 40•16 years ago
|
||
All the patches on here are quite invasive and no longer applicable to trunk, so here's a minimum-necessary-change fix versus moz-central, that hopefully can go on many of the live branches, either as is or with adjustment for churn. It modifies nsCSSScanner::ParseAndAppendEscape and nothing else; and what it does is apply the \-<any character other than a hex digit> rule to \0, so e.g. "\000" is equivalent to "000". Note that this is *not* what Mats' test cases expect; they expect \0 (any form) to be equivalent to a literal NUL, but that is clearly forbidden by CSS2.1 section 4.1.3 ("[the number] must not be zero").
There are two reftests in here. One checks for *exactly* the behavior I coded, inside strings. The other began as Mats' testcase #5 but no longer bears any resemblance; it checks all the cases I could think of where a \0 or literal NUL character should prevent a style rule from matching. Four out of the umpteen subcases of this test fail; these constructs
#id.\0 { color: red }
.cl#\0 { color: red }
are matching <div id="id" class="�"> and <div class="cl" id="�"> respectively, but it seems like they shouldn't. If I could get a little help with that, we could maybe put this one to bed soon.
(I regret that some of the new files are uuencoded and thus not inspectible in-browser. They contain embedded NULs.)
Assignee: mats.palmgren → zweinberg
Attachment #138244 -
Attachment is obsolete: true
Attachment #138245 -
Attachment is obsolete: true
Attachment #178082 -
Attachment is obsolete: true
Attachment #343767 -
Flags: superreview?(dbaron)
Attachment #343767 -
Flags: review?(dbaron)
(In reply to comment #40)
> Four out of the umpteen
> subcases of this test fail; these constructs
>
> #id.\0 { color: red }
> .cl#\0 { color: red }
>
> are matching <div id="id" class="�"> and <div class="cl" id="�">
> respectively, but it seems like they shouldn't. If I could get a little help
> with that, we could maybe put this one to bed soon.
Are the class and ID attributes in those cases what you expect them to be? If so, are we using string comparison functions for the matching in question that don't handle embedded NULs?
Comment on attachment 343767 [details] [diff] [review]
new minimally invasive fix; doesn't quite work
Other than that, the patch looks fine, though, but marking review- because of the failing tests.
Attachment #343767 -
Flags: superreview?(dbaron)
Attachment #343767 -
Flags: superreview-
Attachment #343767 -
Flags: review?(dbaron)
Attachment #343767 -
Flags: review-
Assignee | ||
Comment 43•16 years ago
|
||
It turns out the test failures were just because of errors in the test itself. I had accidentally reused class and ID names, so rules that were meant for other subcases were matching the problem subcases. I did some experiments using JS to check on the attribute values, and they are coming out the way they're supposed to.
Attachment #137905 -
Attachment is obsolete: true
Attachment #138243 -
Attachment is obsolete: true
Attachment #343767 -
Attachment is obsolete: true
Attachment #344334 -
Flags: superreview?(dbaron)
Attachment #344334 -
Flags: review?(dbaron)
Attachment #344334 -
Flags: superreview?(dbaron)
Attachment #344334 -
Flags: superreview+
Attachment #344334 -
Flags: review?(dbaron)
Attachment #344334 -
Flags: review+
Comment on attachment 344334 [details] [diff] [review]
(rev 2) test cases fixed
[Checkin: Comment 45]
ok, r+sr=dbaron
Keywords: checkin-needed
Comment 45•16 years ago
|
||
Comment on attachment 344334 [details] [diff] [review]
(rev 2) test cases fixed
[Checkin: Comment 45]
http://hg.mozilla.org/mozilla-central/rev/9f05f41462ee
Attachment #344334 -
Attachment description: (rev 2) test cases fixed → (rev 2) test cases fixed
[Checkin: Comment 45]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Assignee | ||
Comment 46•16 years ago
|
||
dveditz: shall I prepare patches for older branches? There may be a little adjustment needed.
I suspect to get this in on branches you'd have to prepare patches and request approval; people are unlikely to come looking for you.
Comment 48•16 years ago
|
||
By reading this bug report, I was under the impression this could be used as a hack to specifically target Gecko (like the IE underscore or star hacks).
If anyone thought the same and actually used it on their site, then backporting this patch to branches might be a bad idea.
Assignee | ||
Comment 49•16 years ago
|
||
A couple of casual web searches reveals a number of people asking whether any such hack exists, but nobody (that I saw) describing this as a hack to target Gecko; and in my opinion we should backport the patch, because the possibility of malicious people using the bug to sneak CSS past a filter is of greater concern. I've got a tested 1.9.0.x branch patch now and will have a 1.8.0.x branch patch shortly.
Assignee | ||
Comment 50•16 years ago
|
||
Here's the patch for 1.9.0.x. Beware raw NUL characters in the diff; after applying it you should make sure that the files layout/reftests/bugs/228856-2.html and layout/reftests/bugs/228856-2-ref.html are byte-for-byte identical to the files of the same name that currently exist in mozilla-central. It's probably too late for this to make 1.9.0.4 but I'm flagging it for that release anyway - it *is* a security issue and it's a low-risk patch IMO. 1.9.0.x's reftests pass with the patch applied.
Attachment #345026 -
Flags: approval1.9.0.5?
Attachment #345026 -
Flags: approval1.9.0.4?
Assignee | ||
Comment 51•16 years ago
|
||
Here's the patch for 1.8.1.x. Again, I'm flagging for approval for .18 and .19 despite expecting that this won't make .18, because this is a relatively safe fix for a security issue. There are no reftests on MOZILLA_1_8_BRANCH so this patch, unlike the others, does not include tests. I intended to manually test it, but I got link errors below gfx/ due to some sort of problem with freetype. The modified file itself compiles fine.
I'm assuming this doesn't need fixed in 1.8.0.x (Firefox 1.5) but I'll do it if someone wants.
Attachment #345028 -
Flags: approval1.8.1.19?
Attachment #345028 -
Flags: approval1.8.1.18?
Updated•16 years ago
|
Attachment #345026 -
Flags: approval1.9.0.4?
Updated•16 years ago
|
Attachment #345028 -
Flags: approval1.8.1.18?
Comment 52•16 years ago
|
||
Comment on attachment 345026 [details] [diff] [review]
patch for 1.9.0.x branch
approved for 1.9.0.5 and 1.8.1.19, a=dveditz for release-drivers
(please watch tinderbox for the tree to open before landing)
Attachment #345026 -
Flags: approval1.9.0.5? → approval1.9.0.5+
Updated•16 years ago
|
Attachment #345028 -
Flags: approval1.8.1.19? → approval1.8.1.19+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Whiteboard: [sg:low] xss hazard → [c-n: 1.9.0 and 1.8.1 branches] [sg:low] xss hazard
Comment 53•16 years ago
|
||
Fix checked into 1.8 and 1.9.0 branches
Whiteboard: [c-n: 1.9.0 and 1.8.1 branches] [sg:low] xss hazard → [sg:low] xss hazard
Comment 54•16 years ago
|
||
Verified for 1.9.0 since I see the new tests passing. Is there a good way to verify for 1.8? The manual testcases above don't turn all green in either 1.9.0 or 1.8 (though they turn more green and the results are consistent between 1.9.0 and 1.8).
Keywords: fixed1.9.0.5 → verified1.9.0.5
Assignee | ||
Comment 55•16 years ago
|
||
(In reply to comment #54)
> Verified for 1.9.0 since I see the new tests passing. Is there a good way to
> verify for 1.8? The manual testcases above don't turn all green in either 1.9.0
> or 1.8 (though they turn more green and the results are consistent between
> 1.9.0 and 1.8).
By 'manual testcases' you mean the first four attachments to this bug and the sixth? The behavior as finally agreed upon is not what those expect. You can use the reftests from the m-c patch as manual testcases - http://hg.mozilla.org/mozilla-central/raw-file/9f05f41462ee/layout/reftests/bugs/228856-2.html (should be all green except for headings) and http://hg.mozilla.org/mozilla-central/raw-file/9f05f41462ee/layout/reftests/bugs/228856-1-ref.html (compare to http://hg.mozilla.org/mozilla-central/raw-file/9f05f41462ee/layout/reftests/bugs/228856-1-ref.html )
I don't remember if either of those tests needs other CSS features not available in 1.8, though.
Comment 56•16 years ago
|
||
Thanks, Zack. Those both seem to work and it looks good in 1.8.1.19: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008120103 BonEcho/2.0.0.19pre.
Marking as verified.
Keywords: fixed1.8.1.19 → verified1.8.1.19
Updated•16 years ago
|
Keywords: fixed1.9.1
Updated•16 years ago
|
Alias: CVE-2008-5510
Updated•16 years ago
|
Flags: blocking1.8.0.next+
Updated•16 years ago
|
Attachment #345028 -
Flags: approval1.8.0.next?
Comment 57•16 years ago
|
||
for 1.8.0 we need patch from bug 316394 (and regression patch from 316859)
Comment 58•16 years ago
|
||
Verified on 1.9.1 with builds on OS X and Windows and the mentioned tests from comment 55
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 59•16 years ago
|
||
Comment on attachment 345028 [details] [diff] [review]
patch for 1.8.1.x (MOZILLA_1_8_BRANCH)
a=asac for 1.8.0
Attachment #345028 -
Flags: approval1.8.0.next? → approval1.8.0.next+
Assignee | ||
Comment 60•16 years ago
|
||
(In reply to comment #59)
> (From update of attachment 345028 [details] [diff] [review])
> a=asac for 1.8.0
Do you need me to revise the patch for 1.8.0 or is it good as is?
Comment 62•12 years ago
|
||
The CSS Syntax Level 3 draft defines two points that were undefined in CSS 2.1:
* Any U+0000 NULL character in the CSS source is replaced with U+FFFD REPLACEMENT CHARACTER.
http://dev.w3.org/csswg/css-syntax/#preprocessing-the-input-stream
* Any hexadecimal escape sequence such as \0 that evaluate to zero produce U+FFFD REPLACEMENT CHARACTER rather than U+0000 NULL
http://dev.w3.org/csswg/css-syntax/#consume-an-escaped-character
With this behavior, the result of parsing never contains a NULL character, but anything involving NULL does not produce a know identifier such as a property name.
Does this spec behavior address the security concern of this issue, or should we change the spec to something else?
Flags: needinfo?
Updated•12 years ago
|
Flags: needinfo?
Assignee | ||
Comment 63•12 years ago
|
||
(In reply to Simon Sapin (:SimonSapin) from comment #62)
> The CSS Syntax Level 3 draft defines two points that were undefined in CSS
> 2.1:
>
> * Any U+0000 NULL character in the CSS source is replaced with U+FFFD
> REPLACEMENT CHARACTER.
> http://dev.w3.org/csswg/css-syntax/#preprocessing-the-input-stream
>
> * Any hexadecimal escape sequence such as \0 that evaluate to zero produce
> U+FFFD REPLACEMENT CHARACTER rather than U+0000 NULL
> http://dev.w3.org/csswg/css-syntax/#consume-an-escaped-character
That's not the behavior we implemented here, but I believe it will serve equally well from the security standpoint. The concern was that
.foo { c\0olor: red }
might skate through someone's XSS filter and then get treated as
.foo { color: red }
... upgrading that to a genuine exploit left as an exercise. :)
It's my intention to change Gecko's behavior to match Syn3 once it's CR. In a new bug.
You need to log in
before you can comment on or make changes to this bug.
Description
•