Closed Bug 877500 Opened 7 years ago Closed 6 years ago

0xFF in MIME type (in Content-Type header) causes crash

Categories

(Core :: Networking, defect, critical)

23 Branch
All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox24 --- fixed
firefox-esr17 --- wontfix
b2g18 --- unaffected

People

(Reporter: GPHemsley, Assigned: smichaud)

References

(Blocks 1 open bug, )

Details

(Keywords: crash, csectype-dos, Whiteboard: [adv-main24-])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-73bdb211-d239-4dce-91e9-ae6e22130530 .
============================================================= 

I'm in the process of testing my "parse a MIME type" algorithm from the MIME Sniffing Standard [1], so I decided to test what happens when a 0xFF byte occurs in the MIME type. It turned it to be worse than I expected (and I already wasn't expecting much).

The link in the URL field is to a script that I've been using in my testing. It puts the value of the 'ct' query parameter into a Content-Type header.

[1] http://mimesniff.spec.whatwg.org/#parse-a-mime-type
It only seems to happen when it is in the actual MIME type portion itself (i.e. not after a whitespace character or after the semicolon).

In other words, only up to step 13 in the algorithm as it currently stands. It's fine after that—at least, in the limited cases that I tried. (Which is not to say that the current Gecko implementation actually follows this algorithm; that's another bug that I haven't filed yet.)
Hi gordon - thanks for filing this. The crash you reported is actually in os specific code. (its trying to launch an external handler for that mime type). Would it be easy for you to test on windows and report if the issue is cross platform?
Using Firefox 21 on the Windows XP virtual machine provided by Microsoft, the file prompts for download (i.e. is treated as an unknown MIME type that can't be handled).
Crash Address: 0x7fff8a0bd988
Group: core-security
On the other hand,

Crash Reason	EXC_BREAKPOINT / 0x00000002

And in the native Mac crash report:

Application Specific Information:
*** CFRelease() called with NULL ***
linux also tests fine.
Josh - this looks like its in some old os x code you touched.. do you want to take it?
I'll look at this next week ... unless Josh beats me to it.

I tried the testcase (http://whatwg.gphemsley.org/tests/mimesniff/sniffing.php?ct=text/xml%FF;charset=ISO-8859-5) in FF 21.0 and it "worked" -- I crashed:

bp-7cb55ff6-3439-4057-914b-345392130530
(In reply to Steven Michaud from comment #8)
> I'll look at this next week ... unless Josh beats me to it.
> 
> I tried the testcase
> (http://whatwg.gphemsley.org/tests/mimesniff/sniffing.php?ct=text/xml%FF;
> charset=ISO-8859-5) in FF 21.0 and it "worked" -- I crashed:
> 
> bp-7cb55ff6-3439-4057-914b-345392130530

FWIW, my inspection of the source suggests this bug to have been present since well before Firefox 12. (Bug 675356 churned the code, but I don't think it affected its execution.)
Steven said he'd look at this.

Do we have any idea what the security rating for this should be?
Assignee: nobody → smichaud
(In reply to Patrick McManus [:mcmanus] from comment #2)
> The crash you reported is actually in os specific code (its trying
> to launch an external handler for that mime type).

Actually looks like we're passing a bogus value (always NULL?) to ::CFRelease() in our own code. That's probably our problem, not the OS's (did we miss an error check?).

http://hg.mozilla.org/releases/mozilla-release/annotate/30ec6828d10e/uriloader/exthandler/mac/nsOSHelperAppService.mm#l528
Keywords: csec-dos
hey dan, my comment meant that nsOSHelperAppService.mm is specific code to one OS.. not that the code in question is part of the OS distribution.

I wrote that just to make sure the test case also was run on windows and linux because they were going to execute different code and could conceivably of had a cut and paste problem that needed to be patched separately. (and that was done in comments 6 and 3.. those platforms are fine.).
I hope to get to this today or tomorrow -- or in any case later this week.
I no longer crash with this bug's testcase (http://whatwg.gphemsley.org/tests/mimesniff/sniffing.php?ct=text/xml%FF;charset=ISO-8859-5), even in FF 21 (which crashed previously).  Instead I'm asked to save the testcase as a file.

Has the testcase been changed?
What's changed is that I just upgraded to OS X 10.8.4.

I'd bet that this is an Apple bug, that Apple's fixed in 10.8.4.

I'll test on other versions of OS X, including 10.8.3 (which I still have somewhere).
No, I don't crash on my single remaining 10.8.3 partition, either (using FF 21.0).
I can't crash on OS X 10.7.5 or 10.6.8, either (still using FF 21.0).

I'm now beginning to suspect that there was something about my profile that triggered the crashes on the one machine I tested on, which has now changed :-(

I don't crash with a fresh profile.
From the evidence of our two Socorro crash stacks, it's quite clear this bug's crashes *don't* happen calling CFRelease() on a NULL object.  Instead the object that CFRelease is called on appears to be invalid.

(The crash addresses indicate this.  And CFRelease() deliberately kills the process when it's called on a NULL parameter -- which clearly isn't happening here.)

Problem is, the cfType object (which CFRelease() crashes while releasing) is only alive for a very short time, and it's quite clear from the code that we don't do anything to invalidate it.  So it's likely that this is an Apple bug, after all.

And that we won't be able to do much about it until we can once more reproduce it.
Gordon, can you still reproduce these crashes using FF 21 (on OS X 10.6.8 or any other version of OS X)?
Flags: needinfo?(gphemsley)
(In reply to comment #5)

> Application Specific Information:
> *** CFRelease() called with NULL ***

Where did you find this, Jesse?

I don't see it in either of the Socorro crash logs reported here:

bp-73bdb211-d239-4dce-91e9-ae6e22130530
bp-7cb55ff6-3439-4057-914b-345392130530
Flags: needinfo?(jruderman)
(In reply to Steven Michaud from comment #19)
> Gordon, can you still reproduce these crashes using FF 21 (on OS X 10.6.8 or
> any other version of OS X)?

I was (and am) using Aurora 23 on Mac OS X 10.6.8, but no, I can no longer reproduce. I guess the most recent Apple security update fixed it?
Flags: needinfo?(gphemsley)
Another couple of questions for you, Gordon:

Has your sniffing.php file changed since 2013-05-30 (when I was last able to reproduce this bug's crashes)?

Has your testing environment (on the machine from which you're serving sniffing.php) changed?

And once again, can you still reproduce this bug's crashes with this bug's testcase (http://whatwg.gphemsley.org/tests/mimesniff/sniffing.php?ct=text/xml%FF;charset=ISO-8859-5)?  Now that I've seen your latest comment, I see the answer to this question is "no", as I expected.
> I guess the most recent Apple security update fixed it?

I think it's much more likely that you changed your sniffing.php file or your testing environment in some way.
Gordon:

I used wget to download your (current) sniffing.php file and placed a copy on my own (private) server that understands PHP.

Appending "?ct=text/xml%FF;charset=ISO-8859-5" to its URL doesn't trigger the crashes, either.  But instead of prompting for download it just loads the file into the current page.

Most of the contents is gibberish -- which is a little odd, since the original file is perfectly valid, though in Japanese.  Most interesting, though, is that the following appears at the end of the page:

text/html
undefined

This seems to indicate that the "?ct=..." stuff was ignored by my server.
Gordon:

Software Update should tell you what updates you've installed, and when.  Have you installed any Apple updates since 2013-05-30?
> Have you installed any Apple updates since 2013-05-30?

I should have asked:

Have you installed any Apple updates since you last were able to reproduce these crashes?
The code that crashed is no longer being exercised by this bug's testcase.  I'm going to try to figure out why.
(In reply to Steven Michaud from comment #20)
> (In reply to comment #5)
> 
> > Application Specific Information:
> > *** CFRelease() called with NULL ***
> 
> Where did you find this, Jesse?
> 
> I don't see it in either of the Socorro crash logs reported here:
> 
> bp-73bdb211-d239-4dce-91e9-ae6e22130530
> bp-7cb55ff6-3439-4057-914b-345392130530

It was in the native crash report for a local debug build, in ~/Library/Logs/DiagnosticReports.
Flags: needinfo?(jruderman)
OK, lots of questions here, and they kinda overlap.

First off, yes, I have installed updates to my system since May 30th, which is why I suggested it. In particular, Security Update 2013-002: http://support.apple.com/kb/HT5784

As for the testcase, it has changed since then (including the addition of the document.contentType/mimeType stuff today). I don't know what changes were made when, but this script just passes the 'ct' variable straight into a 'Content-Type' header. (Which is why ISO-8859-5 turns UTF-8 Japanese into gibberish.)

But it turns out that it did stop crashing because of changes in the testcase itself. I must have added a 'Content-Disposition' header after May 30th; removing it (which I've done) crashes this again in Nightly 24:

bp-7a7bb4c7-ad9c-4313-8ddb-6154d2130613
For reference, this was the Content-Disposition header being sent before that stopped it from crashing:

Content-Disposition: inline; filename="sniffing"
I'll continue work on this tomorrow.
I was sure CFRelease() wasn't being called with a NULL parameter here.  I was wrong.

On all versions of OS X that I checked (going back to 10.6.8), CFRelease() calls "kill(getpid(), SIGKILL)" whenever it's called with a NULL parameter, and then continues as if nothing bad had happened.

The process gets killed very quickly, of course.  But I assumed Breakpad would not be invoked -- which is incorrect.  It's not possible to "catch" a SIGKILL, so I'm not sure how Breakpad manages this, but nonetheless it does.  (It's possible to send a signal that *does* prevent Breakpad from running.  As recently as a few years ago, the Java plugin did this under certain circumstances.  My mistake was to assume this is true for all signals.)

I wrote an interpose library to test Firefox's behavior when CFRelease is called on a NULL parameter.  Breakpad gets invoked even in FF 4.0.1.  So this behavior is nothing new.

What should (I think) have given the game away is the following:

Crash Reason    EXC_BREAKPOINT / 0x00000002

But I'm not completely sure, since I'm not able to get Socorro to search by "Crash reason", even though it's supposed to be able to do that.
(In reply to comment #30)

> Content-Disposition: inline; filename="sniffing"

Gordon, could you try putting this header back and making the filename "sniffing.php"?  I'll bet you crash with that (since I think it will exercise the code that crashes).

I have a fix for this bug -- just NULL-check the return value of CFStringCreateWithCString() (which we actually already are doing further up in GetMIMEInfoFromOS()).  But I want to make sure there isn't something else wrong in the code.

If the file's name is "sniffing.php", GetMIMEInfoFromOS() gets called with aMIMEType set to your testcase's bad MIME type and aFileExt set to "php".  GetMIMEInfoFromOS() correctly fails to find a handler for the bad MIME type.  But it *could* legitimately find a handler for the "php" extension (on my system it finds TextWrangler, which is a text editor).  If that happens the code that crashes will be exercised.
Flags: needinfo?(gphemsley)
Attached patch FixSplinter Review
Attachment #762892 - Flags: review?(joshmoz)
Attachment #762892 - Flags: review?(joshmoz) → review+
https://hg.mozilla.org/mozilla-central/rev/7352b0e058d6

Can we get a test for this?
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
thanks steven!
Attached patch TestSplinter Review
Here's a test for this bug.

It was a bit of a challenge to write, and to edit the text file that contains the test.

I couldn't think of a high-level way to perform a test.  So I ended up using lower-level calls, which require special previledges -- so this can't be a "normal" crash test.

I had to use emacs' hexl-mode to append an 0xFF character to the test's mime type ("text/plain"), and even then I had to deal with some wierdness.  Editing the test file in some other editor may very well cause the character to be removed, or to be changed to something else that wouldn't cause a crash.  "patch" seems to have no problems with it, though.

This test does crash without my patch for this bug, and doesn't crash with it.
Attachment #763840 - Flags: review?(joshmoz)
Flags: needinfo?(gphemsley)
It just occurred to me that I haven't tested my test on all platforms.  I've just started a tryserver build to do that.
Comment on attachment 763840 [details] [diff] [review]
Test

Review of attachment 763840 [details] [diff] [review]:
-----------------------------------------------------------------

Will this test fail if the 0xFF character is accidentally removed? Might be a good idea so we don't end up testing nothing if someone does it. You could just check the length of the string or something like that to make sure it's what we expect.
Attachment #763840 - Flags: review?(joshmoz) → review+
Comment on attachment 763840 [details] [diff] [review]
Test

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/175f9d9b9918

I followed your suggestion, Josh.
Flags: in-testsuite? → in-testsuite+
(Following up comment #40)

There were no non-spurious test failures in my all-platform tryserver build.
Whiteboard: [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.