Last Comment Bug 394534 - [FIX]First attribute doesn't win in misplaced content
: [FIX]First attribute doesn't win in misplaced content
Status: RESOLVED FIXED
baking on trunk (9/10)
: fixed1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: x86 Linux
: P1 critical (vote)
: mozilla1.9alpha8
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
http://www.criticalsecurity.net/index...
Depends on:
Blocks: xss
  Show dependency treegraph
 
Reported: 2007-08-31 19:47 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2007-10-04 16:31 PDT (History)
12 users (show)
jst: blocking1.9+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (7.39 KB, patch)
2007-08-31 20:51 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
peterv: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
jonas: approval1.9+
Details | Diff | Splinter Review
Merged to branch (5.92 KB, patch)
2007-09-25 09:35 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2007-08-31 19:47:08 PDT
Problem is also mentioned at http://sla.ckers.org/forum/read.php?2,15336 and http://ha.ckers.org/blog/20070830/overwriting-attributes/

Fix is coming up.  Writing tests now.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2007-08-31 20:51:06 PDT
Created attachment 279208 [details] [diff] [review]
Fix

Makes sure to not reverse the order of tokens when moving them to and from the misplaced content nsDeque.
Comment 2 Blake Kaplan (:mrbkap) 2007-09-01 13:52:28 PDT
Comment on attachment 279208 [details] [diff] [review]
Fix

Index: parser/htmlparser/src/CNavDTD.cpp
+          // XXXbz would it be faster to get the tokens out with ObjectAt and
+          // the PopFront them all?

"then"
Comment 3 Peter Van der Beken [:peterv] 2007-09-02 06:42:08 PDT
Comment on attachment 279208 [details] [diff] [review]
Fix

>Index: parser/htmlparser/src/CNavDTD.cpp
>===================================================================

>+          // XXXbz would it be faster to get the tokens out with ObjectAt and
>+          // the PopFront them all?

then PushFront ...?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2007-09-02 08:03:19 PDT
Comment on attachment 279208 [details] [diff] [review]
Fix

No, "then PopFront".  Maybe I should say "then PopFront them all from mMisplacedContent".

Requesting approval for trunk and branches.  This is bug apparently being used for content-injection attacks...  Risk is as low as it gets in parser code, which is probably "medium to large", so we might want to bake on the trunk for a week or so before landing on the branches.
Comment 5 Peter Van der Beken [:peterv] 2007-09-02 08:17:07 PDT
(In reply to comment #4)
> (From update of attachment 279208 [details] [diff] [review])
> No, "then PopFront".  Maybe I should say "then PopFront them all from
> mMisplacedContent".

Ah, and use PushTokenFront again? (which is what I meant instead of PushFront)
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2007-09-02 08:25:38 PDT
Right.  That approach would be to ObjectAt() and PushTokenFront() starting at the _end_ of the things I want to move, then pop them all off.
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-09-02 15:20:46 PDT
You might want to check if this piece of html:
<table><input type="password">
crashes with the patch.
It seems to crash in my debug build, but I might have done something wrong.

It seems to crash at:
    CToken* theAttrToken = theAttrNode.PopAttributeTokenFront();
in PushMisplacedAttributes
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2007-09-02 16:35:38 PDT
Doesn't seem to crash over here....
Comment 9 Daniel Veditz [:dveditz] 2007-09-06 17:15:16 PDT
Would be nice to fix on the branch, but probably not a stop-ship. We'll look at approvals after it's landed on the trunk.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2007-09-10 16:51:39 PDT
Fix landed on the trunk (by manually applying the change to reftest.list as that part of the patch had bit-rotted). Marking FIXED.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-09-11 09:42:28 PDT
It looks like the test file itself didn't land, so I reverted the reftest.list change to fix the orange.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-09-11 11:58:38 PDT
I checked the test in now that I was able to verify that it passed.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2007-09-11 13:07:16 PDT
Duh, must have forgotten to cvs add that file. Thanks for dealing with it!
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2007-09-12 09:13:18 PDT
Checked in a followup patch addressing the review comments too.
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-09-14 06:04:41 PDT
Apparently there is something wrong with my vc build, so you can just ignore comment 7 any further (which you already did).
Comment 16 Daniel Veditz [:dveditz] 2007-09-24 12:25:33 PDT
Comment on attachment 279208 [details] [diff] [review]
Fix

approved for 1.8.1.8 and 1.8.0.14, a=dveditz for release-drivers
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2007-09-25 09:35:38 PDT
Created attachment 282270 [details] [diff] [review]
Merged to branch
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2007-09-25 09:36:48 PDT
Checked in on both branches.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-04 16:31:55 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

I now get green text and a checkbox input, while I got red text and a text input with Firefox2.0.0.7, testing:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/394534-1.html

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