1163422 - (CVE-2015-2727) Middle-click opens file: links from http:

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Jann Horn]

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

Comment 1

[Daniel Veditz [:dveditz]]

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>

Comment 2

[Daniel Veditz [:dveditz]]

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)

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 5

[:Gijs (he/him)]

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?

Comment 6

[:Gijs (he/him)]

(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 ?

Comment 7

[Frederik Braun [:freddy]]

This is slightly related to bug 1102224, which also asked about flags being kept when opening in a new tab

Comment 8

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 9

[:Gijs (he/him)]

(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.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Comment 11

[:Gijs (he/him)]

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?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch 2

This is better, and probably the right fix to use

Comment 13

[:Gijs (he/him)]

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?

Comment 14

[:Gijs (he/him)]

Also, I can reproduce this when meta- (ie cmd-) clicking on latest 38 beta on OS X, fwiw...

Comment 15

[:Felipe Gomes (needinfo for replies!)]

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

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8604159 [details] [diff] [review]
Patch 2

There was a failure on try, i'm looking into it

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch 3

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

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[:Gijs (he/him)]

I hear we're doing a 38.0.1. Can this ride-along?

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 25

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

We don't normally take sec-moderates on ESR branches without an overriding reason.

Comment 26

[Daniel Veditz [:dveditz]]

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

Comment 27

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/4e1245f345fa

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e1245f345fa

Comment 29

[:Felipe Gomes (needinfo for replies!)]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26f06cf22670

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b33b6374feb

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/f889f0f6d372

Comment 33

[Ada [:adalucinet]]

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.

Comment 34

[Ada [:adalucinet]]

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