Last Comment Bug 162409 - Can trick layout history into filling <input type=file> with text from <input type=text> using key collision
: Can trick layout history into filling <input type=file> with text from <input...
[adt2 RTM] [FIX]
: csectype-disclosure, sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 2000
: -- major (vote)
: mozilla1.0.1
Assigned To: John Keiser (jkeiser)
: bsharma
: David Keeler [:keeler] (use needinfo?)
Depends on:
Blocks: 143047
  Show dependency treegraph
Reported: 2002-08-12 19:10 PDT by Jesse Ruderman
Modified: 2013-06-09 22:15 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

lame demo (1.38 KB, text/html)
2002-08-12 19:11 PDT, Jesse Ruderman
no flags Details
better demo: uploads c:\test.txt (1.31 KB, text/html)
2002-08-12 22:57 PDT, Jesse Ruderman
no flags Details
Patch (1.83 KB, patch)
2002-08-13 01:06 PDT, John Keiser (jkeiser)
no flags Details | Diff | Splinter Review
Patch -u (2.99 KB, patch)
2002-08-13 01:08 PDT, John Keiser (jkeiser)
jonas: review+
john: superreview+
john: approval+
Details | Diff | Splinter Review
Linux-friendly testcase (1.51 KB, text/html)
2002-08-13 19:13 PDT, John Keiser (jkeiser)
no flags Details

Description Jesse Ruderman 2002-08-12 19:10:35 PDT
Layout history uses keys that look like this to refill forms in session history:

InputName and FormName can contain the > character, so it is possible to trick
layout history into filling the contents of a text field into a file-upload
field.  This allows an attacker to upload a file if he knows its filename.
Comment 1 Jesse Ruderman 2002-08-12 19:11:49 PDT
Created attachment 95035 [details]
lame demo

I'm working on a better demo.
Comment 2 Jesse Ruderman 2002-08-12 22:57:19 PDT
Created attachment 95055 [details]
better demo: uploads c:\test.txt

Thanks to jesus_X and grok for writing the server side of this demo.
Comment 3 John Keiser (jkeiser) 2002-08-13 01:06:18 PDT
Created attachment 95073 [details] [diff] [review]

This fixes the problem by putting all developer-controlled elements *after*
non-developer-controlled elements like type in the key.  Because the key is
used only to match, if the first part (containing the type) does not match the
entire key cannot match.  And since none of the things inserted can be
controlled directly by the developer, there is no chance of messing up that
crucial part. 

This has been tested against (to
ensure no regressions), Bugzilla bug entry and attachment pages, and the
enclosed testcase.
Comment 4 John Keiser (jkeiser) 2002-08-13 01:08:05 PDT
Created attachment 95074 [details] [diff] [review]
Patch -u

Universal diff of previous patch.
Comment 5 John Keiser (jkeiser) 2002-08-13 01:10:26 PDT
sicking, bz, reviews?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2002-08-13 01:26:20 PDT
Comment on attachment 95074 [details] [diff] [review]
Patch -u

Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2002-08-13 01:26:34 PDT
Comment on attachment 95074 [details] [diff] [review]
Patch -u

Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2002-08-13 01:27:58 PDT
It might be worth adding some comment to make sure that noone changes this
again. IE something about that author-data should be last so that it can't
tamper with the type.
Comment 9 John Keiser (jkeiser) 2002-08-13 01:33:02 PDT
I actually considered that but figured people watching the checkin would notice;
it would be nice to make this checkin seem as benign as humanly possible given
the horrifying scope of the exploit.
Comment 10 Jaime Rodriguez, Jr. 2002-08-13 08:05:24 PDT
Marking as ADT2 RTM, until we can discuss the severity and discoverability of
this one with jesse and mitch.
Comment 11 John Keiser (jkeiser) 2002-08-13 10:24:37 PDT
Fix checked in to trunk.  Leaving open until we decide what else to do with it.
Comment 12 John Keiser (jkeiser) 2002-08-13 17:03:31 PDT
Comment on attachment 95074 [details] [diff] [review]
Patch -u

a=asa for 1.0.x (asa gave approval just now)
Comment 13 John Keiser (jkeiser) 2002-08-13 17:07:43 PDT
Checked in on 1.1 branch.
Comment 14 John Keiser (jkeiser) 2002-08-13 19:13:07 PDT
Created attachment 95199 [details]
Linux-friendly testcase
Comment 15 Christine Hoffman 2002-08-13 19:46:21 PDT
Tested on Linux with the following:

1. Netscape 7.0 PR1
2. Patch build based on 8/13 trunk build

Using the testcase created in comment 14, clicked on the button 'Make me upload

In Netscape 7.0PR1 contents of file were returned.
With patch build, contents of file were NOT returned.

Comment 16 scottputterman 2002-08-13 19:49:48 PDT
adding adt1.0.1+.  Please check this in tonight.
Comment 17 Jaime Rodriguez, Jr. 2002-08-14 05:09:50 PDT
jkeiser: Did we not check this into the 1.0 branch? Ia sk because there is no
"fixed1.0.1" marking in the keywords, nor did I see a comment saying this was
checked into the 1.0 branch, after Scott provided ADT approval. Pls advise ...
Comment 18 scottputterman 2002-08-14 09:24:03 PDT
According to bonsai, this was checked in last night, so marking fixed1.0.1
Comment 19 John Keiser (jkeiser) 2002-08-14 11:55:19 PDT
Yes, sorry, things got hectic last night.  It should be in this morning's builds.

This is all the builds I am concerned with, resolving fixed.

We need to notify the embedders we know about so that they can pick up this fix.
Comment 20 Jaime Rodriguez, Jr. 2002-08-14 13:57:01 PDT
bsharma: bindu, pls verify this as fixed on the 1.0 branch and the trunk.
Comment 21 bsharma 2002-08-14 14:09:57 PDT
Verified on 2002-08-14-trunk build on Win 2000.

Loaded test cases in comment #1, #2, #3.

All of them give either error or exception in the JS console.
Comment 22 bsharma 2002-08-15 15:16:53 PDT
Verified on 2002-08-15-branch build on Win 2000.

Loaded comment #1, #2 and #3 and all of them gave the appropriate error messages.

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