Closed Bug 1163422 (CVE-2015-2727) Opened 4 years ago Closed 4 years ago

Middle-click opens file: links from http:

Categories

(Firefox :: Security, defect)

40 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox37 --- unaffected
firefox38 + wontfix
firefox38.0.5 + wontfix
firefox39 + verified
firefox40 + verified
firefox41 --- verified
firefox-esr31 --- wontfix
firefox-esr38 39+ verified

People

(Reporter: jann+mozilla, Assigned: Felipe)

References

()

Details

(Keywords: sec-moderate, Whiteboard: [adv-main39+][adv-esr38.1+])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux i686 (x86_64)) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

1. navigate to an http site with the following HTML: <a href="file:///etc/passwd">clickme</a>
2. middle-click the link

dveditz says that this doesn't happen on 37.0.2.


Actual results:

file:///etc/passwd opened in a new tab


Expected results:

nothing should have happened, just like when the link gets left-clicked or "open link in new tab" is selected in the context menu
Yup, I confirm it happens on Nightly(40) and file:/// links are correctly blocked in Release 37.0.2.

If you don't have a server handy you can test this by opening the following data: url

   data:text/html,<a%20href="file:///etc/passwd">clickme</a>
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Product: Firefox → Core
Figured out why this sounded so familiar--
https://www.mozilla.org/en-US/security/advisories/mfsa2015-25/

When we "fixed" that one in bug 1111960 we added spot fixes for beta and aurora at the time (36 and 37), intending to land a "real" fix on trunk. Which we didn't do, so now we're about to regress that advisory in the 38 release in two days. As a "moderate" security bug it's hard to justify a stop-ship rebuild, but that's pretty embarrassing. I hope it's more realistic to get this into 38.0.5

