Closed Bug 683250 Opened 13 years ago Closed 13 years ago

Permit multiple Content-Disposition header if semantic (attachment vs inline) stays the same

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: uweruch, Unassigned)

Details

(Keywords: regression)

Attachments

(1 file)

Permits non-identical Disposition headers provided semantic doesn't change.
6.66 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

I wanted to download programs from the Acronis-Site ( http://www.acronis.de ).


Actual results:

The download is not executed. FF 7 Beta 2 reports an error message.


Expected results:

The download should start without any problems. All other browsers (including FF 6) work fine in this case.
Using Linux and http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe as test file:

Last good nightly: 2011-07-05
First bad nightly: 2011-07-06

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f27dc203e62&tochange=58101c64c83c
Keywords: regression
OS: Windows 7 → All
Hardware: x86_64 → All
Local track down using Linux:

The first bad revision is:
changeset:   72318:cc405052ac0a
user:        Jason Duell <jduell.mcbugs@gmail.com>
date:        Mon Jul 04 23:12:32 2011 -0700
summary:     bug 655389 - CRLF Injection and the parsing of HTTP headers. r=bz

http://hg.mozilla.org/mozilla-central/rev/cc405052ac0a
Failed download:
----------------------------------------------------------
http://download.acronis.com/ATIH2012_trial_en-EU.exe

GET /ATIH2012_trial_en-EU.exe HTTP/1.1
Host: download.acronis.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110830 Firefox/9.0a1
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive

HTTP/1.1 302 Found
Date: Tue, 30 Aug 2011 22:13:51 GMT
Server: Apache/2.2.9 (Fedora)
Location: http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe
Content-Length: 324
Connection: close
Content-Type: text/html; charset=iso-8859-1
----------------------------------------------------------
Successful download:
----------------------------------------------------------
http://download.acronis.com/ATIH2012_trial_en-EU.exe

GET /ATIH2012_trial_en-EU.exe HTTP/1.1
Host: download.acronis.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Connection: keep-alive

HTTP/1.1 302 Found
Date: Tue, 30 Aug 2011 22:15:38 GMT
Server: Apache/2.2.9 (Fedora)
Location: http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe
Content-Length: 324
Connection: close
Content-Type: text/html; charset=iso-8859-1
----------------------------------------------------------
http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe

GET /u/ATIH2012_trial_en-EU.exe HTTP/1.1
Host: download2.acronis.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Connection: keep-alive

HTTP/1.1 200 OK
Server: Apache
Accept-Ranges: bytes
Content-Disposition: attachment; filename="ATIH2012_trial_en-EU.exe", attachment
X-Permitted-Cross-Domain-Policies: all
Content-Type: application/download
Age: 68404
Date: Tue, 30 Aug 2011 22:18:39 GMT
Last-Modified: Mon, 22 Aug 2011 18:11:33 GMT
Content-Length: 252516640
Connection: keep-alive
----------------------------------------------------------
Error message when failing:
----------------------------------------------------------
Corrupted Content Error

The page you are trying to view cannot be shown because an error in the data transmission was detected.

The page you are trying to view cannot be shown because an error in the data transmission was detected.

Please contact the website owners to inform them of this problem.
Works for me:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.20) Gecko/20110803 Firefox/3.6.20
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Opera/9.80 (X11; Linux x86_64; U; en) Presto/2.9.168 Version/11.50
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.107 Safari/535.1

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a2) Gecko/20110830 Firefox/8.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110830 Firefox/9.0a1
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: 7 Branch → Trunk
TE like Bug 682511/Bug 681140 (and per the Error Message)?
These are the actual response headers for http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe [comment 4 has them coalesced probably from firebug or something similar - the headers below are from packet capture].. The rejection is triggered by the duplicated content-disposition header. 

Jason, Julian - can you confirm that's what we should really be doing here?

