1151941 - Windows content sandbox with alternate winstation causes problems for common controls theme support

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: screenshot-low.jpg

See screenshots.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: screenshot-normal.jpg

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is an x64 nightly build, on windows 8.1.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And I don't see anything in the browser console log that looks relevant.

Comment 4

[Bob Owen (:bobowen)]

Looks like this was caused by the previous stronger policy that level 1 represented.

Not sure which of those previous settings caused the problem, so I'll just change this to block the main content sandbox tightening bug for now.

Comment 5

[Bob Owen (:bobowen)]

Looks like this is down to using an alternate desktop.

Comment 6

[Bob Owen (:bobowen)]

Hi Gijs, do you have any idea where this styling change gets implemented?

I'm struggling to find out why using an alternate desktop is blocking this.

Not sure if this is your area, but IIRC you were doing some styling stuff for Windows 10.

Comment 7

[:Gijs (he/him)]

That looks like widget level code to me... nsNativeTheme and moz-appearance implementations and all that jazz. Jim Mathies would know more than me if you need more details than that.

Comment 8

[Jim Mathies [:jimm]]

Looks like it's not a failure on individual widgets, more of a general failure. So the cause is going to be somewhere in here:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#2592

Might be that pres shell variable set up, or more likely it's in the GetTheme code - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#66

Maybe HWND shouldn't be null here? The docs don't say this is acceptable, but clearly it's working generally. Throw in virtual desktops and who knows.

Comment 9

[Bob Owen (:bobowen)]

This only appears to be a problem when we use an alternate winstation as well as desktop.

So we should be able to land the alternate desktop on Nightly.
I'll leave this bug for looking into using an alternate winstation.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

What is the value in the alternate window station? Does it provide a security boundary? Is there anything that stops a thread from doing SetProcessWindowStation(OpenWindowStation("Winsta0", FALSE, WINSTA_ALL_ACCESS)) ?

If there isn't, I'd say that the point is moot.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

The list of "access rights" from https://msdn.microsoft.com/en-us/library/windows/desktop/ms687391(v=vs.85).aspx seems to basically describe the security boundary winstations enforce.

Comment 12

[Bob Owen (:bobowen)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11)
> The list of "access rights" from
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms687391(v=vs.85).
> aspx seems to basically describe the security boundary winstations enforce.

Those are for access rights generally to do things to/with a particular Windows Station (or Windows Station owner) I think, not necessarily what running in that winstation provides.

I think the theming is probably broken because the alternate Windows Station is necessarily a non-interactive one.

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

It looks like the primary resources a WinStation has are a clipboard, and an atom table (not sure what this is tbh).

Comment 14

[Bob Owen (:bobowen)]

Attachment: Bug 1151941: Use an alternate winstation for the content process sandbox. r=cmartin!

Comment 15

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=578d1763a60ff34d64efdd309be13fbdf4d4ac52

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bba7c74dafae
Use an alternate winstation for the content process sandbox. r=cmartin

Comment 17

[Narcis Beleuzu [:NarcisB]]

Backed out for causing marionette test failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/3f25345c1002fd20b96788a696827f6dcf1d1886
Log link: https://treeherder.mozilla.org/logviewer?job_id=366342030&repo=autoland&lineNumber=39508

Comment 18

[Andreea Pavel [:apavel]]

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=0b12f1bb498a5f024341697b2163f725815b12d5&selectedTaskRun=buVmRzW4S1-vP-sy0KnWRQ.0

Comment 19

[Atila Butkovits]

[Remote] failure log: https://treeherder.mozilla.org/logviewer?job_id=366351135&repo=autoland

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13bebbfea591
Use an alternate winstation for the content process sandbox. r=cmartin

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/13bebbfea591