1148732 - (CVE-2015-4483) feed: protocol + POST method => mixed scripting

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Masato Kinugawa]

Attachment: PoC.html

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150320202338

Steps to reproduce:

Using feed: protocol and POST method, Firefox does not block mixed scripting.


Actual results:

Firefox does not block mixed scripting.


Expected results:

Mixed scripting should be blocked.

Comment 1

[Masato Kinugawa]

Oops, please wait. Due to redirect, I can't reproduce this bug from bugzilla's attachment file.

Comment 2

[Masato Kinugawa]

Comment on attachment 8585005 [details]
PoC.html

<form id="f" action="feed:https://mkpocapp.appspot.com/bug1148732/victim" method="post">
</form>
<button onclick="f.submit()">go</button>

Comment 3

[Masato Kinugawa]

Attachment: PoC.html

Comment 4

[Masato Kinugawa]

OK! Using POST request to feed: URL, an attacker can bypass mixed scripting protection.

Steps to reproduce:

1. Go to https://mkpocapp.appspot.com/bug1148732/victim . Firefox blocks HTTP: resource.

2. Go to https://bugzilla.mozilla.org/attachment.cgi?id=8585012 and click "go" button. Firefox does not block HTTP: resource.

Comment 5

[Daniel Veditz [:dveditz]]

I don't know whether to blame the feed code or the mixed content blocker, but I'll start with the latter. Perhaps it's just looking at the top scheme and not de-nesting the URL? If that's the case we might actually have the same problem with other nested schemes like jar:https://example.com/file.zip!/index.html

Tanvi: could you whip up a test for that case?

Does this need to be hidden? It's not really a vulnerability in the sense of putting the user at risk directly. The web site itself is broken and insecure, and in this case our efforts to help protect the user didn't work. It's definitely a bug, but it doesn't seem like an "attack" we need to hide in order to prevent bad guys from learning about and using it.

Comment 6

[Masato Kinugawa]

>Does this need to be hidden?
I think this bug should be hidden.

At least, this bug affects google analytics code. 
The following code is used in many web site:

<script type="text/javascript">
var _gaq = _gaq || [];
_gaq.push(['_setAccount', 'UA-xxxxx-y']);
_gaq.push(['_trackPageview']);
(function() {
var ga = document.createElement('script'); ga.type = 'text/javascript'; ga.async = true;

ga.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.google-analytics.com/ga.js'; 

var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(ga, s);
})();
</script>

This line is vulnerable:
ga.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.google-analytics.com/ga.js'; 

Due to feed: bug, location.protocol points feed:. And mixed scripting occurs.
I have reported it to Google.

Comment 7

[Masato Kinugawa]

Attachment: Spoof_PoC.html

Also, this bug brings address bar spoofing. I attached PoC.

Comment 8

[Tanvi Vyas[:tanvi]]

hmm, something to do with nested urls?

Going to feed:https://mkpocapp.appspot.com/bug1148732/victim directly takes you to https://mkpocapp.appspot.com/bug1148732/victim and the script is blocked.  But going there via the form's "go" button takes you too feed:https://mkpocapp.appspot.com/bug1148732/victim.  At that point, Mixed Content Blocker doesn't find the nested https and assumes the page isn't https.  And hence doesn't block the mixed content.

Comment 9

[Tanvi Vyas[:tanvi]]

The address bar spoof is also interesting.  I do get two separate alert boxes I have to click through for that one.  Dan, can you take a look at the address bar spoof?

Comment 10

[Tanvi Vyas[:tanvi]]

Live example of the nested feed issue (copies from Masato's proof of concept):
https://people.mozilla.org/~tvyas/nested_feed.html

Comment 11

[Tanvi Vyas[:tanvi]]

I'm not sure what's going on here yet.  But a few guesses...

1) The check for scheme in MixedContentBlocker takes place here: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#565.  Perhaps we should be calling NS_GetInnerMostURI() first.  But should I also be checking the outer schemes?  And what do I need to check them against?

2) The feed converter code newURI code looks pretty complicated:
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedConverter.js#528

Maybe there is a bug here?

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> The address bar spoof is also interesting.  I do get two separate alert
> boxes I have to click through for that one.  Dan, can you take a look at the
> address bar spoof?

The first confirmation is irrelevant -- an attacker can get rid of that one by hosting their attack on a regular http page.

Interesting that it appears to be buggy: if you change the form action to feed:https://... it still gives the warning, so it's only comparing scheme != "https" rather than de-nesting URLs (or at least my guess based on its behavior; will check later) and complaining only when the connection really is insecure.

The second confirmation is UN-avoidable in this example, it's a defense we developed for just this kind of phishing attack that takes advantage of the userinfo field. It's pretty confusing though and I can see some users not understanding it.

Another bug is that the URL-bar fixup routine is supposed to strip out userinfo from the URL. Of course it's also supposed to strip the feed: bit so maybe this is simply part of the original problem reported in this bug report.

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> 1) The check for scheme in MixedContentBlocker takes place here:
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsMixedContentBlocker.cpp#565.  Perhaps we should be calling
> NS_GetInnerMostURI() first.  But should I also be checking the outer
> schemes?  And what do I need to check them against?

