1480572 - Address bar spoof in long IP URLs

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Abdulrahman Alqabandi]

Attachment: index.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180802100128

Steps to reproduce:

Host the attached PoC then open it but use the IP of the host. If you hosted on localhost:8000 then open 'http://127.0.0.1/index.html'. You will notice the spoof.


Actual results:

The long url leads to showing the middle/end part of the URL rather than making sure the hostname (ip in this instance) is visible.

Currently the PoC covers the address bar with 'Q', with a bit more work we could make any URL appear instead of the Q's. 


Expected results:

Keep focusing on the IP. If you test this on a non-ip URL; You will end up focusing on the domain clearly visible. This doesn't happen for IP's for some reason.

Tested on latest nightly 63.0a1 (2018-08-02) (64-bit)

Comment 1

[Abdulrahman Alqabandi]

Correction: When hosting on localhost:8000, go to '127.0.0.1:8000/index.html' (forgot the port)

Comment 2

[:Gijs (he/him)]

Does this only affect nightly, not release/beta? If so, I expect this is a dupe of bug 1483122.

Comment 3

[Abdulrahman Alqabandi]

(In reply to :Gijs (he/him) from comment #2)
> Does this only affect nightly, not release/beta?


Does not work on latest stable 61.0.2 (64-bit), did not test beta. 



>  If so, I expect this is a dupe of bug 1483122.


Yes it is.

Comment 4

[:Gijs (he/him)]

Doesn't look like the fix in that bug is fixing this bug, so let's keep both open.

Comment 5

[Drew Willcoxon :adw]

Is this really related to bug 1419391 (and the follow-up bug 1483122)?  This problem existed before that bug was fixed, right?  Was bug 1419391 supposed to address it?  Just trying to understand.

Comment 6

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #5)
> Is this really related to bug 1419391 (and the follow-up bug 1483122)?  This
> problem existed before that bug was fixed, right?

I doubt it [ie that the problem used to exist], given 61 is not affected, and I don't think 62 is affected, either.

>  Was bug 1419391 supposed
> to address it?  Just trying to understand.

I think it was supposed to not impact it. Having looked at the code again, we only set scrollLeft if we get an RTL domain, but we don't explicitly set it to 0 for other domains (or perhaps we do and I can't find the code where we do at 11.30pm at night, which is perhaps equally likely...

Comment 7

[:Gijs (he/him)]

Figured out what's going on here, patch shortly.

Comment 8

[:Gijs (he/him)]

Dan, because this only affects nightly, can I make this public once I have an r+'d patch so I can just land with lando?

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1480572 - actually check for RTL-ness of URL's domain, not just non-LTR-ness, r?adw

WindowUtils.getDirectionFromText returns a constant (DIRECTION_LTR, DIRECTION_RTL or DIRECTION_NOT_SET)
and although the _LTR value is 0, we can't just assume a non-0 value means RTL, as IP addresses aren't
going to have a directionality, because they consist of only latin digits and '.'s, which don't have
strong directionality.

Comment 10

[Daniel Holbert [:dholbert]]

I'm not the Dan you were asking, but I figured I'd drive-by reply:

(In reply to :Gijs (he/him) from comment #8)
> Dan, because this only affects nightly, can I make this public once I have
> an r+'d patch so I can just land with lando?

Quoting https://wiki.mozilla.org/Security/Bug_Approval_Process :
> Core-security bug fixes should just be landed by a developer without any explicit approval if:
> [...]
> B) The bug is a recent regression on mozilla-central. This means
>    - A specific regressing check-in has been identified
>    - The developer can (and has) marked the status flags for ESR, Beta, and Aurora as "unaffected"
>    - We have not shipped this vulnerability in anything other than a nightly build

It doesn't look like we've documented the first two bullet points, but I imagine you can do that before landing, Gijs.

With that, it seems to me like we should keep this hidden and land on inbound (if lando would require making the bug page public), so that we can keep this hidden until Nightly users have received the fix in a day or so and preserve the "don't publish security bugs / testcases until we've shipped the fix to affected users" best-practice.

Comment 11

[:Gijs (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Quoting https://wiki.mozilla.org/Security/Bug_Approval_Process :

Yeah, I'm familiar with this, which is sort of why I asked...

> With that, it seems to me like we should keep this hidden and land on
> inbound (if lando would require making the bug page public), so that we can
> keep this hidden until Nightly users have received the fix in a day or so
> and preserve the "don't publish security bugs / testcases until we've
> shipped the fix to affected users" best-practice.

I guess I don't think the combination of severity * affected user population mean there's much point keeping this hidden once we're landing a patch.


Really, I suppose the real thing I want here is some work on Lando to auto-censor confidential bugs into:

Bug N, r=<reviewer-list>

and no other commit message -- instead of just not working (which yes, is the current state today - you can't land sec-sensitive bugs with Lando, AIUI). I get the Lando folks are working on more important things, but maybe I can contribute a fix... Will look into that tomorrow.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9003310 [details]
Bug 1480572 - actually check for RTL-ness of URL's domain, not just non-LTR-ness, r?adw

:Felipe Gomes (needinfo me!) has approved the revision.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to :Gijs (he/him) from comment #11)
> I guess I don't think the combination of severity * affected user population
> mean there's much point keeping this hidden once we're landing a patch.

If it was guaranteed that the patch would make it to users within ~12 hours of landing, then I'd agree -- but it's also worth considering the (nonzero) possibility that the landing might get backed out due to testfailures or some other unforseen issue, in which case there may be several days of unforseen lag time before we can sort that out & re-land.

In that kind of scenario, we'd be quite happy to have kept the discussion/PoC testcases/etc. hidden beyond the first landing-attempt.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 9003310 [details]
Bug 1480572 - actually check for RTL-ness of URL's domain, not just non-LTR-ness, r?adw

Drew Willcoxon :adw has approved the revision.

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7cfbc863e6dd839f31c8d1b4cca3c04b254f5c

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7e7cfbc863e6

Comment 17

[Anca Soncutean, Desktop QA]

I’ve managed to reproduce this bug with the steps provided in bug’s description on Nightly 63.0a1 (20180802100128) with Windows 10 x64. 
The issue is fixed on the latest Firefox Beta 63.0b12 (20181004174654), LTR build and RTL build, with Windows 10 x64, Mac OS 10.11 and Ubuntu 16.04 x64.