(possibly this should just be duped to bug 1111960)
Component: DOM: Security → Security
Depends on: CVE-2015-0821
Flags: needinfo?(lmandel)
Product: Core → Firefox
Flags: sec-bounty?
38.0.5 should not have any ridealongs but given the nature of this bug, if we have a safe fix, we can consider it for 38.0.5. We should also flag this as a ridealong for any point release.
Flags: needinfo?(lmandel)
It looks like this isn't actually a regression, but we only fixed 1111960 on branches, and now sufficient time has passed and bug 903016 is still not fixed, so we're releasing versions with this bug again? :-(

Felipe, is that accurate?
Flags: needinfo?(felipc)
(In reply to :Gijs Kruitbosch from comment #5)
> It looks like this isn't actually a regression, but we only fixed 1111960 on
> branches, and now sufficient time has passed and bug 903016 is still not
> fixed, so we're releasing versions with this bug again? :-(
> 
> Felipe, is that accurate?

Egh, comment #2 said as much - can we just reland the stopgap from bug 1111960 on 38.0.5 ?
This is slightly related to bug 1102224, which also asked about flags being kept when opening in a new tab
Keywords: sec-moderate
Bah sorry, in my memory bug 1111960 had been fixed in a way that would always apply to aurora/beta (e.g. with ifdefs), not just the current versions at the time.

The patch from bug 1111960 doesn't apply cleanly anymore because there have been new code landed in the file that it removed, but I have a pretty simple and safe workaround for this, I'll post it.

Interestingly I can't reproduce this bug on beta anymore. Maybe something else fixed it? (but I'll confirm). I can only reproduce it on mozilla-release.

I haven't been following closely the details of 38.0.5, is mozilla-release the right tree for it?
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes from comment #8)
> I haven't been following closely the details of 38.0.5, is mozilla-release
> the right tree for it?

Yes.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8604154 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8604154 [details] [diff] [review]
Patch

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

This breaks (modified) clicking on data: and javascript: urls, doesn't it?
Attachment #8604154 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
This is better, and probably the right fix to use
Attachment #8604159 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8604159 [details] [diff] [review]
Patch 2

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

Seems this always throws on failure, so might as well not have the bool and return from the catch only?
Attachment #8604159 - Flags: review?(gijskruitbosch+bugs) → review+
Also, I can reproduce this when meta- (ie cmd-) clicking on latest 38 beta on OS X, fwiw...
Attachment #8604154 - Attachment is obsolete: true
Comment on attachment 8604159 [details] [diff] [review]
Patch 2

Approval Request Comment
[Feature/regressing bug #]: regression from bug 897062. This was fixed temporarily in bug 1111960 for aurora/beta at the time. But this is the non-temporary fix
[User impact if declined]: Security bug as described
[Describe test coverage new/current, TreeHerder]: i'll push to try
[Risks and why]: risk is limited to cmd/shift click on links
[String/UUID change made/needed]: none
Attachment #8604159 - Flags: approval-mozilla-release?
Just pushed it to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec62cf66f9c5

I wonder if I should go ahead and land on inbound
Comment on attachment 8604159 [details] [diff] [review]
Patch 2

There was a failure on try, i'm looking into it
Attachment #8604159 - Flags: approval-mozilla-release?
Attached patch Patch 3Splinter Review
There is the feature of middle-click to paste on Linux, so we must return only if we're in the case if special-clicking a link.

Passed test on https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ab9e5c166e
Assignee: nobody → felipc
Attachment #8604159 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8604366 - Flags: review?(gijskruitbosch+bugs)
Attachment #8604366 - Flags: review?(gijskruitbosch+bugs) → review+
Is this patch for trunk or release branches (or both)? The try on m-e10s means it's for m-c at least but hopefully we don't need a separate backport version.
Flags: needinfo?(felipc)
I hear we're doing a 38.0.1. Can this ride-along?
Flags: needinfo?(lmandel)
We're trying to gtb with 38.0.1 today and this fix doesn't look like it's ready. It's certainly not great to ship a regression like this but this being a sec-moderate I think we can ship 38.0.1 without it. Please correct me if I've misunderstood the impact. Note that taking this fix at this point will likely delay the 38.0.1 release by a day, which may also impact the 38.0.5b2 release.
Flags: needinfo?(lmandel)
This patch applies for both m-c and release and I tested it locally with both trees (and sent to try with m-c).

Let me know what people want to do about landing it for 38.0.1 or 38.0.5b5 and I can push.
Flags: needinfo?(felipc)
We're too late for 38.0.1. I'm happy to take this in 38.0.5 so long as the sec team is OK shipping this fix in 38.0.5 and waiting another 4 weeks to ship the fix to ESR 38. If not, we should wait for 39.
Dveditz, what are your thoughts on taking this for 38.0.5 or waiting for 39 because of ESR? Also, should I land this on m-c or wait to land everything close together?
Flags: needinfo?(dveditz)
We don't normally take sec-moderates on ESR branches without an overriding reason.
We shouldn't ram this into 38.0.5 at this point, but I would like to get this regression fixed on ESR-38 when we do fix it. Since we're just starting ESR-38 let's nip this in the bud and not worry for the next year whether someone can build an exploit by combining this with another bug.

Go ahead and land on m-c when you're ready, and let's get this uplifted to aurora, beta, and ESR-38
https://hg.mozilla.org/mozilla-central/rev/4e1245f345fa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8604366 [details] [diff] [review]
Patch 3

Approval Request Comment
[Feature/regressing bug #]: regression from bug 897062. This was fixed temporarily in bug 1111960 for aurora/beta at the time. But this is the non-temporary fix
[User impact if declined]: Security bug as described
[Describe test coverage new/current, TreeHerder]: landed in central
[Risks and why]: risk is limited to cmd/shift click on links
[String/UUID change made/needed]: none
Attachment #8604366 - Flags: approval-mozilla-esr38?
Attachment #8604366 - Flags: approval-mozilla-beta?
Attachment #8604366 - Flags: approval-mozilla-aurora?
Attachment #8604366 - Flags: approval-mozilla-esr38?
Attachment #8604366 - Flags: approval-mozilla-esr38+
Attachment #8604366 - Flags: approval-mozilla-beta?
Attachment #8604366 - Flags: approval-mozilla-beta+
Attachment #8604366 - Flags: approval-mozilla-aurora?
Attachment #8604366 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty-
Reproduced with 40.0a1 on Windows 7 64-bit by using str from comment 1.
Verified fixed with 39.0b6 (Build ID: 20150615125213), latest Developer Edition (from 2015-06-15) and latest Nightly (from 2015-06-15), under Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
Also verified fixed with ESR 38, across platforms [1], built from https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7

Version: 38.0.1esrpre
Build ID: 20150616140147

[1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2727
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.