Sometimes this is doing the right thing, I guess when it gets the location from a nsIPrincipal? If I have a secure document (e.g. open the console in this bug) and do
  document.location = "data:text/html,<iframe src='http://people.mozilla.org'></iframe>"
that frame is correctly blocked as mixed content.

It should never be harmful to get at the innermost URI. If you get a different URI than you started with then that was definitely the right thing to do. You don't need to worry about "outer" schemes in that case because all the wrapping pseudo-protocols inherit the security properties of the actual network request represented by the real innermost URL.

I don't know if feed://relative will unwrap, but that's OK because in that case feed: is correctly insecure according to your test.

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: Bug1148732.patch

Well, I can fix this bug with a simple patch to check the innermost URI in mixed content blocker.  But I still have many questions about why this is happening.  And this doesn't solve the url bar spoofing attack.

* Should the form action secure->insecure warning be checking nested protocol handlers?  For both the top level page's uri and the form action uri?

* Why does going to feed:url take you to url, but if you do <form action=feed:url>, you are taken to feed:url?

* Is the reason that nsMixedContentBlocker is doing the wrong thing because we aren't getting the requestingLocation from the principal in this case? (This one, I can actually try and answer myself)

Comment 14

[Tanvi Vyas[:tanvi]]

Dan's comment:
(In reply to Daniel Veditz [:dveditz] from comment #12)
> 
> Sometimes this is doing the right thing, I guess when it gets the location
> from a nsIPrincipal? If I have a secure document (e.g. open the console in
> this bug) and do
>   document.location = "data:text/html,<iframe
> src='http://people.mozilla.org'></iframe>"
> that frame is correctly blocked as mixed content.
> 

leads to this question:
(In reply to Tanvi Vyas [:tanvi] from comment #13)
> * Is the reason that nsMixedContentBlocker is doing the wrong thing because
> we aren't getting the requestingLocation from the principal in this case?
> (This one, I can actually try and answer myself)


Here is the answer:
In this example https://people.mozilla.org/~tvyas/nested_feed.html, we do infact get the requestingLocation from a principal.  We get the principal from the requesting context's NodePrincipal() and then use that to set the requestingLocation.  We then later check the scheme of the requestingLocation.

Same for the data: example:
document.location = "data:text/html,<iframe src='http://people.mozilla.org'></iframe>"
We get the uri the same way, from the requestingContext's principal.  And we block the frame (with or without the NS_GetInnermostURI check).

Hence, the root of the problem here may not be whether or not we call NS_GetInnermostURI or used a principal to get the uri, but may be in the feed code itself.

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to Tanvi Vyas [:tanvi] from comment #13)
> And this doesn't solve the url bar spoofing attack.

Yeah, that's going to be tricky. The code that tries to solve that is
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#50

It doesn't know how to handle nested URLs, it would have to de-nest the URLs and then re-wrap them and we currently don't support that. Or we start measuring the use of userpass in URLs on the web and if it's small enough just stop supporting it (have the protocol handlers themselves remove it if found, or reject URLs that have it like IE does/did). 

I have no idea why it makes a difference whether this was a GET or a POST.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 8590452 [details] [diff] [review]
Bug1148732.patch

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

r=dveditz

This will fix similar mixed content bustage with jar:https://example.com/code.jar!/hasmixed.html

Comment 17

[Tanvi Vyas[:tanvi]]

Dan is right. Same issue with jar protocol handler: jar:https://people.mozilla.org/~tvyas/my.zip!/mixedcontent.html

Should we land the patch and mark the bug as keep-open to handle the url bar attack?  And can we get someone familiar with the feed: protocol to make sure the code is sane?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8590452 [details] [diff] [review]
Bug1148732.patch

Please null check the return value of NS_GetInnermostURI

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Bug1148732-04-14-15.patch

Updated patch.

And push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b5201029ed

Comment 20

[Tanvi Vyas[:tanvi]]

Comment on attachment 8592391 [details] [diff] [review]
Bug1148732-04-14-15.patch

Carrying over Dan's r+.

Comment 21

[Tanvi Vyas[:tanvi]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e057eb58b1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0e057eb58b1

Comment 23

[Tanvi Vyas[:tanvi]]

I should have marked this leave-open.  There are other issues in this bug (the biggest being the url bar spoof).

Who is the right person to needinfo or cc for that?  Dan, do you have any suggestions?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Or should we track other related issues in separate bugs?

Comment 25

[Tanvi Vyas[:tanvi]]

Not sure how to handle this because it's a security sensitive bug.  I don't want it to get opened up because we landed a fix to 1/2 of the poc's.

Comment 26

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

Are we going to want this on more than trunk?

Comment 27

[Tanvi Vyas[:tanvi]]

Who can help fix the url spoof security issue?

Comment 28

[Tanvi Vyas[:tanvi]]

Dan, do you know who would be a good candidate to fix the url spoof issue?  Who has worked on such issues in the past?

Comment 29

[Daniel Veditz [:dveditz]]

Looks like the patch didn't fully solve the original PoC in attachment 8585012 [details] -- you're blocking the in-document script but not the mixed iframe. I think you need to do a similar check of the innermost URI for each parent around:
https://dxr.mozilla.org/mozilla-central/rev/531d1f6d1cde1182e9f7f9dff81a4fc5abc0a601/dom/security/nsMixedContentBlocker.cpp#705

I can see places where we add feed: to the scheme, but I don't know where it gets removed from non-feed feeds. It's not URIFixup like I thought. Normally a "spoof" is handled by the Firefox front-end folks, but in this case it's somewhere in how we're handling URLs. Might be in the feed code somewhere but I'm not seeing it.

Is it possible we're preserving the scheme on POSTs so we know what to use if the user tries to reload? Since feed: (and pcast: -- I'm sure we'll have the same problem there) is so non-standard could we simply prevent non-GET methods by throwing errors?

Honestly it'd be nice to kill non-standard feed: entirely, except that we're using it internally. Maybe we need telemetry to see how much use this sees on the real internet (counting our own conversions of "possible feeds" too feed: urls separately).

(In reply to Tanvi Vyas [:tanvi] from comment #25)
> Not sure how to handle this because it's a security sensitive bug.  I don't
> want it to get opened up because we landed a fix to 1/2 of the poc's.

At the point you thought you fixed this we should have cloned a new bug for the spoofing issue, and then add a note to the whiteboard saying something like "don't unhide until bug <newbug> is fixed". We could still do that, but don't close this bug for the first issue until it's actually fixed.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, since you already landed the first patch within this bug I am going to assign it to you.

Comment 31

[Ethan Tseng [:ethan]]

Dimi said he would like to help on this bug.
Thanks, Dimi!

Comment 32

[Dimi Lee [:dimi]]

Hi Tanvi,
Just went through all the comments and did some experiments.
I am not sure what is the expect result for "fixing url spoof", could you help explain a little bit ? thanks!

Comment 33

[Tanvi Vyas[:tanvi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #32)
> Hi Tanvi,
> Just went through all the comments and did some experiments.
> I am not sure what is the expect result for "fixing url spoof", could you
> help explain a little bit ? thanks!

File a new bug for the url spoof and make sure it is security sensitive.

Then, per comment 29, change this check to use innerURI:
https://dxr.mozilla.org/mozilla-central/rev/531d1f6d1cde1182e9f7f9dff81a4fc5abc0a601/dom/security/nsMixedContentBlocker.cpp#706

Comment 34

[Dimi Lee [:dimi]]

(In reply to Tanvi Vyas [:tanvi] from comment #33)
> File a new bug for the url spoof and make sure it is security sensitive.
> 

Bug 1272171 is filed to track url spoof issue

Comment 35

[Dimi Lee [:dimi]]

Attachment: Bug 1148732 - Fix mixed iframe. v1

Comment 36

[Dimi Lee [:dimi]]

I spent some time checking what's the difference between GET and POST.

The difference is that |nsFeedSniffer| only sniffer a 'GET' request[1].
So when necko process response from "https://mkpocapp.appspot.com/bug1148732/victim", it will try to retrive content type by following calling sequence[2]:
CallTypeSniffers -> NS_SniffContent -> GetMIMETypeFromContent
And only GET requests may get content type - "application/vnd.mozilla.maybe.feed"[1].

Then when URI loader load "feed:https://mkpocapp.appspot.com/bug1148732/victim" it will see if we can find a convert for it[3], in this case, 'GET' have "application/vnd.mozilla.maybe.feed" content type so |FeedConverter| take place for following network data. FeedConverter then open channel with "feed" stripped uri[4].

As for PUT requests, since FeedConverter doesn't involve, so the uri still has "feed:" in the beginning.

[1]https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/nsFeedSniffer.cpp#211
[2]https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#964
[3]https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/nsStreamConverterService.cpp#485
[4]https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedConverter.js#261

Comment 37

[Tanvi Vyas[:tanvi]]

Comment on attachment 8754710 [details] [diff] [review]
Bug 1148732 - Fix mixed iframe. v1

Thanks for the patch!  Looks good except for one change...

Error out and reject the load if we can't get innerURI, like we do here:
https://dxr.mozilla.org/mozilla-central/rev/531d1f6d1cde1182e9f7f9dff81a4fc5abc0a601/dom/security/nsMixedContentBlocker.cpp#619

Comment 38

[Dimi Lee [:dimi]]

Attachment: Bug 1148732 - Fix mixed iframe. v2

Hi tanvi,
Thanks for review!

This patch:
- Address comment 37
- separate |!parentURI| and |innerParentURI->SchemeIs| fail check into two blocks for better logic after changing.

Comment 39

[Tanvi Vyas[:tanvi]]

Comment on attachment 8755275 [details] [diff] [review]
Bug 1148732 - Fix mixed iframe. v2

Thanks Dimi!

Comment 40

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1c8f52f1c35

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8f205224d861b25d963c0cc7340b6d73939281

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c8f205224d8