Closed
Bug 1145541
Opened 9 years ago
Closed 9 years ago
Unauthorized redirection while opening tabs in Firefox Hello
Categories
(Hello (Loop) :: Client, defect, P4)
Hello (Loop)
Client
Tracking
(firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed)
People
(Reporter: shahmeerbond, Assigned: standard8)
Details
(Keywords: sec-low)
Attachments
(1 file)
30.13 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36 OPR/27.0.1689.76 Steps to reproduce: Hello there firefox team I found out an issue in the firefox hello When you open a link in a new tab ( target="_blank" ), the page that opens in a new tab can access the initial tab and change it's location using the window.opener property. Paste the link in firefox hello to give it to the victim http://daniel-tomescu.com/hackerone/landpage.php Don't right-click it and open it in new tab, don't use the mouse wheel to open it, don't Ctrl+Click, just do a normal click on the link. Actual results: When the victim visits the link he will be redirected in a new tab But at the same time in his initial tab he will redirected to a scampage Expected results: The Redirection in the initial tab should not have occured, So using this vulnerability an attacker can redirect anyone to any page in the initial tab
Updated•9 years ago
|
Flags: needinfo?(standard8)
Reporter | ||
Comment 1•9 years ago
|
||
The javascript code that does all the magic: window.opener.location.replace(newURL); Ways to solve this: Don't open links from Hello application in new tabs using the target="_blank"; Set the window.opener attribute to null on the new tab before redirecting to the landing page, like this: a) open in a new tab b) use a javascript code: <script>window.opener = null; </script> c) redirect to the landing page: <script>window.location.reload(landingpage)</script> I hope you see why this is dangerous: this method has huge potential for tricking Hello that click on external links from this site to be a victim of a scam page because the redirecting is made in the background, while the user is focused on another tab. Shall i email you on the address?
Comment 2•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #1) > Shall i email you on the address? There is no need to email, the bugreport is enough. People are looking into it.
Assignee | ||
Comment 3•9 years ago
|
||
Ok, we managed to reproduce this. Slightly more detail on the steps to reproduce: 1) Start Firefox, Open the Hello panel, skip the Tour 2) Go back into the Panel, and click the cog icon, click Help => New tab opens Then either: 3a) Open the browser console, enter: window.opener.location.href = "google.com" 3b) Copy the url http://daniel-tomescu.com/hackerone/landpage.php and paste it into the url bar 4) Re-open the Hello panel Actual Results: The Hello panel has changed to the page requested Expected Results: The Hello panel didn't change.
Status: UNCONFIRMED → NEW
Component: Untriaged → Client
Ever confirmed: true
Flags: needinfo?(standard8)
OS: Windows 8.1 → All
Product: Firefox → Loop
Hardware: x86_64 → All
Version: Firefox 38 → unspecified
Assignee | ||
Comment 4•9 years ago
|
||
Marking this as sec-low - it requires manual and specific user interaction to get load the attacking page at the right time. It also only replaces the panel content - there's no access to the Hello data. https://wiki.mozilla.org/Security_Severity_Ratings
Reporter | ||
Comment 5•9 years ago
|
||
But the vulnerability still does exists and can be used to redirect users. This is nowhere near low sec. Does this get me a reward. I am ready to explain more
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #5) > But the vulnerability still does exists and can be used to redirect users. > This is nowhere near low sec. I didn't say it doesn't exist. It is just a lower risk. The main reason being is that it requires a specific set of actions that could be classed as not "normal browsing actions". At best, I think this puts it at sec-moderate. However, there's also no direct access to user data, which points more to sec-low. Yes, if the user gets spoofed then it could be bad for the user, but its a difficult route to get the user into that state. This is the assessment of a colleague and myself, but I'll ask one of the main security team for feedback on this assessment in case we've missed something. > Does this get me a reward. I am ready to explain more I'm not up to date with the current rules, hopefully the security team can answer.
Flags: needinfo?(dveditz)
Comment 7•9 years ago
|
||
Can we workaround this by specifying the hello page in the panel / browser as sandboxed if we don't already?
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7) > Can we workaround this by specifying the hello page in the panel / browser > as sandboxed if we don't already? There's a privileged API that we have that we can extend to open a new tab (or whatever), that should get us to do the right thing. I suspect Mike or myself can pick this up early next week.
Comment hidden (off-topic) |
Comment 10•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #8) > (In reply to :Gijs Kruitbosch from comment #7) > > Can we workaround this by specifying the hello page in the panel / browser > > as sandboxed if we don't already? > > There's a privileged API that we have that we can extend to open a new tab > (or whatever), that should get us to do the right thing. I suspect Mike or > myself can pick this up early next week. I meant using the sandbox CSP directive... but it looks like that's bug 671389 and it's stalled.
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Updated•9 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
Points: --- → 2
Updated•9 years ago
|
Flags: sec-bounty?
Updated•9 years ago
|
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 39.3 - 30 Mar → ---
Comment 13•9 years ago
|
||
As a low rated security issue, this is not eligible for a bounty.
Flags: sec-bounty? → sec-bounty-
Updated•9 years ago
|
Priority: P1 → P4
Reporter | ||
Comment 14•9 years ago
|
||
So this is eligible for a bounty?
Comment 15•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #14) > So this is eligible for a bounty? See comment 13. This question was already answered.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → standard8
Iteration: --- → 40.2 - 27 Apr
Assignee | ||
Comment 16•9 years ago
|
||
This fixes us so that we use the backend rather than window.open. This avoids the exploit. I've tested the patch against the given exploit and it all works fine now - the panel stays the same.
Attachment #8595328 -
Flags: review?(mdeboer)
Comment 17•9 years ago
|
||
Comment on attachment 8595328 [details] [diff] [review] The fix Review of attachment 8595328 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, thanks! ::: browser/components/loop/MozLoopAPI.jsm @@ +709,5 @@ > > /** > + * Opens a URL in a new tab in the browser. > + * > + * @param {String} [url] The new url to open I don't think a url should be 'optional' here ;) ::: browser/components/loop/MozLoopService.jsm @@ +1652,5 @@ > > /** > + * Opens a URL in a new tab in the browser. > + * > + * @param {String} [url] The new url to open Optional? @@ +1656,5 @@ > + * @param {String} [url] The new url to open > + */ > + openURL: function(url) { > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + MozLoopService.log.warn("opening via backend", url); Hmm, I'm quite sure that this is 'debug' level info at most. ::: browser/components/loop/content/js/panel.jsx @@ +342,5 @@ > }, > > handleHelpEntry: function(event) { > event.preventDefault(); > + var helloSupportUrl = this.props.mozLoop.getLoopPref('support_url'); While you're here, can you change this to double quotes? ::: browser/components/loop/test/desktop-local/panel_test.js @@ +199,5 @@ > + })); > + > + expect(view.getDOMNode().querySelectorAll(".icon-account")) > + .to.have.length.of(0); > + }); nit: indentation
Attachment #8595328 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #17) > @@ +1656,5 @@ > > + MozLoopService.log.warn("opening via backend", url); > > Hmm, I'm quite sure that this is 'debug' level info at most. That's an oops I left it in ;-)
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b75551ab6fca
Target Milestone: --- → mozilla40
Assignee | ||
Comment 20•9 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b75551ab6fca
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8595328 [details] [diff] [review] The fix Approval Request Comment [Feature/regressing bug #]: Firefox Hello [User impact if declined]: Low risk of security issue via social engineering attack [Describe test coverage new/current, TreeHerder]: Landed in m-c, manual testing of links performed, some automated checks for unit test functions where possible. [Risks and why]: Low - only changing the way links are opened. [String/UUID change made/needed]: None
Attachment #8595328 -
Flags: approval-mozilla-beta?
Attachment #8595328 -
Flags: approval-mozilla-aurora?
Comment 22•9 years ago
|
||
Comment on attachment 8595328 [details] [diff] [review] The fix [Triage Comment] 38.0 is in m-r now should be in 38 beta 7.
Attachment #8595328 -
Flags: approval-mozilla-release+
Attachment #8595328 -
Flags: approval-mozilla-beta?
Attachment #8595328 -
Flags: approval-mozilla-aurora?
Attachment #8595328 -
Flags: approval-mozilla-aurora+
Comment 23•9 years ago
|
||
Why are we approving this on release but not beta?
Comment 24•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #23) > Why are we approving this on release but not beta? Because release will merge to beta. Long story. release will become 38, beta will become 38.0.5.
Reporter | ||
Comment 25•9 years ago
|
||
Can this be a CVE or a bounty>
Comment 26•9 years ago
|
||
(In reply to Muhammad Shahmeer from comment #25) > Can this be a CVE or a bounty> This was minused for bounty (see sec-bounty flag) because it is rated "sec-low" and is not much of a security issue. We generally pay bounties only for sec-high and sec-critical rated issues. See https://www.mozilla.org/en-US/security/bug-bounty/faq/#eligible-bugs for details.
Comment 27•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/532ce9f42266 (In reply to Al Billings [:abillings] from comment #23) > Why are we approving this on release but not beta? Don't worry, it'll get everywhere it needs to go :)
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → fixed
Flags: in-testsuite+
Comment 28•9 years ago
|
||
Also, this is the third time you've asked about a bounty and been told that it is not eligible. Please don't keep asking here. If you have further questions, email security@mozilla.org. This issue will be assigned a CVE by the time it ships in a version of Firefox.
Updated•9 years ago
|
Reporter | ||
Comment 29•9 years ago
|
||
Okay Sorry
Updated•9 years ago
|
status-firefox38.0.5:
--- → fixed
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•