Closed Bug 1148732 (CVE-2015-4483) Opened 9 years ago Closed 8 years ago

feed: protocol + POST method => mixed scripting

Categories

(Core :: DOM: Security, defect)

36 Branch
x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
firefox50 --- fixed

People

(Reporter: masatokinugawa, Assigned: dimi)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-low, Whiteboard: [keep hidden while 1272171 is][adv-main40+], [domsecurity-active][post-critsmash-triage][adv-main50-])

Attachments

(4 files, 3 obsolete files)

Attached file PoC.html (obsolete) —
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.
Oops, please wait. Due to redirect, I can't reproduce this bug from bugzilla's attachment file.
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>
Attachment #8585005 - Attachment description: victim.html → PoC.html
Attached file PoC.html
Attachment #8585005 - Attachment is obsolete: true
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.
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: needinfo?(tanvi)
Keywords: sec-low
Product: Firefox → Core
>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.
Attached file Spoof_PoC.html
Also, this bug brings address bar spoofing. I attached PoC.
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.
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?
Flags: needinfo?(dveditz)
Live example of the nested feed issue (copies from Masato's proof of concept):
https://people.mozilla.org/~tvyas/nested_feed.html
Flags: needinfo?(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?
(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.
Flags: needinfo?(dveditz)
Attached patch Bug1148732.patch (obsolete) — Splinter Review
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)
Attachment #8590452 - Flags: review?(dveditz)
Attachment #8590452 - Flags: review?(bugs)
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.
(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 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
Attachment #8590452 - Flags: review?(dveditz) → review+
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?
Attachment #8590452 - Flags: review?(bugs) → review+
Comment on attachment 8590452 [details] [diff] [review]
Bug1148732.patch

Please null check the return value of NS_GetInnermostURI
Updated patch.

And push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b5201029ed
Attachment #8590452 - Attachment is obsolete: true
Attachment #8592391 - Flags: review?(bugs)
Comment on attachment 8592391 [details] [diff] [review]
Bug1148732-04-14-15.patch

Carrying over Dan's r+.
Attachment #8592391 - Flags: review+
Attachment #8592391 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b0e057eb58b1
Assignee: nobody → tanvi
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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?
Assignee: tanvi → jeroentulp
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Or should we track other related issues in separate bugs?
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.
Are we going to want this on more than trunk?
Assignee: jeroentulp → administration
Assignee: administration → nobody
Who can help fix the url spoof security issue?
Whiteboard: [adv-main40+]
Alias: CVE-2015-4483
Group: core-security → dom-core-security
See Also: → 1216365
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?
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
Flags: needinfo?(tanvi)
Tanvi, since you already landed the first patch within this bug I am going to assign it to you.
Assignee: nobody → tanvi
Whiteboard: [adv-main40+] → [adv-main40+], [domsecurity-active]
Flags: needinfo?(tanvi)
Assignee: tanvi → nobody
Flags: needinfo?(tanvi)
Dimi said he would like to help on this bug.
Thanks, Dimi!
Assignee: nobody → dlee
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!
(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
Flags: needinfo?(tanvi)
Whiteboard: [adv-main40+], [domsecurity-active] → [adv-main40+], [domsecurity-active][don't-unhide]
See Also: → 1272171
(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
Attachment #8754710 - Flags: review?(tanvi)
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 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
Attachment #8754710 - Flags: review?(tanvi) → review-
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.
Attachment #8754710 - Attachment is obsolete: true
Attachment #8755275 - Flags: review?(tanvi)
Comment on attachment 8755275 [details] [diff] [review]
Bug 1148732 - Fix mixed iframe. v2

Thanks Dimi!
Attachment #8755275 - Flags: review?(tanvi) → review+
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main40+], [domsecurity-active][don't-unhide] → [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage]
Whiteboard: [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage] → [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage][adv-main50+]
Whiteboard: [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage][adv-main50+] → [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage][adv-main50-]
Group: core-security-release
Keywords: leave-open
Whiteboard: [adv-main40+], [domsecurity-active][don't-unhide][post-critsmash-triage][adv-main50-] → [keep hidden while 1272171 is][adv-main40+], [domsecurity-active][post-critsmash-triage][adv-main50-]
You need to log in before you can comment on or make changes to this bug.