Closed
Bug 724599
(CVE-2012-1950)
Opened 13 years ago
Closed 13 years ago
[CAL-2012-0001]Spoof after a URL is dragged to location bar, by canceling the load
Categories
(Firefox :: Address Bar, defect)
Tracking
()
People
(Reporter: curtisk, Assigned: dao)
References
(Depends on 1 open bug)
Details
(Whiteboard: [sg:moderate][advisory-tracking+])
Attachments
(5 files, 3 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
image/png
|
Details | |
|
(deleted),
patch
|
neil
:
feedback+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
Screenshot
[CAL-2012-0001]firefox drag spoof vulnerability
1 Affected Products
=================
tested Firefox 9.0.1
tested Firefox 10.0
2 Vulnerability Details
=====================
Code Audit Labs of vulnhunt.com had discovered a vulnerability in Firefox 10.0 which does not properly handle drag and drop operations on URL strings, which allows user-assisted remote attackers to spoof the Omnibox.
this issue is also affect chrome . we also reported to chrome ,they are addressing the vulnerability now.
3 Analysis
=========
N/A
4 Crash info:
===============
N/A
5 POC:
====
<title>Drag spoof testcase by Code Audit Labs of VulnHunt.com</title>
<body draggable="true">
<h3><a href="http://www.google.com" id="a">Drag me to address bar</a></h3>
<h3 id="note" style="display:none;">I am not google at all</h3>
</body>
<script>
ondragend = function(){
stop();
document.title = 'Done';
a.style.display = 'none';
note.style.display = 'block';
}
</script>
6 About Code Audit Labs:
=====================
Code Audit Labs secure your software,provide Professional include source
code audit and binary code audit service.
Code Audit Labs:" You create value for customer,We protect your value"
http://www.VulnHunt.com
|
Reporter | |
Updated•13 years ago
|
status-firefox10:
--- → affected
|
||
Comment 1•13 years ago
|
||
|
|
||
Comment 2•13 years ago
|
||
Screenshot. The basic problem is that the URL gets dragged, but the page immediately cancels the load (via window.stop) and the location still displays the "typed in" google.com URL.
I tend to think that this doesn't need to remain private.
|
|
||
Updated•13 years ago
|
Component: General → Location Bar
Product: Core → Firefox
QA Contact: general → location.bar
|
|
||
Comment 3•13 years ago
|
||
I think that this should be classified as [sg:moderate] because 1) you can't spoof the domain identifier/larry, only the the text box 2) it requires some level of user interaction
Whiteboard: [sg:moderate]
|
Assignee | |
Comment 4•13 years ago
|
||
|
|
Assignee | |
Comment 5•13 years ago
|
||
This doesn't fail without the patch. ondragend never got called for my synthetic drag.
|
|
Assignee | |
Updated•13 years ago
|
Attachment #594983 -
Flags: feedback?(enndeakin)
|
||
Comment 6•13 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #4)
> Created attachment 594980 [details] [diff] [review]
> patch
This prevents the dragend handler from firing, but a site can still just use an interval that calls stop() repeatedly to achieve the same effect, can't it? Being able to detect the drag end isn't really the problem here, IMO.
It would be nice if web pages couldn't interrupt URL bar-triggered loads by calling stop(). That would probably be complicated to fix, though.
An easier fix might be ensuring that the stop() reverts the URL bar.
|
|
Assignee | |
Comment 7•13 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> This prevents the dragend handler from firing, but a site can still just use
> an interval that calls stop() repeatedly to achieve the same effect, can't
> it?
I tried to modify the test case to do this but couldn't get it to work.
If it worked, we could presumably prevent it with LOAD_FLAGS_STOP_CONTENT (in addition to suppressing dragend).
> An easier fix might be ensuring that the stop() reverts the URL bar.
I couldn't find a decent way to differentiate between user- and script-initiated stops.
|
|
||
Comment 8•13 years ago
|
||
Bug 725611 uses a different way of accomplishing the same thing via mousemove handlers and an onbeforeunload handler, so I think the only effective way to deal with this is going to be by changing the behavior on stop(), not dealing with the drag behavior.
|
|
||
Comment 9•13 years ago
|
||
http://gavinsharp.com/tmp/stopalot.html seems to work for me (essentially the same testcase with a setInterval(stop, 100) instead of dragend).
Why would we need to differentiate between the types of stops?
I can never remember how the userTypedClear/userTypedValue stuff actually works :(
|
|
||
Updated•13 years ago
|
Attachment #594980 -
Flags: review?(gavin.sharp) → review-
|
|
Assignee | |
Comment 10•13 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Why would we need to differentiate between the types of stops?
Because people may want to stop page loads without throwing away the URLs they just put there.
|
||
Comment 11•13 years ago
|
||
(In reply to Gavin Sharp from comment #9)
> I can never remember how the userTypedClear/userTypedValue stuff actually works :(
There's a nice comment in browser.xml that explains the minutiae, or did you want something more high-level?
|
|
||
Comment 12•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #11)
> There's a nice comment in browser.xml that explains the minutiae, or did you
> want something more high-level?
Maybe. The comment talks about different scenarios, but it's hard to associate the various cases to the actual code that increments/decrements userTypedClear (including the code in tabbrowser).
|
|
||
Comment 13•13 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #10)
> Because people may want to stop page loads without throwing away the URLs
> they just put there.
I'm not sure that this matters much in practice, but it should be relatively easy to add a STOP_USER_TRIGGERED flag to nsIWebNavigation and pass it from BrowserStop, right? Not sure how to best propagate that to the progress listener, though (another state flag, too? that seems kind of wrong...).
|
|
Assignee | |
Comment 14•13 years ago
|
||
I replaced isTabEmpty's empty-body check with an opener check, since the latter seems more accurate and safer and the former only happens to work because about:newtab doesn't have a body.
Attachment #594980 -
Attachment is obsolete: true
Attachment #594983 -
Attachment is obsolete: true
Attachment #594983 -
Flags: feedback?(enndeakin)
Attachment #596981 -
Flags: review?(gavin.sharp)
|
|
Assignee | |
Updated•13 years ago
|
tracking-firefox-esr10:
--- → ?
|
||
Updated•13 years ago
|
status-firefox-esr10:
--- → affected
status-firefox11:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
|
||
Comment 15•13 years ago
|
||
[Triage Comment]
This isn't ready to land yet, we're committed to sg:{critical,high} and this is sg:moderate - why would we take this for esr10? Please re-nominate if there's a good case for going outside the agreed upon process here. Thank you.
|
|
||
Updated•13 years ago
|
Attachment #596981 -
Flags: feedback?(neil)
|
|
||
Comment 16•13 years ago
|
||
Comment on attachment 596981 [details] [diff] [review]
let stop() revert to the current URI
Can you add a comment that mentions why you're using isTabEmpty? Something like "if the current tab is empty, leave the typed URL (but allow it to change)". Perhaps also worth adding a comment explaining that setting userTypedValue resets userTypedClear.
(Really wish we could rename userTypedClear to "disallowURLBarUpdating" or somesuch, but the compatibility hit probably isn't worth it)
Attachment #596981 -
Flags: review?(gavin.sharp) → review+
|
|
||
Comment 17•13 years ago
|
||
Shouldn't we be checking whether the request status was successful or failure?
|
|
Assignee | |
Comment 18•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #17)
> Shouldn't we be checking whether the request status was successful or
> failure?
Why? What case are you concerned about?
|
|
||
Comment 19•13 years ago
|
||
(In reply to Dão Gottwald from comment #18)
> (In reply to comment #17)
> > Shouldn't we be checking whether the request status was successful or
> > failure?
> Why? What case are you concerned about?
The case where the page refreshes automatically while you're trying to type a URL into the URL bar.
|
|
Assignee | |
Comment 20•13 years ago
|
||
now checking the request status
Attachment #596981 -
Attachment is obsolete: true
Attachment #596981 -
Flags: feedback?(neil)
Attachment #610035 -
Flags: feedback?(neil)
|
|
||
Comment 21•13 years ago
|
||
Comment on attachment 610035 [details] [diff] [review]
patch v2
>+ if (!Components.isSuccessCode(aRequest.status) &&
>+ !isTabEmpty(this.mTab)) {
>+ // Restore the current document's location in case the
>+ // request was stopped (possibly from a content script)
>+ // before the location changed.
>+
>+ this.mBrowser.userTypedValue = null;
f=me on this; from my point of view the rest of the patch is just padding.
Attachment #610035 -
Flags: feedback?(neil) → feedback+
|
|
Assignee | |
Comment 22•13 years ago
|
||
The "location bar reflects loaded page after stop()" test was failing, replacing aRequest.status with aStatus fixed it. I don't know what the former is, apparently it's not the same...
|
|
Assignee | |
Comment 23•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
|
|
||
Comment 25•13 years ago
|
||
Cancelling a load group fires the OnStopRequest before cancelling the request, which explains why the request doesn't have the "right" status yet. I'm not sure what's normally expected, for instance it also seems to be possible to fire an OnStopRequest with different error status results.
|
|
Assignee | |
Updated•13 years ago
|
Flags: in-testsuite+
|
||
Comment 26•13 years ago
|
||
[CC'ing Philip Chee so he can help SeaMonkey if we do need a port of this sec fix]
|
||
Updated•13 years ago
|
status-firefox14:
--- → fixed
|
||
Comment 27•13 years ago
|
||
Verified fixed in trunk with nightly Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120330 Firefox/14.0a1.
Status: RESOLVED → VERIFIED
|
|
||
Updated•13 years ago
|
|
||
Updated•13 years ago
|
Summary: [CAL-2012-0001]firefox drag spoof vulnerability → [CAL-2012-0001]Spoof after a URL is dragged to location bar, by canceling the load
|
|
||
Comment 28•13 years ago
|
||
If this rates being fixed on the ESR branch we ought to take it in 13+, no? Moving tracking 14+ back to nomination.
|
|
||
Comment 29•13 years ago
|
||
Is there some reason we shouldn't land this in Firefox 13 (June) rather than Firefox 14 (mid-July)? Please request beta approval.
tracking-firefox13:
--- → +
|
|
||
Comment 30•13 years ago
|
||
I am not inclined to land this in the late stages of beta given that it's sg:moderate and changes (crutfy) core browser UI progress listener code.
|
|
||
Comment 31•13 years ago
|
||
Are we tracking this for ESR 13+ still or should it be 14+?
|
|
||
Comment 32•13 years ago
|
||
This is 14+ at this point.
|
|
||
Comment 33•13 years ago
|
||
This is still marked Firefox 13 affected as well.
|
|
Assignee | |
Comment 34•13 years ago
|
||
It's too late to land this for Firefox 13.
|
|
||
Comment 35•12 years ago
|
||
Dao: can you prepare an ESR patch (if required) and then request ESR approval?
|
|
Assignee | |
Comment 36•12 years ago
|
||
|
|
Assignee | |
Comment 37•12 years ago
|
||
Attachment #640182 -
Flags: approval-mozilla-esr10?
|
|
||
Comment 38•12 years ago
|
||
Do we have an original reporter beyond "Code Audit Labs" for this?
Whiteboard: [sg:moderate] → [sg:moderate][advisory-tracking+]
|
|
||
Comment 39•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #37)
> Created attachment 640182 [details] [diff] [review]
> esr10 patch
When will this patch be landing. Lukas? Alex?
|
|
||
Comment 40•12 years ago
|
||
Comment on attachment 640182 [details] [diff] [review]
esr10 patch
Good to land. Thanks Dao.
Attachment #640182 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
|
|
Assignee | |
Comment 41•12 years ago
|
||
|
|
||
Updated•12 years ago
|
Alias: CVE-2012-1950
|
|
||
Updated•12 years ago
|
Group: core-security
|
||
Comment 42•12 years ago
|
||
Sorry for being a bit late, but verified fixed in ESR using 2012-09-09 Firefox 10.0.8esrpre.
You need to log in
before you can comment on or make changes to this bug.
Description
•