1366818 - (CVE-2021-38508) Spoofing dialog and don't exit fullscreen mode

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Raphael]

Attachment: dialog.jpg

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.112 Safari/537.36

Steps to reproduce:

Open Poc
1.spoof.html.(Fake Message Box)
2.spoof2.html (Spoofing Notification window)
3.fullscreen.html (Don't exit fullscreen mode)


Actual results:

SetCustomValidity method displays a message on top of all Windows, under certain circumstances, this may lead to the substitution of the message browser

Comment 1

[Raphael]

Attachment: spoof(old_not work).html

Comment 2

[Raphael]

Comment on attachment 8870075 [details]
spoof(old_not work).html

<script>
function aaa() 
{
window.open('https://laraweron.mysit.ru/ff/2.html','palette_navig2','toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=no,resizable=no,copyhistory=no,width=1,height=1,screenX=400,screenY=1000' );
}
</script>
<a href="http://google.com" onclick='aaa();'>click me</a>

Comment 3

[Raphael]

Attachment: spoof2.html

Comment 4

[Raphael]

Attachment: fullscreen.html

Comment 5

[Raphael]

Attachment: spoof.html

Comment 6

[Raphael]

I believe that this is a security error

Comment 7

[Daniel Veditz [:dveditz]]

I'm bouncing this back to front-end. The "Core" back-end components seem to be handling things OK and just set state; it's the UX that's wrong for both examples. The popup is positioned and displayed from browser/modules/FormSubmitObserver.jsm

1) the popup is displayed relative to the form element, but no attempt is made to make sure the form element is visible or on-screen. Web Content must not be able to trigger Web-supplied text into a chrome element that can float outside the browser window. By positioning the element just off screen the message pointer bleeds into the browser chrome and can spoof a legit door hanger, or in this case try to cover up part of one of our own door hangers.

NB: the spoof2 example is not terribly convincing in Nightly, in part because we've changed our UI and things no longer line up. Ignore the sloppiness, assume that with more care this could be customized to each platform and version. Focus instead on the fact that a floating top-most element has escaped the "line of death" (https://textslashplain.com/2017/01/14/the-line-of-death/ )

2) the fullscreen example is annoying, but probably should be a separate bug. You can escape using hotkeys to switch to another tab, or Cmd-W to close the window. Why is this example able to eat the ESC that quits fullscreen, but not the others? In any case fullscreen content should never be able to evade ESC.

Gijs: what would be a better component for this bug?

Comment 8

[Raphael]

> NB: the spoof2 example is not terribly convincing in Nightly, in part
> because we've changed our UI and things no longer line up. 

Changed the code for version 55 Firefox Nightly

Comment 9

[Raphael]

Attachment: Nightly.jpg

Comment 10

[Raphael]

Attachment: spoof2(Nightly-55ver).html

Comment 11

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> I'm bouncing this back to front-end. The "Core" back-end components seem to
> be handling things OK and just set state; it's the UX that's wrong for both
> examples. The popup is positioned and displayed from
> browser/modules/FormSubmitObserver.jsm
> 
> 1) the popup is displayed relative to the form element, but no attempt is
> made to make sure the form element is visible or on-screen.

Actually, this isn't quite true - we do make an 'attempt' to check the form element is visible, see bug 1319078. I'm not sure why the focus manager thinks opacity:0 elements are focusable... we can add lazy bounds checking for the scroll position stuff, and/or constrain at what positions we allow the popup to open in the first place.

> Web Content must
> not be able to trigger Web-supplied text into a chrome element that can
> float outside the browser window.

This sounds like we should just stop showing custom validity error messages at all, then. We don't have in-browser-content-anchored popups.

> By positioning the element just off screen
> the message pointer bleeds into the browser chrome and can spoof a legit
> door hanger, or in this case try to cover up part of one of our own door
> hangers.

I don't think any interaction is possible with these form validity popups.

> NB: the spoof2 example is not terribly convincing in Nightly, in part
> because we've changed our UI and things no longer line up. Ignore the
> sloppiness, assume that with more care this could be customized to each
> platform and version. Focus instead on the fact that a floating top-most
> element has escaped the "line of death"
> (https://textslashplain.com/2017/01/14/the-line-of-death/ )
> 
> 2) the fullscreen example is annoying, but probably should be a separate
> bug.

Yes.

> You can escape using hotkeys to switch to another tab, or Cmd-W to
> close the window. Why is this example able to eat the ESC that quits
> fullscreen, but not the others? In any case fullscreen content should never
> be able to evade ESC.

