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)

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jar, Assigned: security-bugs)

References

Details

(Whiteboard: [rtm++])

Attachments

(6 files)

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: 25 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

Created:
Updated:
Size: