1199601 - Make formatValue more resilient to spoofing attacks

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

[:Gijs (he/him)]

Attachment: Patch v1.1

Comment 2

[:Gijs (he/him)]

Comment on attachment 8673686 [details] [diff] [review]
Patch v1.1

Redirecting here from bug 1246330.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[:Gijs (he/him)]

Attachment: Patch v1.2

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!

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

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

nice

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a60bad5487cc

Comment 7

[:Gijs (he/him)]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/74f29f1fcc46

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10acc7fdcf09