The form submit popup eats escape to hide that popup, and it's reshown every 50ms because the form is submitted every 50ms.

IMO we should use the popup blocker (ie require user input) to control form submissions and/or the showing of these validity messages, but I don't know if that would break the web. What do other browsers that implement custom validity form submission checking do with this stuff?

> Gijs: what would be a better component for this bug?

Typically form validation stuff has lived in Core::DOM, judging by e.g. bug 1088761 and bug 1319078, so I think the original component was correct... anyway, :jdai, are you able to take a look at this? I can help with reviews etc., but I don't have cycles to dive into this in detail right now.

Comment 12

[Raphael]

In the 55th version of Firefox, the notification message about the geolocation is added to the alert, but there is no such word when using the function. (navigator.mediaDevices.getUserMedia)
I changed the code for version 55 Firefox.
Example: http://laraweron.mysit.ru/ff/ff_night.html
I assume that the function (navigator.mediaDevices.getUserMedia)  requires a corresponding message.

Comment 13

[Raphael]

Attachment: Nightly_2.jpg

Comment 14

[John Dai[:jdai]]

> > Gijs: what would be a better component for this bug?
> 
> Typically form validation stuff has lived in Core::DOM, judging by e.g. bug
> 1088761 and bug 1319078, so I think the original component was correct...
> anyway, :jdai, are you able to take a look at this? I can help with reviews
> etc., but I don't have cycles to dive into this in detail right now.

Sure. I would like to take this bug. I also set this bug to Core::DOM component. Feel free to reset component if anyone have concern.

Comment 15

[:Gijs (he/him)]

Re-adding the sec flag.

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to :Gijs from comment #11)
> I don't think any interaction is possible with these form validity popups.

No, but they can cover other elements and gain some authority by being able to appear anchored outside the content window. The screenshots here might be about the worst you can do: try to elicit clicks on some other permission prompt by "replacing" the text. It's not terribly convincing if you see these a lot (there's a drop shadow across the top of the buttons, plus the font/style is all wrong) but might fool a more casual user. Maybe put up a message "from the browser" about some emergency patch and when the user clicks open up a fake Mozilla site as part of a social engineering attack.

Comment 17

[John Dai[:jdai]]

After I tested other browsers behavior, Chrome and Safari have same behavior as firefox which means it'll keep showing popup window every small time period. The test is spoof2(Nightly-55ver).html.

Comment 18

[Raphael]

Attachment: Screenshot_2.jpg

After two years, this bug has not been fixed why ?
Test PoC https://karmaim.000webhostapp.com/2019.html

Comment 19

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

Thanks for comments and reporting the issue. We weren't getting to the bug as we were working on other critical or higher priority issues according to the triage result. Thank you for the understanding.

Comment 20

[John Dai[:jdai]]

