Comment on attachment 160234 [details] Test case This was originally set as text/html. Sorry. :)
Attachment #160234 - Attachment mime type: text/html → text/plain
not sure this bug should be marked security closed... I'm I missing something?
15 years ago
Attachment #160234 - Attachment mime type: text/plain → text/html
(In reply to comment #3) > not sure this bug should be marked security closed... I'm I missing something? My reasoning was that since this bug is triggered by a string of a particular size (0) maybe the code isn't robust enough to cope with some other strings. Since it seemed plausible to me that such a vulnerability could also be used to perform a buffer overflow attack I've marked the bug as security-sensitive. Of course, I hope that I'm wrong and someone quickly finds that an empty string is the only case where the code fails and removes the security-sensitive flag.
Confirming. I've spent some time poking at this and it's crashing on a null dereference. Removing security-sensitive flag: an empty rule won't lead to a buffer overflow, and non-empty illegal strings are handled OK. Not nice to be able to crash someone at will, of course. Crashing at line 2680 in nsCSSStyleSheet.cpp (aviary branch) dereferencing firstRule. The obvious local fix would be an if (firstRule) check like the if (nextRule) above, but I defer to bz in case he wants to fix this higher on the food chain (this section last touched Sept 2001 fixing bug 93977 and bug 98358)
Assignee: general → dveditz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 160375 [details] [diff] [review] Aviary patch This fixes the crash, but as mentioned earlier I don't know if this should have been caught higher up. The patch also includes a somewhat unrelated change from NS_ENSURE_SUCCESS() to NS_ENSURE_TRUE() on inserting the rules. _SUCCESS() is a noop here, if the check is useful (for performance?) we should make this change, otherwise drop it. "unrelated" because in the case of this bug if we avoid the crash we successfully insert 0 elements and call DidDirty() anyway.
(In reply to comment #5) > but I defer to bz in case he wants to fix this higher on the > food chain I think I do, yes. Patch coming up....
Created attachment 160435 [details] [diff] [review] Patch
Attachment #160375 - Attachment is obsolete: true
Comment on attachment 160375 [details] [diff] [review] Aviary patch Marking r-, since this isn't quite the best fix (and misses similar code for rule groups).
Created attachment 160436 [details] [diff] [review] Patch with the InsertElementsAt() change that dveditz caught
Attachment #160435 - Attachment is obsolete: true
15 years ago
Assignee: dveditz → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha5
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 160436 [details] [diff] [review] Patch with the InsertElementsAt() change that dveditz caught This is a low-risk crash fix well-worth taking on the branches, in my opinion.
Comment on attachment 160436 [details] [diff] [review] Patch with the InsertElementsAt() change that dveditz caught a=dveditz for 1.7 branch
Attachment #160436 - Flags: approval1.7.x? → approval1.7.x+
Comment on attachment 160436 [details] [diff] [review] Patch with the InsertElementsAt() change that dveditz caught a=asa for aviary checkin.
Attachment #160436 - Flags: approval-aviary? → approval-aviary+
Keywords: fixed-aviary1.0, fixed1.7.x
You need to log in before you can comment on or make changes to this bug.