537120 - (CVE-2010-1197) Content-Disposition: attachment ignored if Content-Type: multipart also present

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Brandon Sterne (:bsterne)]

Attachment: testcase (load from mod_php server)

Ilja van Sprundel from IOActive reported this issue to security@m.o.

When we receive both a C-D:attachment and a C-T:multipart header, we follow the C-T header and display the content inline.  This could potentially lead to XSS if a site lets users specify Content-Type for a file but rely on C-D to serve the file as an attachment.  Apparently, other browsers honor the C-D header.  I tested in Chromium and that is the case.

(filing as sec-sensitive for now)
-------

Hey Guys, 
There's a small security bug in FF that would allow files that are offered as a download to be rendered as inline html instead: 

If Content-Disposition is set to attachment, but content-type is set to multipart (attacker controlled),then the content-disposition is ignored, and the content is seen as a mime header.if that mime header defines a content-type of text/html then it will be rendered inline. 

This allows for XSS on some webapplications. specifically webapplications that allow an attacker to upload a file, and set it's content-type, but where the webapp sets the content disposition to attachment, trying to force the user to download the file, instead of rendering the content inline (this is actually quite common, e.g. a webmail application, webapp allowing users to upload and download files, ...). 

Firefox appears to be the only browser that does this. IE, chrome and safari will honor the content-disposition and just offer it as a download. testes both on FF 3.0 and and 3.5. 

Regards,
Ilja van Sprundel.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is pretty similar to bug 344278, and happening for the same reasons.  See bug 344278 comment 9 for the analysis and possible solutions.

Comment 2

[Brandon Sterne (:bsterne)]

Boris, any chance you can take a swing at the patch you proposed in bug 344278 comment 9?  If you don't have time for this one, please reassign to someone appropriate.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I can do it if it's not needed any time in the next few weeks... and I'm not sure who has time offhand.  jst, do you know?

Comment 4

[Brandon Sterne (:bsterne)]

Just wanted to ping you guys again.  This is an externally reported bug and the reporter is asking about progress.

Comment 5

[Johnny Stenback (:jst)]

The people most suitable for working on this are unfortunately not available to work on this right now, but the plan is to have this fixed in time for 3.6.3. IOW, we should start seeing progress here in about 4 or so weeks.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

jst/bz - it's been four weeks :) any chance of this making an early-week code freeze? I'm guessing no.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Still trying to figure out a way to write an automated test; in the meantime I put up a test at http://landfill.mozilla.org/ryl/testMultipart.cgi

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1: make sure multipart parts install a type sniffer

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2: change the URILoader to not do conversions for external handling

This is safe because the only channels that can force external handling right now are HTTP and part channels, and they both do type sniffing themselves.

There's an automated test for the multipart changes in part 1.  I manually tested part 2 and filed bug 558412 on test infrastructure that would be needed for automated testing.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 438136 [details] [diff] [review]
Part 2: change the URILoader to not do conversions for external handling

Stealing review, r=jst

Comment 11

[Jason Duell]

Comment on attachment 438135 [details] [diff] [review]
Part 1: make sure multipart parts install a type sniffer

looks & tests good.

Comment 12

[Johnny Stenback (:jst)]

Checked in on trunk:

http://hg.mozilla.org/mozilla-central/rev/31759ae87a88
http://hg.mozilla.org/mozilla-central/rev/fccdd48b82ae

Comment 13

[LegNeato]

Comment on attachment 438135 [details] [diff] [review]
Part 1: make sure multipart parts install a type sniffer

a=LegNeato for 1.9.2.4 and 1.9.1.10

Please note code freeze for 1.9.2.4 is today @ 11:59 pm PST.

Comment 14

[Johnny Stenback (:jst)]

Fixed for 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e76b3e0159d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de273d70e79b

Comment 15

[Johnny Stenback (:jst)]

Fixed for 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/abc92619654c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f9e9ac4016a7

Also verified on mozilla-central, 1.9.2 and 1.9.1.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Do we have a web server with mod_php installed anywhere? I thought that mine had it installed but it doesn't and I don't have root on that box.

Comment 17

[Johnny Stenback (:jst)]

For moco internal testing you could use http://gandalf/~jst/content-disposition.php, other than that I don't know...

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Johnny, can you give me the internal IP of that machine? I can't resolve it with just the single name when I VPN into the net.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

I have it now. Post-fix, the user is prompted to download the file appropriately.

I verified it for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100413 Namoroka/3.6.4pre (.NET CLR 3.5.30729).

For 1.9.1, I used Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100413 Shiretoko/3.5.10pre (.NET CLR 3.5.30729).

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 438135 [details] [diff] [review]
Part 1: make sure multipart parts install a type sniffer

Requesting approval1.9.0.next on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 438136 [details] [diff] [review]
Part 2: change the URILoader to not do conversions for external handling

Requesting approval1.9.0.next on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 438135 [details] [diff] [review]
Part 1: make sure multipart parts install a type sniffer

Approved for 1.9.0.20, a=dveditz

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 438136 [details] [diff] [review]
Part 2: change the URILoader to not do conversions for external handling

Approved for 1.9.0.20, a=dveditz

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Part 1:

Checking in netwerk/streamconv/converters/nsMultiMixedConv.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp,v  <--  nsMultiMixedConv.cpp
new revision: 1.96; previous revision: 1.95
done
Checking in netwerk/streamconv/converters/nsMultiMixedConv.h;
/cvsroot/mozilla/netwerk/streamconv/converters/nsMultiMixedConv.h,v  <--  nsMultiMixedConv.h
new revision: 1.29; previous revision: 1.28
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_multipart_streamconv.js,v
done
Checking in netwerk/test/unit/test_multipart_streamconv.js;
/cvsroot/mozilla/netwerk/test/unit/test_multipart_streamconv.js,v  <--  test_multipart_streamconv.js
initial revision: 1.1
done

Part 2:

Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.151; previous revision: 1.150
done