Closed
Bug 55731
Opened 25 years ago
Closed 25 years ago
JS is run from file types that have no application specified
Categories
(Core :: Security, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jar, Assigned: security-bugs)
References
Details
(Whiteboard: [rtm++])
Attachments
(6 files)
556 bytes,
patch
|
Details | Diff | Splinter Review | |
2.27 KB,
patch
|
Details | Diff | Splinter Review | |
1.41 KB,
patch
|
Details | Diff | Splinter Review | |
3.02 KB,
patch
|
Details | Diff | Splinter Review | |
4.06 KB,
patch
|
Details | Diff | Splinter Review | |
4.07 KB,
patch
|
Details | Diff | Splinter Review |
There appear to be two problems:
a) When we don't have an association of a file type with an app, then we almost
load it as HTML (if there is an HTML tag inside). We don't parse and display
the HTML, but we do run JavaScript when we find some!?! We should always have
failsafe approach, where we only process files as HTML/JS if they are on the
approved list (I think). Without this restriction there is an endless game to
get new file types downloaded with trojan contents [:-(] .
b) We again are being burnt by the fact that the attacker can guess where we
will place a file (re: default user directory). At some point we need to put
some more randomization in this placement.. but we never seem to get aronud to
it... and it has burnt us many times.
Bennett Haselton wrote:
At 10:40 PM 10/7/00 -0700, you wrote:
REQUIREMENTS:
1. The attacker must control the news server being used by the user.
2. The attacker must be able to guess the user's profile directory (assumed
to be c:\program files\netscape\users\default )
3. The user must not have ".dat" files associated with any application on
their computer. (Certain multimedia applications associate .dat files with
themselves when they're installed; thinking that .dat files are sound files.)
HOW IT WORKS:
1. The user opens the Netscape newsgroup reader, right-clicks on the name
of their selected news server in the left-hand column, and picks "subscribe
to newsgroups".
2. A new dialog box opens titled "Communicator: Subscribed to newsgroups"
with the message "Receiving newsgroups..." in the status bar at the bottom.
While the list is being downloaded, it is stored in the file
c:\program files\netscape\users\default\News\host-news.uswest.net\hostinfo.dat
3. The news server inserts a newsgroup named
<html><script>alert('hello world');</script></html>
so a line shows up in the hostinfo.dat file like
<html><script>alert('hello world');</script></html>,,1,39d7e561,4b1a
4. The attacker directs the user to a page with a frame that points to
file:///c%7C/program%20files/netscape/users/default/News/host-news.uswest.ne
t/hostinfo.dat
which causes the script to be loaded and executed.
Once you can place JavaScript in a file at a known location on the user's
hard drive where it can be executed, that opens the doors to the usual
attacks that this makes possible: reading local HTML files such as the
user's bookmarks file, reading the settings in their prefs.js file, etc.
HOW TO FIX IT:
After the JavaScript-in-cookies hole was published (which relied on being
able to load a .txt file as an HTML file), a change was made to
Communicator so that files which *already have an application associated
with their extension* could not be loaded as HTML files. So, for example,
if you put
<html><script>alert('hello world');</script></html>
into a .txt file and load that file in Communicator, the JS code won't run.
However, the fix was incomplete, since files that don't already have an
application associated with them, can still be loaded as HTML files
provided that you have <HTML> and </HTML> tags somewhere in them. That's
why hostinfo.dat can be loaded as an HTML file. (That's also why this
exploit fails if .dat files are already associated with an application on
your machine -- Communicator will launch the associated application instead
of loading the .dat file into the frame.)
So, to close the hole, ensure that Communicator cannot load files as HTML
files even in the case where the file type has no application associated
with it.
-Bennett
bennett@peacefire.org http://www.peacefire.org
(425) 649 9024
Assignee | ||
Comment 1•25 years ago
|
||
Let's fix both issues. I may need some help on these but I'll see they get done.
Status: NEW → ASSIGNED
Comment 2•25 years ago
|
||
mitch, are you talking about fixing this in communicator or in seamonkey? The
bug report talks about the problem in communicator so it's not clear why this
was filed as a bugzilla bug. Presumably the same problem exists in seamonkey so
this report is really talking about the seamonkey problem and fix even though it
keeps mentioning communicator.
Just wanted to clarify this. Correct me if I'm wrong.
Comment 3•25 years ago
|
||
BTW, if you are indeed talking about fixing it in seamonkey, and you still need
some help, I'll be glad to pitch in.
Comment 4•25 years ago
|
||
Bug definitely exists in seamonkey as I just confirmed with the obvious simple
test program -- namely storing the following into a file with an unrecognized
suffix (e.g., .xxx)
<html>
<head>
<script>
alert ("running javascript");
</script>
</head>
<body>
This is a <b> test </b> of parsing html
</body>
</html>
However, I don't understand Jim's comment about "we don't parse and display the
html". In the above example, the word "test" came up in boldface so wouldn't
that imply parsing of html?
Comment 5•25 years ago
|
||
Regarding the second part of this bug report (using random names), the code for
doing something similar already exists for the wallet and single-signon
database. So it wouldn't be difficult to re-use part of that coding for the
current problem.
Comment 6•25 years ago
|
||
BTW, take a look at bug 14981. Turns out I already reported this problem and
then somehow got fooled into thinking was fixed (turned into a works-for-me).
So now it is broken again, unless maybe the works-for-me was inadvertently done
on files for which applications were registered.
Comment 8•25 years ago
|
||
This _seems_ like a good thing to get for the RTM. Is it actually a limited
case? Steve Morse, any chance you could continue to help out with this one?
Whiteboard: [need info]
Comment 9•25 years ago
|
||
And have it ready by Monday morning? Probably not a chance. Give me a
realistic drop-dead date on this and I'll see what I can do.
Comment 10•25 years ago
|
||
Actually it wasn't as difficult to find the place to make this change as I
thought. And it's a one-line change -- simply commenting out the line in the
unknownDecoder that is setting the content type to html if the file contains
anything that smells like html. In fact, the code that is currently doing that
is preceded by the comment:
// If the buffer contains "common" HTML tags then lets call it HTML :-)
Note the smiley face in the comment!
Comment 11•25 years ago
|
||
Comment 12•25 years ago
|
||
Hey Steve, unfortunately this isn't the right fix for this bug. You'll introduce
some pretty big regressions on bugs which require the ability to sniff HTML
content when we don't know the content type for the file. Just to give you one
example, users will not be able to login to myEbay and www.techfed.com.
I don't think this is the right way to be trying to fix this problem due to the
amount of functionality we'll break.
Comment 13•25 years ago
|
||
I agree and had concerns about that myself. After speaking with mscott, we both
agreed that the correct fix would be to not consider it to be html only if the
file is on the local disk. Am currently working up a patch for that.
Comment 14•25 years ago
|
||
Comment 15•25 years ago
|
||
mscott wanted me to point out that we will be losing some functionality --
namely not allowing users to execute html files on their hard disk if the files
are not specifically of type html. So we want the reporter (jar in this case)
and pdt (selmer in this case) to be aware of this consequence if they really
want this possible attack to be blocked.
Just spoke with judson to get his approval as module owner and he made the same
point.
Comment 16•25 years ago
|
||
Comment 17•25 years ago
|
||
Morse's patch (revised by me) will prevent the unknown content type decoder from
detecting HTML files on disk.
If we check this in we'll need to re-open (among others that I didn't find):
http://bugzilla.mozilla.org/show_bug.cgi?id=44627
http://bugzilla.mozilla.org/show_bug.cgi?id=29236
The above bugs were filed because people need to be able to open HTML files from
disk if they lack .html or .htm extensions.
I've been crucified over these kinds of bugs in the past. They've been marked
blockers on me in fact.
r=valeski on the code that steve has provided, however I don't recommend that we
check it in on the grounds of breaking 4.x backwards compat, and the nuiscence
that it will create. I recommend we attack this from another angle (perhaps
ensuring, on a laborious case by case basis, that we don't allow certain files
we use on disk, to be opened as html (perhaps forcing a TEXT_PLAIN type on them
up front).
Comment 18•25 years ago
|
||
I just spoke with rpotts and he authorized me to add the following to the bug
report:
r=rpotts
a=rpotts (as module owner)
Have spoken with mscott and he will be doing a super-review on this.
So now the question comes down to whether pdt really wants to add this
belt-and-suspenders protection against an attack, realizing that the trade-off
is to introduce some backwards incompatibility with 4.x.
Comment 19•25 years ago
|
||
Just spoke with jar about this issue and he's now in favor of having this fix
under control of a hidden pref which is off by default. This way we are not
losing and backwards compatibility now, but if we discover an attack later we
can turn the pref on (which can be done with signed javascript).
So I'll revise the patch to put it under control of a hidden pref.
Comment 20•25 years ago
|
||
Comment 21•25 years ago
|
||
Just catching up on this bug now (per morse's request) Few comments--
This is possibly the best fix we can do for now. As for the bugs mentioned by
Jud, the impact would be minimal since this filll only affect local file cases.
As for the review-- I'd suggest you use PL_strncasecmp to make sure we don't
trip over a mixed-case of file:// which would be a bad loophole too.
Comment 22•25 years ago
|
||
The latest patch (above) includes valeski's suggestion to use nsXPIDLCString. It
also includes jar's suggestion to use a hidden pref.
The hidden pref is security.requireHTMLsuffix and it is set to false by default.
To enable this feature, add the following line to your prefs.js file:
user_pref("security.requireHTMLsuffix", true);
Comment 23•25 years ago
|
||
Another review note from performance perspective: you'd want to move the check
for file:// urls before you actually search for the HTML ,TITLE, etc. Thus you
don't waste time searching for HTML hints if it were a file URL in the first
place.
Comment 24•25 years ago
|
||
on that last patch, you'd want to load the preference once per the decoder and
not each time the Determine function is called.
Comment 25•25 years ago
|
||
Comment 26•25 years ago
|
||
seems ok except that this doesn't have the case compare check that I'd
suggested. if this includes the change from PL_strcmp to PL_strcasecmp, then
r=gagan
Comment 27•25 years ago
|
||
Oops, I added that strcasecmp in but then when doing all the cut-and-pasting to
restructure for the optimizations you requested, I accidentally lost it.
Posting new patch which simply has strcasecmp instead of strcmp.
Comment 28•25 years ago
|
||
Comment 29•25 years ago
|
||
Is it guaranteed that "<FRAMESET" etc will always have the '<' touching the
tagname in these calls to Find?
Comment 30•25 years ago
|
||
I don't really expect this to be a robust check for HTML, and in the worst case
we'd drop to not detecting this as HTML which isn't so bad considering it
doesn't end on a valid extension and has bad HTML. So its ok if the <FRAMESET>
(or any of the HTML check tags) don't match. We'd still be safe as far as this
bug goes. morse: with that last patch r=gagan
Comment 31•25 years ago
|
||
I don't think it's valid html otherwise. I just tried the following test on 4.x
(note that there is a space inside the <b> tag):
<html> <body> This is a < b> test </b> </ body> </html>
and the word "test" did not appear in boldface.
Comment 32•25 years ago
|
||
gagan, I think you misunderstood selmer's question. He wasn't asking about
mismatched tags. Rather it was a tag with a space between the "<" and the first
character of the tag name.
In any case, if this is valid html, then it's a separate bug that always
existed. I didn't change the logic in that part of the code.
Comment 33•25 years ago
|
||
right I don't think selmer's comment applied to this bug but he was wondering
about our detection code in general. That should be another bug although I don't
think it's a big deal.....
I'm going to go ahead and give this an sr=mscott. Feel free to check this into
the branch now to get some early test coverage.
I'd like Mitch to confirm that he thinks this is the right thing to do as well
before we make any final decisions though...
Comment 34•25 years ago
|
||
Scott, I assume that was a typo and you meant to say "feel free to check this
into the TRUNK now to get some early coverage." So that's what I'll do.
Comment 35•25 years ago
|
||
cc'ing rickg & harishd, who may be able to turn off HTML "sniffing" in the
parser. Not sure if that is more appropriate...
Comment 36•25 years ago
|
||
It also troubles me that we're doing several strcasecmp()'s through the
buffer...that's an awful lotta passes through what could end up being quite a
deal of data. Parser guys, tell me there's a better way...
Updated•25 years ago
|
Whiteboard: [need info] → [rtm+]
Comment 37•25 years ago
|
||
Hey Chris, I don't think you need to be so worried about the passes we make
searching for some HTML tags. Keep in mind that the unkown content sniffer only
extracts and performs this search on the first 256 bytes in the data stream.
Sure, you could optimize it and make it a little faster but I think it's a case
of, "is this really a hot spot?" I don't think it is...
Comment 38•25 years ago
|
||
Ok, ignore my previous comments. mscott set me straight. Just calm two of my fears:
1. The buffer that we grovel through five times is small, right?
2. By default, we run in "insecure" mode, because mRequireHTMLsuffix is
defaulted to PR_FALSE in the ctor. Is this correct?
thanks!
Comment 39•25 years ago
|
||
Oops, crossed in the mail. So the only question left is, "is the default setting
correct?"
Reporter | ||
Comment 40•25 years ago
|
||
As I understand this (please correct me if I'm wrong) we now have a change that
is controlled by a hidden pref. The default behaviour is unchanged from present
semantics. We would (by taking this patch) have the option of telling users to
change the pref (or providing a signed JS that does the deed on their behalf).
This is the argument that will be used in a discussion at the 11:30am PDT
meeting on Monday, when trying to decide to land with the limbo slew or not.
I believe Chris's recent question was a request for assurance that the default
pref leaves semantics unchanged (and that it was intentional). I believe the
issue is that a change in default semantics at this late date would be too
risky. The best we can do is put in the option to change the semantics after a
ship (if we convince the user it is worth it when faced with whatever attacks
might appear).
Comment 41•25 years ago
|
||
Yes, Jim, your understanding is correct. The default setting is to leave the
behavior unchanged from the current semantics.
Comment 42•25 years ago
|
||
I have 2 problems with the patch: First, morse is duplicating a capability
already working in the HTML parsing engine. Second, based on the patch I've seen
here, he'll report false positives, to wit:
/* <HTMLookAndFeel is <Headline news */
Neither is a real tag, and both are in what amounts to a comment.
Comment 43•25 years ago
|
||
Rick, your raising the wrong issues here. The things you are commenting on
always existed and I didn't change them. The patch I'm including supresses
doing the things you don't like in the particular case that the url is from the
local disk.
If you feel strongly about the two issues you just raised, then you should open
separate bug reports on them. But let's not allow that to distract us from the
problem that this patch is fixing.
Comment 44•25 years ago
|
||
I see -- so the act of pointing out problems in code that is up for review is
being described as wrong. Hmm. Perhaps I really don't understand.
My example shows how the code I reviewed won't work. If the code is necessary,
then it should at *least* work correctly. I don't care who wrote it per se, but
whoever feels ownership for this stuff should feel compelled to get the right
answer.
Comment 45•25 years ago
|
||
This bug is in candidate limbo. We will reconsider this fix once we have a
candidate in hand, but we can't take this fix before then. Please check into
the trunk ASAP.
Comment 46•25 years ago
|
||
It's already checked into the trunk.
Updated•25 years ago
|
QA Contact: czhang → junruh
Comment 47•25 years ago
|
||
rtm++, please checkin ASAP so we can build today.
Whiteboard: [rtm+] → [rtm++]
Comment 48•25 years ago
|
||
Patch is now checked in on the branch.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Comment 49•25 years ago
|
||
Verified on the 11/2 branch Win, Mac and Linux builds.
Testcase is http://junruh.mcom.com/55731.xxx
Keywords: vtrunk
Comment 51•25 years ago
|
||
Marking VERIFIED FIXED per junruh's comments of 2000.11.03.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•