Closed Bug 802026 (CVE-2013-0759) Opened 12 years ago Closed 11 years ago

URL spoofing with credentials info of URL & 204

Categories

(Firefox :: Address Bar, defect)

16 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox17 --- wontfix
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr10 18+ verified
firefox-esr17 18+ verified
b2g18 --- fixed

People

(Reporter: masatokinugawa, Assigned: Gavin)

Details

(Keywords: csectype-spoof, sec-high, Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

1. Go to attachment file and click "go" button.
2. Address bar points "google.com". But in fact, it is not "google.com".



Expected results:

Firefox should display URL correctly.
Attachment #671741 - Attachment mime type: text/plain → text/html;charset=utf-8
Status: UNCONFIRMED → NEW
Component: Untriaged → Location Bar
Ever confirmed: true
We've had problems with navigation, 204's and the address bar in the past. Is the old bug back or is this new?
(In reply to Daniel Veditz [:dveditz] from comment #3)
> We've had problems with navigation, 204's and the address bar in the past.
> Is the old bug back or is this new?

I don't know if the UI redress issue is new; I also don't know if the technique for suppressing the warning dialog is known.
Component: Location Bar → Document Navigation
Product: Firefox → Core
So isn't this more like an UI issue. What I see is location bar which has 
http://www.google.com ...lots of white space... @l0.cm/fg.html
Daniel, you moved this from LocationBar component to DocumentNavigation. Any reason why?
Component: Document Navigation → Location Bar
Product: Core → Firefox
Attached patch patchSplinter Review
There's a simpler way to reproduce:
1) Load "http://spoof             @gavinsharp.com" (copy/paste from here, those are some fancy unicode spaces).
2) (Accept the warning)
3) Focus URL bar, press Esc.

Expected: URL stays the same
Actual: URL switches to the "spoofy" value

The underlying issue is that we're not calling createExposableURI when URLBarSetURI reverts the URL to the current URI. This happens in the case above, but also when finishing toolbar customization and, since bug 724599, when a load fails, as in this bug's testcase.

I searched the addons MXR for users of XULBrowserWindow._uriFixup - edilee's addon is the only one I found find:
https://mxr.mozilla.org/addons/source/310123/bootstrap.js#1236
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #691245 - Flags: review?(dao)
Mardak: see the last part of comment 7. You'll need to stop using XULBrowserWindow._uriFixup in AwesomeBar HD. I recommend just retrieving the service  yourself rather than using a random Firefox property.
OS: Windows Vista → All
Hardware: x86 → All
Attachment #691245 - Flags: review?(dao) → review+
Comment on attachment 691245 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Fairly easily, given the test. It's clear that we're touching spoofing-related functionality (display of the URL in the URL bar, stripping of the user/pass).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's clear what we're fixing, but probably not that clear how you would exploit this in practice.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch applies pretty much as-is to beta/aurora, very little uplift risk.

How likely is this patch to cause regressions; how much testing does it need?

I think it's unlikely to cause regressions - we're just being more consistent and applying existing URL bar display behavior to more cases. For beta/aurora, I can avoid the property removal that could potentially affect add-ons.
Attachment #691245 - Flags: sec-approval?
Adding Alex so we can discuss when we want to take this given that it affects branches and we're in the middle of the cycle.
Comment on attachment 691245 [details] [diff] [review]
patch

sec-approval+

For the branches--especially ESRs--we need a patch without the _uriFixup removal.
Attachment #691245 - Flags: sec-approval? → sec-approval+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: URL bar vulnerable to spoofing
Testing completed (on m-c, etc.): none yet
Risk to taking this patch (and alternatives if risky): relatively low risk. No add-on impact, and we're just making existing code used for displaying URLs in the URL bar apply more broadly.
String or UUID changes made by this patch: none
Attachment #692103 - Flags: approval-mozilla-esr17?
Attachment #692103 - Flags: approval-mozilla-beta?
Attachment #692103 - Flags: approval-mozilla-aurora?
Attached patch esr10 patch (obsolete) — Splinter Review
[Approval Request Comment]
(see comment 12)
Attachment #692106 - Flags: approval-mozilla-esr10?
Comment on attachment 692103 [details] [diff] [review]
beta/aurora patch

Approving on branches as its a low risk patch for a sec high bug and unlikely to cause regressions.
Attachment #692103 - Flags: approval-mozilla-esr17?
Attachment #692103 - Flags: approval-mozilla-esr17+
Attachment #692103 - Flags: approval-mozilla-beta?
Attachment #692103 - Flags: approval-mozilla-beta+
Attachment #692103 - Flags: approval-mozilla-aurora?
Attachment #692103 - Flags: approval-mozilla-aurora+
Attachment #692106 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment on attachment 692106 [details] [diff] [review]
esr10 patch

This patch is going to need an update, the test won't work as-is on ESR10.
Attachment #692106 - Attachment is obsolete: true
Comment on attachment 692103 [details] [diff] [review]
beta/aurora patch

This patch will also need a test update for ESR17. Notes to self:
- whenNewWindowLoaded doesn't exist on the ESRs
- whenDelayedStartupFinished doesn't exist in the right head.js on ESR10
- openToolbarCustomizationUI/closeToolbarCustomizationUI don't exist on ESR10
- pageloaderror may not be a valid test on esr10 (since it's pre-bug 724599)
I just removed the two non-working sub-tests ("customization" and "loading a new page)", leaving just the handleRevert test for ESR10.
Attachment #692664 - Flags: approval-mozilla-esr10+
Attached patch ESR17 patchSplinter Review
For ESR17, I just added whenNewWindowLoaded to head.js.
Attachment #692665 - Flags: approval-mozilla-esr17+
Attachment #692103 - Attachment description: beta/aurora/esr17 patch → beta/aurora patch
This is the followup I landed in comment 19 to work around bug 822056 (which I didn't catch locally, because I was testing before per-window PB got turned on).
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #23)
> Created attachment 692666 [details] [diff] [review]
> followup fix for trunk/beta

(I also rolled this in when I pushed the original patch to Aurora)
Sigh, the beta bustage was the same as the ESR bustage - whenNewWindowLoaded was not defined. I added it as with ESR-17:
https://hg.mozilla.org/releases/mozilla-beta/rev/ef57a0236dbd
This is still broken on Aurora.
Oops, somehow managed to forget to open an Aurora TBPL tab :( Too many branches.
https://hg.mozilla.org/releases/mozilla-aurora/rev/640b594b297f
Keywords: verifyme
Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]
Alias: CVE-2013-0759
Confirmed reproducible with Firefox 16.0.

Verified fixed with:
* Firefox 20.0a1 2013-01-04
* Firefox 19.0a2 2013-01-04
* Firefox 18.0b7
* Firefox 10.0.12esr build1
* Firefox 17.0.1esrpre 2013-01-03

Is this something we can put in testsuite?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Keywords: verifyme
QA Contact: anthony.s.hughes
Group: core-security
Comment on attachment 671741 [details]
spoof_credentials_and_204.html

><!DOCTYPE html>
><html>
><head>
><meta charset="utf-8">
></head>
><body>
>222
><script>
>function a(){
>	window.open('http://www.google.comãããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããããã@l0.cm/fg.html', 'x');
>	setTimeout(function(){
>		window.open('http://l0.cm/204', 'x');
>	}, 500);
>}
></script>
><input type="button" value="go" onclick="a()">
></body>
></html>
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.