Closed Bug 55731 Opened 24 years ago Closed 24 years ago

JS is run from file types that have no application specified

Categories

(Core :: Security, defect, P2)

Product:
Core
Component:
Security
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: jar, Assigned: security-bugs)

References

Details

(Whiteboard: [rtm++])

Attachments

(6 files)

Patch to prevent unknown file types to be treated as html
556 bytes, patch
Details | Diff | Splinter Review
Revised patch to block local files only
2.27 KB, patch
Details | Diff | Splinter Review
Revised diff to utilize nsXPIDLCString for safer memory management.
1.41 KB, patch
Details | Diff | Splinter Review
Revised patch using hidden pref
3.02 KB, patch
Details | Diff | Splinter Review
Patch that includes all Gagan's suggested optimizations
4.06 KB, patch
Details | Diff | Splinter Review
Same as previous patch but uses strcasecmp instead of strcmp
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
Keywords: rtm
Priority: P3 → P2
Let's fix both issues. I may need some help on these but I'll see they get done.
Status: NEW → ASSIGNED
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.
BTW, if you are indeed talking about fixing it in seamonkey, and you still need 
some help, I'll be glad to pitch in.
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?
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.
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.

*** Bug 14981 has been marked as a duplicate of this bug. ***
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]
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.
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!
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.


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.
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.
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).

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.

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.
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. 
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);

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. 
on that last patch, you'd want to load the preference once per the decoder and
not each time the Determine function is called. 
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
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.
Is it guaranteed that "<FRAMESET" etc will always have the '<' touching the
tagname in these calls to Find?
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
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.
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.
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...
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.
cc'ing rickg & harishd, who may be able to turn off HTML "sniffing" in the
parser. Not sure if that is more appropriate...
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...
Whiteboard: [need info] → [rtm+]
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...
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!
Oops, crossed in the mail. So the only question left is, "is the default setting
correct?"
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).
Yes, Jim, your understanding is correct.  The default setting is to leave the 
behavior unchanged from the current semantics.
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.
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.
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. 
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.
It's already checked into the trunk.
QA Contact: czhang → junruh
rtm++, please checkin ASAP so we can build today.
Whiteboard: [rtm+] → [rtm++]
Patch is now checked in on the branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified on the 11/2 branch Win, Mac and Linux builds.
Testcase is http://junruh.mcom.com/55731.xxx
Keywords: vtrunk
Mass changing QA to ckritzer.
QA Contact: junruh → ckritzer
Marking VERIFIED FIXED per junruh's comments of 2000.11.03.
Status: RESOLVED → VERIFIED
Removing NS_Confidential flag.
Group: netscapeconfidential?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: