Closed Bug 1199601 Opened 4 years ago Closed 4 years ago

Make formatValue more resilient to spoofing attacks

Categories

(Firefox :: Address Bar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main45-][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

Spinning this off from bug 1195976 where the immediate need is addressed by the patch for the core bug that resulted in a broken URL being passed to the location bar.

However, we just had another report (bug 1199430) where one of the impacts of another non-Firefox-UI bug also includes the location bar mis-highlighting the domain.

I think it's worth spending some time to make that code less easy to fool, to reduce the impact of such bugs in the future, even if it is "only" spoofing / sec-low.
Group: firefox-core-security
Keywords: sec-want
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8673686 - Flags: review?(dao)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8673686 [details] [diff] [review]
Patch v1.1

Redirecting here from bug 1246330.
Attachment #8673686 - Flags: review?(dao) → review?(mak77)
Comment on attachment 8673686 [details] [diff] [review]
Patch v1.1

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

please add tests, like "http://foo.bar?q=test", "http://foo.bar#test", "http://foo.bar?.mozilla.org", "http://foo.bar?@mozilla.org" to browser_urlHighlight.js
Those should be fine for the changes to the last group.

Though, you also want some test for the change to "(?:[^\/#?@]+@)?". I'm not completely sure what this is catching, the old regex seems to properly handle cases where the user:password part contains those chars, so I'd like to see examples of how that is preventing us to mismatch, first.
Attachment #8673686 - Flags: review?(mak77)
Blocks: 1246330
Attached patch Patch v1.2Splinter Review
Note that interestingly, one of your own examples, foo.bar?@mozilla.org, without the 'http', does not currently highlight at all correctly on Nightly (and continues to not do so without the change to (?:[^\/#?]+@)? ) - though I got rid of the extra @ in there which does not seem to have been correct (sorry, it's been 4 months since I wrote the patch, so I just had to re-verify what I was doing). I added a number of other tests to verify we do the right thing here. Thanks for bringing that up!
Attachment #8673686 - Attachment is obsolete: true
Attachment #8718760 - Flags: review?(mak77)
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

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

nice
Attachment #8718760 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/a60bad5487cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: it's possible for the wrong part of a URL to be highlighted in the URL bar
[Describe test coverage new/current, TreeHerder]: has tests!
[Risks and why]: pretty low, all we're doing is changing the regexp that deals with highlighting, for which there are pretty good unit tests already
[String/UUID change made/needed]: nope
Attachment #8718760 - Flags: approval-mozilla-beta?
Attachment #8718760 - Flags: approval-mozilla-aurora?
Group: firefox-core-security → core-security-release
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

Ah, regexp :p Should not be too risky, taking it.

Should be in 45 beta 7
Attachment #8718760 - Flags: approval-mozilla-beta?
Attachment #8718760 - Flags: approval-mozilla-beta+
Attachment #8718760 - Flags: approval-mozilla-aurora?
Attachment #8718760 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main45-]
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.