1294413 - Potential address bar spoof using @title (or spoofing a "browser" message) for form validation popup

Status: NEW

(Firefox :: General, defect, P2)

Description

[Abdulrahman Alqabandi]

Attachment: input.html

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

Look at the PoC.html attached for a live example (though you might want to adjust the CSS depending on browser settings, this can be refined to work for default clean builds) 

What I did essentially is:

1. Set x-moz-errormessage='http://somewebsite"
2. Using CSS I set the input to go -XXpx from the top



Actual results:

The x-moz-errormessage covers the address bar


Expected results:

The x-moz-errormessage should be below the main window, IOW make the error message disappear if its CSS is set for it to go outside the main documents boundaries.

Comment 1

[Abdulrahman Alqabandi]

Attachment: Screenshot of the resulting spoof

Comment 2

[Abdulrahman Alqabandi]

Attachment: The real PoC

Opps wrong HTML uploaded

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Looks like a legit bug, but not very convincing as a spoof - therefore unhiding.

Comment 4

[Abdulrahman Alqabandi]

(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #3)
> Looks like a legit bug, but not very convincing as a spoof - therefore
> unhiding.

I guess I agree its a low spoof. But still could be used to display various messages (like 'This website is secure!') as well as hiding the url. 

I was thinking of this: https://www.mozilla.org/en-US/security/advisories/mfsa2016-52/

but I guess since its HTML that's covering the address bar its a more convincing spoof?

Comment 5

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Abdulrahman Alqabandi[test] from comment #4)

> but I guess since its HTML that's covering the address bar its a more
> convincing spoof?

Pretty much.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Abdulrahman Alqabandi[test] from comment #0)
> Created attachment 8780064 [details]
> input.html
> 
> User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36
> (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36
> 
> Steps to reproduce:
> 
> Look at the PoC.html attached for a live example (though you might want to
> adjust the CSS depending on browser settings, this can be refined to work
> for default clean builds) 
> 
> What I did essentially is:
> 
> 1. Set x-moz-errormessage='http://somewebsite"

This seems not necessary to reproduce this issue as long as "required" attribute has been assigned.

> 2. Using CSS I set the input to go -XXpx from the top
> 
> 
> 
> Actual results:
> 
> The x-moz-errormessage covers the address bar
> 
> 
> Expected results:
> 
> The x-moz-errormessage should be below the main window, IOW make the error
> message disappear if its CSS is set for it to go outside the main documents
> boundaries.

Comment 8

[Daniel Veditz [:dveditz]]

Repeating part of my bug 1439255 comment 2 here:

This error message UI violates the absolute rule that web content should NEVER appear in the chrome area. https://textslashplain.com/2017/01/14/the-line-of-death/  That little pointer tail is dangerous -- even if we make sure the message is inside the content area we need to make sure the tail doesn't cross into chrome. Kind of dangerous even if it starts as position 0 since users might assume that it's crossing over. Can we get rid of the tail? Can we pad it by 10px if its too close to the edge?

In this case the form is invisible. Should we be showing errors from invisible content? That's a tricky one though because it might be visble-but-ghostly or obscured with other overlapping content or a busy background. Probably should ignore this (as we do) and just worry about keeping the popup from escaping the content area.

x-moz-errormessage is non-standard and here we can see why it's dangerous. Can we kill it?

This bug is filed in "Layout: Form Controls" but I suspect this be front-end code (browser/modules/FormSubmitObserver.jsm).

Comment 9

[Jonathan Kingston [:jkt] he/him]

> x-moz-errormessage is non-standard and here we can see why it's dangerous. Can we kill it?

Yes! There appears no reason to keep this however it's important to state this won't fix some of the issue.

In the following method it shows other validation messages too:
https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/dom/html/nsIConstraintValidation.cpp#49

> and just worry about keeping the popup from escaping the content area.

Yeah this works for iframes too after trying the POC on codepen for example.

Also as it takes user focus this was a form of an evil trap, I was able to close tabs but it was preventing me scrolling iframes not owned by the POC.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Using the standard compliant setCustomValidity works too:

input.setCustomValidity("Argh")

Comment 11

[:Gijs (he/him)]

This popup is created/positioned in https://dxr.mozilla.org/mozilla-central/source/browser/modules/FormValidationHandler.jsm#113-117 . We should just bound the position in there so it's not allowed to 'escape' the confines of the browser rect.

Comment 12

[:Gijs (he/him)]

And to answer dveditz's question in bug 1439255 comment #4, the code lives in browser/modules, so even though this deals with form validation in principle it's a frontend bug to fix. :jkt, do you have cycles or do you want me to take this?

Comment 13

[Jonathan Kingston [:jkt] he/him]

I'm going to take a look now to see if I can do this, sorry I took so long to respond. Looks like you are still busy too, however will unassign if I can't finish it soon.

Comment 14

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1294413 - Bounding client validation popup to selected browser.

Review commit: https://reviewboard.mozilla.org/r/226482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226482/

Comment 15

[Jonathan Kingston [:jkt] he/him]

It looks like I need to fix date time picker too, I refactored the validation code to match the function in: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/toolkit/modules/DateTimePickerHelper.jsm#125

The POC doesn't work until the anchor position sits within the page.

The fix I think also prevents spamming the user when the window isn't visible which on Linux spans across workspaces even. However I need to check this further.

Looks ok in try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75251fb8769019fec4e28abcfe0f3e6bcf241b1b&selectedJob=166957414

:Gijs would you be able to check over the solution used (ignoring the missing datetime fix and tests) and I will follow up with the remainder on Tuesday.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

https://reviewboard.mozilla.org/r/226482/#review232412

::: browser/modules/FormValidationHandler.jsm:112
(Diff revision 1)
> +    let docRect = tabBrowser.selectedBrowser.getBoundingClientRect();
> +    let anchorRect = this._anchor.getBoundingClientRect();

These calls flush layout synchronously, which will cause jank. Can you use `nsIDOMWindowUtils` to use the lazy variants instead? I somewhat assume you can use the same left/top/width/height values from the `aPanelData` for the anchor's size and position, whereas the tabbrowser's data won't have changed since we last did a style + layout pass.

Comment 17

[Jonathan Kingston [:jkt] he/him]

Looks like Date picker needs the "FullZoomChange", "TextZoomChange", "ZoomChangeUsingMouseWheel" etc too I think this can be done as a follow up as it's limited spoofability and more just annoying in comparison.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226482/diff/1-2/

Comment 19

[:Gijs (he/him)]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

https://reviewboard.mozilla.org/r/226482/#review233422

::: browser/modules/FormValidationHandler.jsm:105
(Diff revision 2)
> -    this._panel.hidden = false;
> +    /* first ignore validation messages from non visible forms */
> +    if (Services.focus.activeWindow != window ||
> +        tabBrowser.selectedBrowser != aBrowser) {
> +      return;
> +    }

What ensures that we show the validation message if the window/tab becomes active later?

::: browser/modules/FormValidationHandler.jsm:121
(Diff revision 2)
> +    anchorRect.height = aPanelData.contentRect.height;
> +    anchorRect.width = aPanelData.contentRect.width;

Nit: These are only used by the next 2 lines, so just inline it there?

::: toolkit/modules/DateTimePickerHelper.jsm:141
(Diff revision 2)
> +    anchorRect.height = rect.height;
> +    anchorRect.width = rect.width;

Same comments here. And yeah, this looks like it wants to be just 1 file. Maybe file a follow-up if you haven't already.

::: toolkit/modules/DateTimePickerHelper.jsm:158
(Diff revision 2)
> +    this._anchor.left = rect.left;
> +    this._anchor.top = rect.top;
> +    this._anchor.width = rect.width;
> +    this._anchor.height = rect.height;

This must be broken because `rect` doesn't exist here, you want `anchorRect`. I'm confused that you missed this as well as any automated tests, apparently? (There's a bunch of orange on the trypush but it looks related to the other patch underneath this one - xpcshell won't pick this up, I expect...

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

https://reviewboard.mozilla.org/r/226482/#review233422

> This must be broken because `rect` doesn't exist here, you want `anchorRect`. I'm confused that you missed this as well as any automated tests, apparently? (There's a bunch of orange on the trypush but it looks related to the other patch underneath this one - xpcshell won't pick this up, I expect...

Yeah sorry I just meant to backup my work, I forgot to remove the r?. I still have to add some tests.

Comment 21

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226482/diff/2-3/

Comment 22

[:Gijs (he/him)]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

https://reviewboard.mozilla.org/r/226482/#review255448

Sorry, I should have picked up these comments in previous rounds of review. :-(

::: dom/html/test/browser_prevent_error_spoofing.js:3
(Diff revision 3)
> +function checkPopupShow() {
> +  ok(gInvalidFormPopup.state == "showing" || gInvalidFormPopup.state == "open",
> +     "[Test " + testId + "] The invalid form popup should be shown");
> +}

Nit: unused, it seems?

::: toolkit/modules/DateTimePickerHelper.jsm:101
(Diff revision 3)
> +  // Send close event to browser-content.js
> +  _sendCloseEvent(browser) {

Seems like we could do this in `_closePicker` based on an optional argument? Given that everything that invokes `_sendCloseEvent` also invokes `_closePicker()`, seems the logic should really be in there.

::: toolkit/modules/DateTimePickerHelper.jsm:146
(Diff revision 3)
> +    anchorRect.height = rect.height;
> +    anchorRect.width = rect.width;
> +    anchorRect.bottom = anchorRect.top + rect.height;
> +    anchorRect.right = anchorRect.left + rect.width;
> +
> +    /* if the anchor is outside of the document don't display */

I'm pretty confused by this. The summary of the commit says "bound" - what we've done for other cases where we don't want popups displayed outside of a given set of bounds (e.g. bug 1401477)  is to just change the bounds so that the popup *does* display, but at a position inside the bounds. In this case, we'd change the position of the anchor based on the bounds, not stop it displaying altogether.

Note that from this patch, it's also not obvious to me that this actually does what it says in cases where the form-validation or date/time popup *anchor* may be inside the bounds of the window, but the actual *popup* is then displayed outside it (e.g. because the popup displays above instead of below the failed-validation form element, etc.).

Comment 23

[Jonathan Kingston [:jkt] he/him]

Wow I 100% removed you this time from review! I again was just backing up work whilst I switched bookmark. The test is broken etc (for some reason the popup actually flickers outside the bottom of the window).

> is to just change the bounds so that the popup *does* display, but at a position inside the bounds. In this case, we'd change the position of the anchor based on the bounds, not stop it displaying altogether.

Ah this would simplify the code I thought about showing it again when the node was visible again.

Sorry for wasting your time and I appreciate the super quick review. I'll make sure I manually remove you next time when I'm doing this.

Comment 24

[Justin Dolske [:Dolske]]

Still working on this? Just making a pass through idle P1s. If not, please unassign and reset priority.

Comment 25

[Jonathan Kingston [:jkt] he/him]

Unassigning as not working on this. I think it's likely a p2 as it's a pretty bad usability and spoof bug.

Comment 26

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1294413 - Bounding client validation popup to selected browser.

Comment 27

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8957598 [details]
Bug 1294413 - Bounding client validation popup to selected browser.

Obsolete, I updated the patch to prevent bitrotting

Comment 28

[Matthew N. [:MattN]]

x-moz-errormessage was removed in bug 1513890 so I guess this should focus on the the date picker and other content popups.