HTTP/1.1 200 OK
Server: Apache
Accept-Ranges: bytes
content-disposition: attachment; filename="ATIH2012_trial_en-EU.exe"
X-Permitted-Cross-Domain-Policies: all
Content-Disposition: attachment
Content-Type: application/download
Age: 89622
Date: Tue, 30 Aug 2011 23:28:52 GMT
Last-Modified: Mon, 22 Aug 2011 18:11:33 GMT
Content-Length: 252516640
Connection: keep-alive
The headers in comment 3 and comment 4 are from the Live HTTP headers add-on, version 0.17. Apparently one can't trust that add-on for purposes like these.
(In reply to Thomas Ahlblom from comment #9)
> The headers in comment 3 and comment 4 are from the Live HTTP headers
> add-on, version 0.17. Apparently one can't trust that add-on for purposes
> like these.

http://redbot.org/?uri=http%3A%2F%2Fdownload2.acronis.com%2Fu%2FATIH2012_trial_en-EU.exe
(In reply to Patrick McManus from comment #8)
> These are the actual response headers for
> http://download2.acronis.com/u/ATIH2012_trial_en-EU.exe [comment 4 has them
> coalesced probably from firebug or something similar - the headers below are
> from packet capture].. The rejection is triggered by the duplicated
> content-disposition header. 

Indeed.

> Jason, Julian - can you confirm that's what we should really be doing here?

The header fields are invalid syntactically, but if you look at the actual contents, it's a harmless cases (attachment vs named attachment).

We've discussed this as part of another bug; is a broken C-D a security problem? If the answer is "yes" (and I think that's what BZ wants it to be), then the new behavior makes sense.

If the answer is "no" (-> me), then the header fields could be ignored, which, in this case, would lead to a download dialogue (because of the content-type not being supported for inline display).

