751 bytes, application/zip
55.03 KB, image/png
1.48 KB, patch
|Details | Diff | Splinter Review|
Confirmed on latest Nightly. Seems that we have several odd things happening here.
I'm wondering if there were any follow-up on this issue this week? Thanks.
Moving this to the proper component so, maybe, people will see it.
Any follow-ups on this issue?
If you have to convince a user to download a file to a specific path that the website then needs knowledge of to get to a spoof, that feels like it's very hard to exploit. If you can direct the user to download a particular file to a particular location, it feels like that would enable you to do more severe things than spoof the location bar. In other words, I'm not really sure about the severity here. In fact, does this work if spoof.html is hosted on http? The screenshot shows it on file:///. I would expect it to only work if hosted locally, because the POST to file:/// is presumably not allowed. And if that *is* allowed, that feels much more like a docnav issue to me than something to do with the location bar. (In reply to Yuyang Zhou from comment #0) > Never change the address bar and navigation utill the page has been full > loaded. As written, I don't believe this helps (and it has other downsides both from a UX and security perspective). AIUI, it is possible for the testcase to work even if google.com loads fully - in fact, if location.href has been updated to reflect google.com, it *is* loaded (at least to some degree - waiting for extra images or stylesheets or whatever won't make a difference here). Instead, the location bar should be updating when the JS runs, though I'd argue that the JS shouldn't be running against the other window at all, because it shouldn't have the same principal as the file-linked-to-google.com document. That's a Core issue, and it wouldn't go away by just not updating the location bar. Boris, I would expect that we get notified (like through nsIWebProgressListener) when the JS runs here somehow. As noted, I also think the JS shouldn't be running at all, and the fact that it does is a Core issue that needs to be addressed. Am I missing something?
Also pulling in Christoph because of the ... interesting... similarities with bug 1278013. Which is fixed for 48. Not sure why it's not helping here.
There is nothing preventing POST to a file:/// URL that I know of. It just gets turned into a GET, with the postdata dropped on the floor. This does in fact look like bug 1278013 to me. And at least on Mac if I try the attached testcase (replacing the .url file with an equivalent .desktop file) I don't see the JS running or the spoof working. I assume the problem _is_ reproducible on Windows? If so, I'd really appreciate it if someone who has easy access to a Windows debug build could check what things look like in nsJSProtocolHandler.cpp, in the nsJSThunk::EvaluateScript, around the principal->Subsumes(objectPrincipal) call. What are principal and objectPrincipal in this case?
Wait. The patch for bug 1278013 is using "^" and not "&~" to remove the security-inheritance bit? Which means it might be _adding_ it in this case in some situations? That could explain what's going on, if so... :( Gijs, if you change nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() ^ nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; to: nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() & ~nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; in nsBaseChannel::Redirect, does that fix the problem?
7 months ago
(In reply to Boris Zbarsky [:bz] from comment #10) > Wait. The patch for bug 1278013 is using "^" and not "&~" to remove the > security-inheritance bit? Which means it might be _adding_ it in this case > in some situations? That could explain what's going on, if so... :( > > Gijs, if you change > > nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() ^ > nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; > > to: > > nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() & > ~nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL; > > in nsBaseChannel::Redirect, does that fix the problem? With this change, AFAICT the JS no longer runs. It's hard to be 100% sure because of the timeout, which means the testcase feels like it'll be a bit racy, but actually it's 100% reproducible for me on beta, and 0% reproducible after this change, when testing on self-compiled nightly. So I suspect you're right. Please needinfo if it'd be helpful for me to poke at this more (e.g. stacks for how we're hitting this case for this exploit) and I'll try and answer any questions tomorrow if that's helpful.
OK, I thought about it more, and I can reproduce this bug on Mac too. All I have to do is make sure that p.desktop is in the _parent_ of the directory that houses spoof.html, so that normally the load of p.desktop would _not_ inherit the principal. The use of xor instead of &~ means the inherit bit gets added in that situation... Patch coming up.
We should take this on all branches, including 48 and 45esr. Because once someone looks at the advisory for bug 1278013 this would be the obvious thing to try. :(
Created attachment 8786922 [details] [diff] [review] Be more careful about removing out inherit principal bit on base channel redirects
7 months ago
Created attachment 8786924 [details] [diff] [review] Be more careful about removing the inherit principal bit on base channel redirects
7 months ago
Comment on attachment 8786924 [details] [diff] [review] Be more careful about removing the inherit principal bit on base channel redirects [Security approval request comment] How easily could an exploit be constructed based on the patch? Good question. Probably pretty easily if you know how this codepath is hit and how our file security model works. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not as far as I could avoid. I did explicitly talk about removing the bit, but I assume anyone who sees fooo & ~bar will know that anyway. Which older supported branches are affected by this flaw? 45esr, 48, 49, 50, 51. If not all supported branches, which bug introduced the flaw? Bug 1278013. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should be a trivial backport. How likely is this patch to cause regressions; how much testing does it need? This seems pretty safe to me. Minimal testing is probably OK.
Comment on attachment 8786924 [details] [diff] [review] Be more careful about removing the inherit principal bit on base channel redirects Review of attachment 8786924 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Boris Zbarsky [:bz] from comment #12) > The use of xor instead of &~ means the inherit bit gets added in that situation... Arrrh. Headbang!!! Thanks Boris for fixing this up!
Daniel, note that this bug basically allows file:// stuff to violate our security model for file://. Place a .url/.desktop file in a dir pointing to an arbitrary file, have a file in a _child_ of that dir that links to that .url/.desktop, load that file from the child dir, and suddenly you are same-origin with the arbitrary file the .url/.desktop links to. So it may be worth taking this in a security update to 48, unless there simply isn't time for one before 49 ships anyway.
There isn't time for this before 49 ships. We've had our last beta. As a sec-moderate (unless this is mis-rated), it doesn't need sec-approval+ to land on trunk.
Comment on attachment 8786924 [details] [diff] [review] Be more careful about removing the inherit principal bit on base channel redirects Approval Request Comment [Feature/regressing bug #]: Bug 1278013 [User impact if declined]: Allows files to attack other files on the hard drive in ways they shouldn't be able to do, in addition to creating spoofing risks. [Describe test coverage new/current, TreeHerder]: Test coverage is lacking for file:// stuff. [Risks and why]: I think this is pretty low risk, but since this is a regression from what we thought was a low-risk fix... [String/UUID change made/needed]: None.
Thanks for the timely release of the patch. I'm wondering if this report could be assigned a CVE ID?
I believe we assign CVEs when we get closer to actually shipping the fix... So far it's just checked into the development trunk.
Doesn't match the esr45 criteria
Wait, what? Why not? What are these criteria?
(In reply to Boris Zbarsky [:bz] from comment #26) > Wait, what? Why not? What are these criteria? Generally sec-high or sec-critical unless a special case is made. We don't take moderate or low rated issues on the ESR branch normally for stability reasons.
I see. So same-origin policy bypass for file:// URIs is not something we would normally take on ESR?
Dan is the one who rated it sec-moderate so you would need to ask him why it isn't a high if you think it is misrated. Otherwise, policy is that we don't take moderate or low bugs on ESR. We have to walk a line between "all security fixes" and "no changes, keep it stable."
(In reply to Sylvestre Ledru [:sylvestre] from comment #25) > Doesn't match the esr45 criteria This was already marked "49+", because on security grounds it does. "sec-moderate" doesn't mean "we never take it on ESR", it means we judge it case-by-case. The download requirement lessens the mass-pwnage possibilities (thus not "high"), but the impact to affected individuals can still be severe. Looks like it's too late for 49 (RC already shipped, this isn't a re-spin type fix) so I'll move the ESR to match "50+", but we do still want it on ESR just as we took bug 1278013 on ESR.
Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and sec-moderate. If you think that the risk is high enough to take it into ESR, we should probably bump the severity.
Well, I can't tell, because our severity ratings are all web-site-centric and don't really address the threat vector being a local file. In this case, the attack would involve placing a specially crafted .desktop/.url file on the users disk, so maybe it's not that dangerous. I just can't tell. Do we consider a file on a network drive reading arbitrary files off your hard disk a sec-moderate or a sec-high?
(In reply to Sylvestre Ledru [:sylvestre] from comment #31) > Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and > sec-moderate. > If you think that the risk is high enough to take it into ESR, we should > probably bump the severity. We do take exceptions to this policy. The dev or someone else just makes a case for it normally. I didn't realize Dan had wanted it. Bumping the severity isn't really a solution as it messes up our rating system, which is not "make this more severe so we'll take it on ESR." The ratings have specific meanings (as much as possible).
(In reply to Al Billings [:abillings] from comment #33) > (In reply to Sylvestre Ledru [:sylvestre] from comment #31) > > Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and > > sec-moderate. > > If you think that the risk is high enough to take it into ESR, we should > > probably bump the severity. > > We do take exceptions to this policy. The dev or someone else just makes a > case for it normally. I didn't realize Dan had wanted it. Bumping the > severity isn't really a solution as it messes up our rating system, which is > not "make this more severe so we'll take it on ESR." The ratings have > specific meanings (as much as possible). Additionally, as Dan said in comment #30, we took bug 1278013 on ESR, which was also sec-moderate rated and involved a similar-ish problem. This bug is a direct regression from that bug, and so arguably as a regression that we uplifted to ESR, the fix for that regression should be uplifted as well, particularly because it is a security issue that, as noted in comment #16, is reasonably "obvious" given both the commit here and the previous fix which was already CVE'd and made public as fixed in earlier versions of ESR.
Comment on attachment 8786924 [details] [diff] [review] Be more careful about removing the inherit principal bit on base channel redirects As we will ship 50 with the sec fix and several people think we should take it despite its severity, let's do it.
I've managed to reproduce this issue on 48.0.2 (2016-08-04) using STR form comment 0. This is verified fixed on 45.5.0esr (20161031153904), 50.0-build2 (20161104212021) and 51.0a2 (2016-11-8) using Windows 10 x64. Marking here accordingly.
Does this bug need to still be private? Someone inquired in bug 1320701.