(In reply to Raphael from comment #18)

Created attachment 9106012 [details]
Screenshot_2.jpg

After two years, this bug has not been fixed why ?
Test PoC https://karmaim.000webhostapp.com/2019.html

Dear reporter,

I'm fixing bug 1551758, I hope it can fix this issue. Thank you for your patient.

Comment 21

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #20)

(In reply to Raphael from comment #18)

Created attachment 9106012 [details]
Screenshot_2.jpg

After two years, this bug has not been fixed why ?
Test PoC https://karmaim.000webhostapp.com/2019.html

Dear reporter,

I'm fixing bug 1551758, I hope it can fix this issue. Thank you for your patient.

After fixed bug 1551758, It prevents keep showing popup window every small time period. However, it still show custom validity popups and cover part of door hanger in spoof2(Nightly-55ver).html.

(In reply to :Gijs (he/him) from comment #11)

(In reply to Daniel Veditz [:dveditz] from comment #7)

I'm bouncing this back to front-end. The "Core" back-end components seem to
be handling things OK and just set state; it's the UX that's wrong for both
examples. The popup is positioned and displayed from
browser/modules/FormSubmitObserver.jsm

1. the popup is displayed relative to the form element, but no attempt is
   made to make sure the form element is visible or on-screen.

Actually, this isn't quite true - we do make an 'attempt' to check the form
element is visible, see bug 1319078. I'm not sure why the focus manager
thinks opacity:0 elements are focusable... we can add lazy bounds checking
for the scroll position stuff, and/or constrain at what positions we allow
the popup to open in the first place.

The test spoof2(Nightly-55ver).html set opacity:1 at test() function. In bug 1319078, we did check form element visibility, however, we treat offscreen elements can be focused in here. AFAICK, focus manager are following as spec said for offscreen elements can be focused. According below comments, I think we should stop showing form validity error messages if the elements are not in the browser's viewport. Does that sounds make sense to you?

Web Content must
not be able to trigger Web-supplied text into a chrome element that can
float outside the browser window.

This sounds like we should just stop showing custom validity error messages
at all, then. We don't have in-browser-content-anchored popups.

By positioning the element just off screen
the message pointer bleeds into the browser chrome and can spoof a legit
door hanger, or in this case try to cover up part of one of our own door
hangers.

I don't think any interaction is possible with these form validity popups.

Comment 22

[:Gijs (he/him)]

(In reply to John Dai[:jdai] from comment #21)

(In reply to John Dai[:jdai] from comment #20)

(In reply to Raphael from comment #18)

Created attachment 9106012 [details]
Screenshot_2.jpg

After two years, this bug has not been fixed why ?
Test PoC https://karmaim.000webhostapp.com/2019.html

Dear reporter,

I'm fixing bug 1551758, I hope it can fix this issue. Thank you for your patient.

After fixed bug 1551758, It prevents keep showing popup window every small time period. However, it still show custom validity popups and cover part of door hanger in spoof2(Nightly-55ver).html.

(In reply to :Gijs (he/him) from comment #11)

(In reply to Daniel Veditz [:dveditz] from comment #7)

I'm bouncing this back to front-end. The "Core" back-end components seem to
be handling things OK and just set state; it's the UX that's wrong for both
examples. The popup is positioned and displayed from
browser/modules/FormSubmitObserver.jsm

1. the popup is displayed relative to the form element, but no attempt is
   made to make sure the form element is visible or on-screen.

Actually, this isn't quite true - we do make an 'attempt' to check the form
element is visible, see bug 1319078. I'm not sure why the focus manager
thinks opacity:0 elements are focusable... we can add lazy bounds checking
for the scroll position stuff, and/or constrain at what positions we allow
the popup to open in the first place.

The test spoof2(Nightly-55ver).html set opacity:1 at test() function. In bug 1319078, we did check form element visibility, however, we treat offscreen elements can be focused in here. AFAICK, focus manager are following as spec said for offscreen elements can be focused. According below comments, I think we should stop showing form validity error messages if the elements are not in the browser's viewport. Does that sounds make sense to you?

I think it makes sense not to show the validation error message if the element is outside the viewport. We can probably check this in the parent, and check that the point at which we're anchoring the validation popup is within the bounds of the frame for the originating browsing context.

However, we probably want to make sure that on a normal, non-bad-actor page, we scroll broken form elements that show the validity error into view? Maybe? Or are there spec reasons not to do that?

Comment 23

[John Dai[:jdai]]

(In reply to :Gijs (he/him) from comment #22)

I think it makes sense not to show the validation error message if the element is outside the viewport. We can probably check this in the parent, and check that the point at which we're anchoring the validation popup is within the bounds of the frame for the originating browsing context.

However, we probably want to make sure that on a normal, non-bad-actor page, we scroll broken form elements that show the validity error into view? Maybe? Or are there spec reasons not to do that?

The spec doesn't define UI behavior for validation popup and any focusable invalid fields will show the error outline. I expect we will show validation popup at first invalid element in the viewport, and show the error outline for focusable invalid elements which including outside the viewport's invalid elements.

Comment 24

[:Gijs (he/him)]

(In reply to John Dai[:jdai] from comment #23)

(In reply to :Gijs (he/him) from comment #22)

I think it makes sense not to show the validation error message if the element is outside the viewport. We can probably check this in the parent, and check that the point at which we're anchoring the validation popup is within the bounds of the frame for the originating browsing context.

However, we probably want to make sure that on a normal, non-bad-actor page, we scroll broken form elements that show the validity error into view? Maybe? Or are there spec reasons not to do that?

The spec doesn't define UI behavior for validation popup and any focusable invalid fields will show the error outline. I expect we will show validation popup at first invalid element in the viewport, and show the error outline for focusable invalid elements which including outside the viewport's invalid elements.

OK. Then yeah, this makes sense - are you able to work on this?

Comment 25

[John Dai[:jdai]]

(In reply to :Gijs (he/him) from comment #24)

(In reply to John Dai[:jdai] from comment #23)

(In reply to :Gijs (he/him) from comment #22)

I think it makes sense not to show the validation error message if the element is outside the viewport. We can probably check this in the parent, and check that the point at which we're anchoring the validation popup is within the bounds of the frame for the originating browsing context.

However, we probably want to make sure that on a normal, non-bad-actor page, we scroll broken form elements that show the validity error into view? Maybe? Or are there spec reasons not to do that?

The spec doesn't define UI behavior for validation popup and any focusable invalid fields will show the error outline. I expect we will show validation popup at first invalid element in the viewport, and show the error outline for focusable invalid elements which including outside the viewport's invalid elements.

OK. Then yeah, this makes sense - are you able to work on this?

Yes, I'm working on it. :-)

Comment 26

[John Dai[:jdai]]

Attachment: Bug 1366818 - Fix spoofing dialog for invalid form elements;

If the invalid form element offscreen, it won't display validation popup
for user.

Comment 27

[John Dai[:jdai]]

Attachment: spoof_cover_screenshot.png

I'm not very familiar with the door hanger, but basically, we should make the geolocation door hanger at the top of the window, otherwise, form invalid pop-up still can cover the geolocation door hanger. Maybe adding z-index to the door hanger to achieve it, but I could be wrong.

Comment 28

[:Gijs (he/him)]

(In reply to John Dai[:jdai] from comment #27)

Created attachment 9119114 [details]
spoof_cover_screenshot.png

I'm not very familiar with the door hanger, but basically we should make door hanger at the top of the window, otherwise, form invalid pop-up still can cover the door hanger. Maybe adding z-index to the door hanger to achieve it.

They're both popups, I don't know that we can specify a relative ordering. I don't think z-index comes into it, because they're different OS-level widgets - but I could well be wrong. Neil, do you know?

Comment 29

[:Gijs (he/him)]

We could also just make sure that form validation popups hide permission doorhangers and vice versa.

Comment 30

[John Dai[:jdai]]

Attachment: spoof_cover_chromium.png

(In reply to :Gijs (he/him) from comment #29)

We could also just make sure that form validation popups hide permission doorhangers and vice versa.

Upload chromium behavior for reference.

Comment 31

[(inactive) Nihanth Subramanya [:nhnt11]]

Gijs, Neil: Is it possible to enforce a rule where any doorhangers that are anchored to elements in web content are always displayed below doorhangers that are anchored to elements in browser chrome?

Comment 32

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #31)

Gijs, Neil: Is it possible to enforce a rule where any doorhangers that are anchored to elements in web content are always displayed below doorhangers that are anchored to elements in browser chrome?

The popups are in the parent and the anchor is in content; the popup manager has no idea we're "anchoring to content" because the parent process has no reference to the content process element. We just pass some coordinates from the content process and anchor on those in the parent. So no, I don't think we can do what you suggest in a 'general' sense, because the popup manager is not aware we're anchoring to a content element.

Note that in addition to the approach in the (current version of the) patch, we should also sanitize coordinates sent by the child in the parent to avoid displaying popups outside of the browser viewport, to avoid the same attack being possible from a compromised child process.

Comment 33

[Neil Deakin]

We don't have a mechanism to change the level of a popup while it is open. The workaround is to just close the popups and reopen them in the order you want. It might be possible to implement something that just calls nsIWidget::Show or something similar to what it does.

Comment 34

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Neil Deakin from comment #33)

We don't have a mechanism to change the level of a popup while it is open. The workaround is to just close the popups and reopen them in the order you want. It might be possible to implement something that just calls nsIWidget::Show or something similar to what it does.

Is it possible to set the level of a popup just before showing it? i.e. if we have a popup A already open, can we control whether a new popup B gets shown above or below it?

(In reply to :Gijs (he/him) from comment #32)

(In reply to Nihanth Subramanya [:nhnt11] from comment #31)

Gijs, Neil: Is it possible to enforce a rule where any doorhangers that are anchored to elements in web content are always displayed below doorhangers that are anchored to elements in browser chrome?

The popups are in the parent and the anchor is in content; the popup manager has no idea we're "anchoring to content" because the parent process has no reference to the content process element. We just pass some coordinates from the content process and anchor on those in the parent. So no, I don't think we can do what you suggest in a 'general' sense, because the popup manager is not aware we're anchoring to a content element.

Note that in addition to the approach in the (current version of the) patch, we should also sanitize coordinates sent by the child in the parent to avoid displaying popups outside of the browser viewport, to avoid the same attack being possible from a compromised child process.

Whoever is passing the coordinates should know though, right? Can we force consumers to declare their intent and rely on developers and reviewers to pass the right one?

Comment 35

[Neil Deakin]

Is it possible to set the level of a popup just before showing it? i.e. if we have a popup A already open, can we control whether a new popup B gets shown above or below it?

Well, one can always implement code that does this, or implement any window ordering behaviour. But since we only have a need to open a popup frontmost, we don't have any code that manipulates the ordering of popups, either before or after they have been opened.

Implementing this would require work on each platform's widget code.

Comment 36

[John Dai[:jdai]]

Attachment: Bug 1366818 - Make pop-up notification on top of the window;

This patch makes the door hanger pop-up on top of the window, form validation pop-up default set level by the parent so that we can make sure the door hanger will be top of form validation pop-up. Note: This patch only works well on MacOS, not for Linux now.

Comment 37

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

Hey, can you help us understand what the next step is here? Thanks!

Comment 38

[John Dai[:jdai]]

Hi Emilio,
Per comment 36, I made the notification pop-up at the top of the window on macOS so that the form validation pop-up will under notification pop-up, however, it requires some work on each platform. Is it possible to support at Layout? If not, maybe comment 33 is another way to approach. Thank you.

Comment 39

[Emilio Cobos Álvarez (:emilio)]

So is the issue that gtk widgets don't honor level="top"? That's not really a layout issue, it's a Widget :: Gtk issue.

I'm happy to poke at it if necessary, though it may be worth pinging the folks with more GTK background (Martin Stransky, Karl Tomlinson) first.

Also, does your patch work on Windows?

It's not clear to me what concrete thing you want to do with comment 33? We could theoretically support dynamic switching of popup levels, maybe. At least I think in GTK it is possible.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Also, level="top" may ignore other windows that aren't Firefox, is that what you want?

Comment 41

[:Gijs (he/him)]

How hard would it be to reimplement the form validation popup and display it in the child instead, using the same layers / anon content as the devtools inspector & co (so it can't be interfered with by web content)? This would also fix the positioning issues we've had with this generally, would mean it automatically goes away if the document unloads, etc. It'd also make it easier for this code to live in the child instead of requiring IPC at all... It could also potentially live more in C++ land which may be more comfortable for the DOM folks who've been working on this?

Comment 42

[John Dai[:jdai]]

Attachment: spoof_cover_apply_patch_on_windows.png

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39)

So is the issue that gtk widgets don't honor level="top"? That's not really a layout issue, it's a Widget :: Gtk issue.

Yes, it seems the level="top" can't work on Linux platform.

I'm happy to poke at it if necessary, though it may be worth pinging the folks with more GTK background (Martin Stransky, Karl Tomlinson) first.

Also, does your patch work on Windows?

Yes, it work on windows, please check the attachment file.

It's not clear to me what concrete thing you want to do with comment 33? We could theoretically support dynamic switching of popup levels, maybe. At least I think in GTK it is possible.

Comment 33 seems to remember and recognize all the pop-up so that it can close all the pop-up then open the notification pop-up at front. I will redirect to GTK forks. Thank you.

Hi Karl,
Base on GTK background, do you think comment 33, comment 38 or comment 41 is more approachable? Thank you.

Comment 43

[Karl Tomlinson (:karlt)]

Comment 41 sounds like the preferred solution to me. Pop-ups are only needed when there is a desire to extend some display beyond the bounds of the browser main window or above other windows. Each of those are non-goals for the prompt message.

Having content messages in a pop-up blurs the boundaries of chrome and content. Tooltips are also popups containing content messages, but I feel uncomfortable about them also.

I don't know how approachable the solution of comment 41 is due to existing dependence on existing infrastructure, but it would be worth checking and comment 41 seems to indicate that it has been achieved before. (I'm not clear that moving to C++ would be an advantage though.)

Having form validation messages implemented in the child would mean they would not look like native pop-ups, but that may be an advantage to clarify the chrome/content boundary.

I'm not clear on the distinction between comment 33 and comment 38.
Closing and re-opening windows (comment 33) depends on having a list of unrelated popups and would lead to some flickering. Essentially it is a way to implement level="top" (comment 38). There may be a better way, but I don't know because this is not something that is normally required.

Pop-ups are usually either transient (tooltips) or have focus and only one window has focus at a time, so that it can be dismissed with Esc, for example. If there is more than one pop-up, then they are related and have a hierarchy so that it is clear which has focus.
By only showing pop-ups when the app has focus, apps don't compete with each other to show the foremost window.

Here, both pop-ups are within the same app, but the same issues with competing for attention are involved, and I assume Esc does not dismiss one of the pop-ups. How would the user know which popup Esc would dismiss?

It is also a bug if content can steal focus without user action.

If comment 41 is not an option, then, rather than trying to set z-levels for pop-ups, I'd suggest ensuring that popups are dismissed when they (or their parent widgets) lose focus.

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Comment 41 would also fix some long-standing issues with these popups, like these sticking around when scrolling and so on.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Isn't comment 41 really hard because of same issues we've had with <select size=1> popup, when the document causing the popup is some cross-origin iframe. iframe which is triggering the popup maybe really small and using parent document's area to show the popup would be cross-origin information leak.

Comment 46

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #45)

Isn't comment 41 really hard because of same issues we've had with <select size=1> popup, when the document causing the popup is some cross-origin iframe. iframe which is triggering the popup maybe really small and using parent document's area to show the popup would be cross-origin information leak.

My understanding is that the highlighter code from the inspector uses native anon content and therefore isn't web-observable, so I wouldn't have thought so (ie even if we inserted in the parent document)? I mean, there might be mouseenter events or such, but I would not expect that to be any worse than with the chrome overlay we have now...

Comment 47

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #45)

Isn't comment 41 really hard because of same issues we've had with <select size=1> popup, when the document causing the popup is some cross-origin iframe. iframe which is triggering the popup maybe really small and using parent document's area to show the popup would be cross-origin information leak.

But <select> has to render outside of the viewport of the iframe to be usable. Maybe we don't have the same constraint with form popups. They're less critical IMHO.

My understanding is that the highlighter code from the inspector uses native anon content and therefore isn't web-observable, so I wouldn't have thought so (ie even if we inserted in the parent document)? I mean, there might be mouseenter events or such, but I would not expect that to be any worse than with the chrome overlay we have now...

In presence of spectre I think you can pretty much assume that every data that goes through the process can be observed, even if not trivially.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #47)

But <select> has to render outside of the viewport of the iframe to be usable. Maybe we don't have the same constraint with form popups. They're less critical IMHO.

Really? How would we position the popup in an iframe which is very small? Or not show them at all? That would be rather breaking change.
(When <select> was discussed, it was suggested that small iframes with <select> would be rare, but it took like 5 minutes to find examples. So I have no particular trust that there aren't small iframes which trigger popups)

Comment 49

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #48)

Really? How would we position the popup in an iframe which is very small? Or not show them at all? That would be rather breaking change.
(When <select> was discussed, it was suggested that small iframes with <select> would be rare, but it took like 5 minutes to find examples. So I have no particular trust that there aren't small iframes which trigger popups)

I assume it'd just get clipped by the viewport of the iframe. Select is different because you need to see the popup to interact with it, and the popup can get much larger than the form validation popups.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

So basically the popup wouldn't be shown at all if we want to show also the relevant form control.

Comment 51

[Neha Kochar [:neha]]

John, are you actively working on this sec issue?

Comment 52

[John Dai[:jdai]]

(In reply to Neha Kochar [:neha] from comment #51)

John, are you actively working on this sec issue?

No, I'm not working on this sec issue.

Comment 53

[Neha Kochar [:neha]]

Jens, could you pls find another assignee for this if jdai isn't planning to work on this?

Comment 54

[Daniel Veditz [:dveditz]]

(In reply to Karl Tomlinson (:karlt) from comment #43)

Comment 41 sounds like the preferred solution to me. Pop-ups are only needed when there is a desire to extend some display beyond the bounds of the browser main window or above other windows. Each of those are non-goals for the prompt message.

Anti-goals, even.

Keep content-generated messages in content, like we've done with javascript alerts/prompts. We've long talked about the "line of death" in browser UI, but it's a "plane of death", too. If it web-generated content floats up into a new OS plane it becomes confusable with browser UI, or can be used to hide legit browser UI (as here). Looks cool, but any feature like it ends up causing us no end of problems.

Comment 55

[Jens Stutte [:jstutte]]

Hi Olli, Emilio, can you briefly re-state what is the desired behavior now here and what should be implemented? From reading the comments I am not having the impression that we reached consensus already, but I might just not have enough context.

Comment 56

[Emilio Cobos Álvarez (:emilio)]

So there are multiple things that could be done here. Tweaking the z-index of the form message panel might be the easiest. Others like rendering them on the child etc are a bit trickier, as Olli noted in comment 50, and would probably at least need UX input.

I suspect Gijs is probably in a better position to assess the different trade-offs here, though he's on PTO... Perhaps Johann would have feedback?

Comment 57

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #56)

So there are multiple things that could be done here. Tweaking the z-index of the form message panel might be the easiest.

This isn't really that easy, I think? It's a panel right now, so it's not in the same widget level window as the main window, which is why it appears on top of everything else. To make it lower z-index than the full screen warning we'd either need a bunch of work on panel widget code (see comment 35), or have it no longer be a panel, instead absolutely positioning something on top of web content that lives inside the same widget window. I don't know how likely that is to cause issues; in the past there was an effort to split the browser up into different "zones" for web rendering / gfx's benefit, but I think that project didn't end up going anywhere. I don't know if making similar work harder is a problem. In this case we'd also need to re-implement its dismissal ourselves (ie hide it when other stuff gets focus or escape is pressed, which right now the panel code deals with for us).

Others like rendering them on the child etc are a bit trickier, as Olli noted in comment 50, and would probably at least need UX input.

I'm a little confused. Why can't we show them using the toplevel child? Yes, this might leak information to the top frame from the inner frame in the presence of spectre/meltdown; are there reasonable cases where this is problematic, the inner frame doesn't want to / can't use X-Frame-Options or similar, and the presence/absence of a form validation popup would be quite that interesting to the toplevel child? I'm struggling to imagine such a situation, especially as the toplevel likely can't engineer a situation where such a validation popup would appear for the inner frame (ie that'd depend on user input and/or the behaviour of the prospective victim site), and even where it does, its wording is likely not predictable. But either way, it doesn't really have any UX implications that way, right? (ni for this)

I suspect Gijs is probably in a better position to assess the different trade-offs here, though he's on PTO... Perhaps Johann would have feedback?

Other options that would help here would be not showing form validation warnings if the form is auto-submitted (no user interaction), or not re-showing the warning for the same document / browsing context more than once (but it may be tricky to keep track of that information).

Ultimately I think anything other than a band-aid is likely to need significant re-architecturing work, which does also mean it's more important we pick the right solution, which I guess is why this bug hasn't progressed much - it's hard to determine which solution is least-bad.

In terms of band-aids, we could add code to the form validation popup that checks for other open popups (checking with querySelectorAll + checking the popups' state properties) before allowing it to open, and/or hides itself if other panels/popups open (by listening for popupshowing events). Does that seem like a reasonable enough fix here? (ni for this also) Because perhaps then we should just opt for that rather than spend more time debating the pros and cons of the various options to completely redo the implementation...

Comment 58

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #57)

Others like rendering them on the child etc are a bit trickier, as Olli noted in comment 50, and would probably at least need UX input.

I'm a little confused. Why can't we show them using the toplevel child? Yes, this might leak information to the top frame from the inner frame in the presence of spectre/meltdown; are there reasonable cases where this is problematic, the inner frame doesn't want to / can't use X-Frame-Options or similar, and the presence/absence of a form validation popup would be quite that interesting to the toplevel child? I'm struggling to imagine such a situation, especially as the toplevel likely can't engineer a situation where such a validation popup would appear for the inner frame (ie that'd depend on user input and/or the behaviour of the prospective victim site), and even where it does, its wording is likely not predictable. But either way, it doesn't really have any UX implications that way, right? (ni for this)

One could argue that on a small window a custom validation message might want to overflow the content area... But that's an issue as soon as we get away from <panel>s. I don't think the space there is a huge issue if we're willing to accept that, though I don't know if that would be acceptable security-wise.

I suspect Gijs is probably in a better position to assess the different trade-offs here, though he's on PTO... Perhaps Johann would have feedback?

Other options that would help here would be not showing form validation warnings if the form is auto-submitted (no user interaction), or not re-showing the warning for the same document / browsing context more than once (but it may be tricky to keep track of that information).

The user-interaction bit at least seems relatively straight forward. We have a HasValidUserGestureActivation on the window context.

Ultimately I think anything other than a band-aid is likely to need significant re-architecturing work, which does also mean it's more important we pick the right solution, which I guess is why this bug hasn't progressed much - it's hard to determine which solution is least-bad.

Nod.

In terms of band-aids, we could add code to the form validation popup that checks for other open popups (checking with querySelectorAll + checking the popups' state properties) before allowing it to open, and/or hides itself if other panels/popups open (by listening for popupshowing events). Does that seem like a reasonable enough fix here? (ni for this also) Because perhaps then we should just opt for that rather than spend more time debating the pros and cons of the various options to completely redo the implementation...

There may be multiple form validation popups, that's one of the tricky cases I guess. The popup manager does keep track of all the open popups so we could use that... I guess it depends on how urgently should we fix this, but not opposed to such a band-aid.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

That band-aid could in practice work rather well.

Comment 60

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #58)

(In reply to :Gijs (he/him) from comment #57)

In terms of band-aids, we could add code to the form validation popup that checks for other open popups (checking with querySelectorAll + checking the popups' state properties) before allowing it to open, and/or hides itself if other panels/popups open (by listening for popupshowing events). Does that seem like a reasonable enough fix here? (ni for this also) Because perhaps then we should just opt for that rather than spend more time debating the pros and cons of the various options to completely redo the implementation...

There may be multiple form validation popups, that's one of the tricky cases I guess. The popup manager does keep track of all the open popups so we could use that... I guess it depends on how urgently should we fix this, but not opposed to such a band-aid.

There's only 1 popup per parent browser window, and I guess I would expect that switching parent/browser windows will hide popups in the other window anyway. This appears to work for me, so the WIP patch I'm about to put up doesn't do anything explicit to deal with that case.

Comment 61

[:Gijs (he/him)]

Attachment: Bug 1366818, r?emilio

Comment 62

[:Gijs (he/him)]

Attachment: Bug 1366818 - part 2, r?emilio

Comment 63

[Raphael]

Vulnerability fixed, what's next?

Comment 64

[:Gijs (he/him)]

(In reply to Raphael from comment #63)

Vulnerability fixed, what's next?

Bug 1726621 may have helped a bit, but the problem in attachment 8870079 [details] is not fixed yet, AFAIK. I need to update the patches that I wrote here, add automated tests, and then land them - but I've been out for most of the past 7 weeks and I haven't gotten around to it.

Comment 65

[:Gijs (he/him)]

I filed bug 1735327 for requiring user action for form validation UI to be shown (ie the current "part 2" patch here that I'll likely abandon - I'm not sure I'll find time to put it in 1735327, as doing this "properly" will also involve updating all the automated tests for this code...).

I'll work on getting part 1 ready to land in this bug, to address the sec issue.

Comment 66

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

r=emilio
https://hg.mozilla.org/integration/autoland/rev/69569fc1cf6f4c84ea80903aa056bb7c2f3d25c6
https://hg.mozilla.org/mozilla-central/rev/69569fc1cf6f

Comment 67

[:Gijs (he/him)]

Comment on attachment 9236535 [details]
Bug 1366818, r?emilio

Beta/Release Uplift Approval Request

• User impact if declined: sec-moderate issue (spoofing of browser messaging)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open attachment 8870300 [details]. The form validation popup with custom text should not appear persistently at the same time as the geolocation permission popup.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straightforward JS only changes to form validation panels opening/hiding, which have pre-existing automated tests (I'm adding a test for this issue too but it's not landing until we've landed this fix on supported branches or decided not to).
• String changes made/needed: Nope

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate
• User impact if declined: See above
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above
• String or UUID changes made by this patch: Nope

Comment 68

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 69

[Bogdan Maris, Desktop Test Engineering]

Reproduced the issue with the the custom text pop-up being displayed on top of the geolocation permission pop-up using an old Nightly build from 2021-10-11. Verified that using latest Nightly build across platforms (Windows 10, Ubuntu 18.04 and macOS 11.6) this does not happen anymore. The text pop-up is displayed after the geolocation one is dismissed.

Comment 70

[Raphael]

I am sincerely glad that after five years the error has been fixed. The expiration date of the error reward has not expired yet ?

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9236535 [details]
Bug 1366818, r?emilio

Approved for 94.0b7.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0abafedb0d37

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9236535 [details]
Bug 1366818, r?emilio

Approved for 91.3esr.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/41aba950cd86

Comment 75

[Bogdan Maris, Desktop Test Engineering]

Also verified that this is also fixed on 94.0b7 and latest esr91 build from treeherder across platforms (Windows 10, Ubuntu 18.04 and macOS 11.6).

Comment 77

[Tom Ritter [:tjr]]

Attachment: advisory.txt