Given the fact that this change made it into the beta without complaints you may want to stick with it, and try to get the servers fixed.
> 
> We've discussed this as part of another bug; is a broken C-D a security
> problem? If the answer is "yes" (and I think that's what BZ wants it to be),
> then the new behavior makes sense.
> 

Boris, I just want to confirm that you think the behavior in this case is desirable. I could go with either argument.

I have submitted the problem report to acronis's support system.
Same problem with Firefox 7 Beta 3. Cannot download programs from the Acronis-Site. http://www.acronis.de

Regards,
Uwe
So... as far as I understand, this is not a Problem of FF 7 Beta 3. Instead there is anything wrong with die Acronis-Site, right?

Uwe
(In reply to Helios from comment #14)
> So... as far as I understand, this is not a Problem of FF 7 Beta 3. Instead
> there is anything wrong with die Acronis-Site, right?
> 
> Uwe

There definitively is something wrong with the site (it sends multiple C-D headers when there should only be one), but it's not entirely clear whether Firefox needs to be as draconian as it is now. 

It would be interesting to know how many more sites are affected.
(In reply to Helios from comment #14)
> So... as far as I understand, this is not a Problem of FF 7 Beta 3. Instead
> there is anything wrong with die Acronis-Site, right?
> 
> Uwe

probably. acronis violates a new security policy in ff7. Before closing the bug, I wanted to check with a few people to confirm that the specific acronis behavior is actually something we want to fall under that policy.
Thanks for the information, Julian and Patrick! I hope this will be fixed by Acronis. It's not a really big deal (although it's a little annoying)... with the Internet Explorer, the problem doesn't pop up. And for downloading the updates, it's okay that far (at least for me).

@Julian: the Acronis website is the only one I know with this problem.

Thank you very much for your support.

Uwe
Patrick, from a security point of view there is a problem with going from "attachment" to "inline" but not from "inline" to "attachment".  In my opinion; I know Julian disagrees.

One could also argue that there is a problem with injecting a filename, but we really have no way to detect this in general.

What header value did we end up with for content-disposition before we added the duplicate header detection?
(In reply to Boris Zbarsky (:bz) from comment #18)
> Patrick, from a security point of view there is a problem with going from
> "attachment" to "inline" but not from "inline" to "attachment".  In my
> opinion; I know Julian disagrees.

:-)

> One could also argue that there is a problem with injecting a filename, but
> we really have no way to detect this in general.
> 
> What header value did we end up with for content-disposition before we added
> the duplicate header detection?

Not sure, but the observable behaviour was that in this case the filename param was picked up.

Slightly related: why does FF6 offer a download of type "Videoclip"?
(In reply to Julian Reschke from comment #19)
> Slightly related: why does FF6 offer a download of type "Videoclip"?

...because VLC has registered itself for that media type. Sigh.
Hi guys,

is there any news in this case? The problem still exists with FF 7 Beta 4.

Regard
Uwe
(In reply to Helios from comment #21)
> Hi guys,
> 
> is there any news in this case? The problem still exists with FF 7 Beta 4.
> 
> Regard
> Uwe

Uwe, the root problem lies with the non-compliant website application. If the problem is urgent for you please contact their customer support. I have already done so and they have not found it worth responding to so far.

-Pat
Hi Patrick,

thank you very much for the information and your support. I will contact Acronis describing the problem.

Best regards,
Uwe
So this patch allows multiple inline or attachment headers that have different filename parameters (or nonvalues), so long as the inline/attachment semantic stays consistent.

Note that I had to hand-roll parsing logic--can't use nsIMIMEHeaderParam on the socket transport thread.

>> What header value did we end up with for content-disposition 
>> before we added the duplicate header detection?
>
>Not sure, but the observable behaviour was that in this case the filename param was picked up.

In FF 6 we were still merging Content-Dispo headers into one value, so we wind up seeing 

    attachment; filename="ATIH2012_trial_en-EU.exe", attachment

and get the filename.  With this patch, we wind up taking only the filename param from the 1st header seen, which seems correct from a general security angle: if the Bad Guys inject C-D from an earlier header than the server's own CD, then they can make you see a filename of their choice: not clear how bad that is (and we already and will always use such a filename in the absence of any legit CD header, i.e if you inject a CD header into a response that doesn't otherwise have one).  

Note that we'd need to get this into both aurora/beta channels to make it really worthwhile (otherwise the regression ships in our releases, after which I'm not sure it's worth fixing).
Attachment #558015 - Flags: review?(bzbarsky)
Summary: Firefox 7 Beta 2 cannot download programs from the Acronis-Site. → Permit multiple Content-Disposition header if semantic (attachment vs inline) stays the same
(In reply to Jason Duell (:jduell) from comment #24)
> Created attachment 558015 [details] [diff] [review]
> Permits non-identical Disposition headers provided semantic doesn't change.
> 
> So this patch allows multiple inline or attachment headers that have
> different filename parameters (or nonvalues), so long as the
> inline/attachment semantic stays consistent.
> 
> Note that I had to hand-roll parsing logic--can't use nsIMIMEHeaderParam on
> the socket transport thread.

Please don't. When it works it'll be code bloat. When it doesn't in edge cases, it will be very confusing and hard to debug.

I'm not sure this merits a fix, as long as we know about one broken site only.

If it *does* need to be fixed then we should fix it properly; fold the multiple instances into a single header field (comma-separated), and then let the real C-D parser decide what do do with it.
(In reply to Jason Duell (:jduell) from comment #24)

> angle: if the Bad Guys inject C-D from an earlier header than the server's
> own CD, then they can make you see a filename of their choice: not clear how
> bad that is

That seems potentially bad - the bad guys can create a default name to overwrite something entirely different. I wouldn't want that.

Even in this case, those 2 headers (if they were taken separately) would give different results I believe (I bow to the CD-masters on that one - but one gives you a default filename and the other gives a specific one). As such, there is probably a security exposure if one of them is an injection. So we're doing the right thing with CORRUPTED.
By popular demand.
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Attachment #558015 - Flags: review?(bzbarsky)
So who's contacting the site to get them to fix this?
(In reply to Boris Zbarsky (:bz) from comment #28)
> So who's contacting the site to get them to fix this?

I think Patrick did that on Sep 1 (see comment #12).
(In reply to Julian Reschke from comment #29)
> (In reply to Boris Zbarsky (:bz) from comment #28)
> > So who's contacting the site to get them to fix this?
> 
> I think Patrick did that on Sep 1 (see comment #12).

Yes, yesterday I received a reply written by a human saying that my mail had been forwarded to engineering. My mail contains a link to this bug as well as a description of the issue.
Ah, great.  Sorry again for missing that part.  :(
> Yes, yesterday I received a reply written by a human saying that my mail had
> been forwarded to engineering. My mail contains a link to this bug as well
> as a description of the issue.

Thank you again for your support, Patrick. Will see if Acronis anything gonna change regarding to this subject.

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

Attachment

General

Creator:
Created:
Updated:
Size: