Closed Bug 1369670 Opened 7 years ago Closed 7 years ago

Blank pages are printed with security.sandbox.content.level set to 3 when Users folder is a junction point

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- disabled
firefox56 --- verified

People

(Reporter: bmaris, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

(Whiteboard: sbwc2)

Attachments

(2 files, 1 obsolete file)

Part 2: On Windows resolve junction points and symlinks in any paths that are used for sandbox policy rules
7.64 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Part 1: Don't rely on the case of the profile not changing in test_capabilities.py
3.08 KB, patch
whimboo
: review+
Details | Diff | Splinter Review 
[Affected versions]:
- Latest Nightly 55.0a1

[Affected platforms]:
- Windows 7 64bit

[Steps to reproduce]:
1. Make Windows Users directory on junction point
2. Install Firefox and open it
3. Change `security.sandbox.content.level` value to `3`
4. Visit http://mozqa.com/data/firefox/images/mozilla_logo.jpg
(I also reproduced with a .gif, .txt, .html, .svg, .png etc)
5. Print the page

[Expected result]:
- Image is printed successfully

[Actual result]:
- Nothing is printed on paper

[Regression range]:
- Not sure if this is a regression, could not find a bug where this value of the pref was first introduced. Also the nature of the steps would mean that I should try and manually find a regression which is very time consuming.

[Additional notes]:
- Not sure if Windows 10 is also affected we had trouble setting up the environment on the machine that runs it.
- Not entirely sure if this is the right component for this issue, please change it if necessary.
- With pref security.sandbox.content.level value set to `2`, the image or anything I print is successfully printed on paper.
This is down to the fact that the chromium sandbox doesn't allow rules that contain reparse points (symbolic links, junction points, etc.).

I need to write a utility function to resolve these.
I'll probably need to change the paths in the directory service as well, to make sure the content process is using the same path as in the rule.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Whiteboard: sbwc2
Summary: Blank pages are printed with security.sandbox.content.level set to 3 → Blank pages are printed with security.sandbox.content.level set to 3 when Users folder is a junction point
As part of the fix for this I'm having to resolve the profile path to remove any junction points and symlinks.
This also results in potential case changes and is causing marionette test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=385e685658ee0e213cd9fa1c405c2c06cb984bab

I think this is the assertion at [1]

Looks like I'll need to change these tests somewhat.

whimboo - what are these tests trying to prove?

[1] https://hg.mozilla.org/mozilla-central/file/035c25bef7b5/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py#l60
Flags: needinfo?(hskupin)
(In reply to Bob Owen (:bobowen) from comment #3)

> what are these tests trying to prove?

They are returning the profile path as part of the capabilities when a
new session is started with the remote protocol.

It looks to me like the path is correct, but that this is down to a
Python Unicode string comparison problem.  The first string is u"foo"
and the second is "foo" (non-Unicode).
(In reply to Andreas Tolfsen ‹:ato› from comment #4)
> (In reply to Bob Owen (:bobowen) from comment #3)
> 
> > what are these tests trying to prove?
> 
> They are returning the profile path as part of the capabilities when a
> new session is started with the remote protocol.

Thanks, I (sort of) understand what the assertion is doing.
What I want to understand is why that assertion is there? i.e. what is it trying to prove by comparing the two paths?
Is it trying to prove that Firefox honours the profile passed on the command line (or however it is passed)?

> It looks to me like the path is correct, but that this is down to a
> Python Unicode string comparison problem.  The first string is u"foo"
> and the second is "foo" (non-Unicode).

I'd be slightly surprised if my change had affected encoding, I thought it was probably because the resolved path has the casing as provided by the win32 API call and the one we're comparing it against (self.caps["moz:profile"]) is all lower case.
As Andreas already set, looks like an encoding issue? But hard to say because the code changes are not visible anywhere which might give an indication.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #6)
> As Andreas already set, looks like an encoding issue? But hard to say
> because the code changes are not visible anywhere which might give an
> indication.

If I transform the resolved path back to lower case the test passes, so it definitely seems to be a case issue not an encoding one.
(In fact if the profile happened to be on a path with a symlink or junction point then with my change it won't just be case that would cause this to fail.)

I think the problem is that this check (and possibly the one below it) assume that the profile directory will exactly match the one passed in.
I'm not sure that is valid, although it happens to have worked in the past.

I'm still not clear on what we are trying to test here, do we need marionette to be testing the -profile parameter?
If we want to keep the tests, could I change them to do a lowercase check (or better still a proper same file check)?
Flags: needinfo?(hskupin)
Can you please explain why case changes can happen here? I mean for those platforms which are about the case, changing the case of the profile path would result in path which does not exist. I think I need more information to give a statement here. Sorry.
Flags: needinfo?(hskupin) → needinfo?(bobowencode)
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Can you please explain why case changes can happen here? I mean for those
> platforms which are about the case, changing the case of the profile path
> would result in path which does not exist. I think I need more information
> to give a statement here. Sorry.

That's true on linux and Mac, but not on Windows, which is case insensitive as far as file paths are concerned.

If I use os.path.normcase (which does nothing on posix) as part of the check it works.

In theory if the path contained symlinks or junction points then we would still fail as the path would be completely different (although still pointing to the same directory). Luckily this doesn't happen on automation.
So, it would be nice if I could use os.path.realpath or even os.path.samefile, but realpath doesn't work properly in python 2 and samefile isn't available at all until python 3.2.
Flags: needinfo?(bobowencode)
This uses os.path.normcase to fix the current issue on automation.
Ideally we would use os.path.samefile but that's not available in python 2.
os.path.realpath is implemented in python 2, but it doesn't resolve symlinks and junction points
and doesn't normalise case.
Attachment #8879860 - Flags: review?(hskupin)
Sorry for the late reply. So in mozprofile we create a new profile folder with this line:

> self.profile = tempfile.mkdtemp(suffix='.mozrunner')

Which means that tempfile always seems to lowercase the path on Windows. 

I think as long as we can work with the profile path as returned via the capabilities on the Python side eg. for accessing files, I'm ok with just updating this test. Otherwise we would have to ensure to really return a valid path via the capabilities.
Comment on attachment 8879860 [details] [diff] [review]
Part 1: don't rely on the case of the profile not changing in test_capabilities.py

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

Please fix the import order. Otherwise this looks fine. Thanks.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ +5,5 @@
>  from marionette_driver.errors import SessionNotCreatedException
>  
>  from marionette_harness import MarionetteTestCase
>  
> +import os.path

Global imports have to be put first. Please move it up.
Attachment #8879860 - Flags: review?(hskupin) → review+
Comment on attachment 8879861 [details] [diff] [review]
Part 2: On Windows resolve junction points and symlinks in any paths that are used for sandbox policy rules

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

::: toolkit/xre/nsXREDirProvider.cpp
@@ +132,5 @@
> +  // If the mXULAppDir is different it lives below the mGREDir. To avoid
> +  // confusion resolve that as well even though we don't need it for sandbox
> +  // rules. Some tests rely on this for example.
> +  if (!mozilla::widget::WinUtils::ResolveJunctionPointsAndSymLinks(mXULAppDir)) {
> +    NS_WARNING("Failed to resolve XUL App Dir.");

wondering why you used warnings here vs something like a release assert or not reached? is this expected for a GRE dir?

::: widget/windows/WinUtils.h
@@ +472,5 @@
>     *         false if something failed or aPath does not exist, aPath will
>     *               remain unchanged.
>     */
>    static bool ResolveJunctionPointsAndSymLinks(std::wstring& aPath);
> +  static bool ResolveJunctionPointsAndSymLinks(nsIFile* aPath);

Pleased add a comment header here explaining what this routine does.
Attachment #8879861 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #15)
> ::: widget/windows/WinUtils.h
> @@ +472,5 @@
> >     *         false if something failed or aPath does not exist, aPath will
> >     *               remain unchanged.
> >     */
> >    static bool ResolveJunctionPointsAndSymLinks(std::wstring& aPath);
> > +  static bool ResolveJunctionPointsAndSymLinks(nsIFile* aPath);
> 
> Pleased add a comment header here explaining what this routine does.

nm, splinter cut the existing header off sufficiently that I didn't notice.
Thanks jimm

(In reply to Jim Mathies [:jimm] from comment #15)
...
> > +  // If the mXULAppDir is different it lives below the mGREDir. To avoid
> > +  // confusion resolve that as well even though we don't need it for sandbox
> > +  // rules. Some tests rely on this for example.
> > +  if (!mozilla::widget::WinUtils::ResolveJunctionPointsAndSymLinks(mXULAppDir)) {
> > +    NS_WARNING("Failed to resolve XUL App Dir.");
> 
> wondering why you used warnings here vs something like a release assert or
> not reached? is this expected for a GRE dir?

Even if we can't add the rules it doesn't always mean things won't work, so I don't want to crash.
I hope failures are pretty rare anyway.
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b7d6dbc698
Part 1: Don't rely on the case of the profile not changing in test_capabilities.py. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/46db8ae423f2
Part 2: On Windows resolve junction points and symlinks in any paths that are used for sandbox policy rules. r=jimm
(In reply to Wes Kocher (:KWierso) from comment #19)
> Backed these out for failures in test_capabilities.py
> https://treeherder.mozilla.org/logviewer.html#?job_id=109624887&repo=mozilla-
> inbound

Does Firefox not return the full folder name of the user?

> 'c:\\users\\cltbld.t-w864-ix-328.001\\appdata\\local\\temp\\tmp5lifu7.mozrunner' != 'c:\\users\\cltbld~1.001\\appdata\\local\\temp\\tmp5lifu7.mozrunner'
(In reply to Henrik Skupin (:whimboo) from comment #20)
> (In reply to Wes Kocher (:KWierso) from comment #19)
> > Backed these out for failures in test_capabilities.py
> > https://treeherder.mozilla.org/logviewer.html#?job_id=109624887&repo=mozilla-
> > inbound
> 
> Does Firefox not return the full folder name of the user?
> 
> > 'c:\\users\\cltbld.t-w864-ix-328.001\\appdata\\local\\temp\\tmp5lifu7.mozrunner' != 'c:\\users\\cltbld~1.001\\appdata\\local\\temp\\tmp5lifu7.mozrunner'

I think Firefox is returning the full name, it's the build machine that seems to be configured with the short name for the user's dir.

Looks like I'm going to have to resolve fully for the test.
Flags: needinfo?(bobowencode)
This uses os.path.normcase to fix the current issue on automation.
Ideally we would use os.path.samefile but that's not available in python 2.
os.path.realpath is implemented in python 2, but it doesn't resolve symlinks and junction points
and doesn't normalise case.
Attachment #8881793 - Flags: review?(hskupin)
Attachment #8879860 - Attachment is obsolete: true
Comment on attachment 8881793 [details] [diff] [review]
Part 1: Don't rely on the case of the profile not changing in test_capabilities.py

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

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
@@ +8,5 @@
>  
> +# Unlike python 3, python 2 doesn't have a proper implementation of realpath or
> +# samefile for Windows. However this function, which does exactly what we want,
> +# was added to python 2 to fix an issue with tcl installations and symlinks.
> +from FixTk import convert_path

I wonder if should be better a part of mozprofile, because it's the place where we store the path of the new created profile. It's because the same issue would exist for any other harness using this mozbase package.
(In reply to Henrik Skupin (:whimboo) from comment #24)
> Comment on attachment 8881793 [details] [diff] [review]
> Part 1: Don't rely on the case of the profile not changing in
> test_capabilities.py
> 
> Review of attachment 8881793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py
> @@ +8,5 @@
> >  
> > +# Unlike python 3, python 2 doesn't have a proper implementation of realpath or
> > +# samefile for Windows. However this function, which does exactly what we want,
> > +# was added to python 2 to fix an issue with tcl installations and symlinks.
> > +from FixTk import convert_path
> 
> I wonder if should be better a part of mozprofile, because it's the place
> where we store the path of the new created profile. It's because the same
> issue would exist for any other harness using this mozbase package.

I guess we could, but there is nothing actually wrong with the path, just the assumption that the representation in Firefox itself will be identical.
I'd also feel more uncomfortable using this function from some weird place by default and not just for the test.
(In reply to Bob Owen (:bobowen) from comment #25) 
> > I wonder if should be better a part of mozprofile, because it's the place
> > where we store the path of the new created profile. It's because the same
> > issue would exist for any other harness using this mozbase package.
> 
> I guess we could, but there is nothing actually wrong with the path, just
> the assumption that the representation in Firefox itself will be identical.
> I'd also feel more uncomfortable using this function from some weird place
> by default and not just for the test.

Ok, I'm fine with the current implementation. But would you mind filing a new bug under testing/mozbase to get this fixed in mozprofile itself? Thanks.
Comment on attachment 8881793 [details] [diff] [review]
Part 1: Don't rely on the case of the profile not changing in test_capabilities.py

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

Please update the commit message for the latest changes. With that r=me.
Attachment #8881793 - Flags: review?(hskupin) → review+
Blocks: 1376900
(In reply to Henrik Skupin (:whimboo) from comment #27)
> Comment on attachment 8881793 [details] [diff] [review]
> Part 1: Don't rely on the case of the profile not changing in
> test_capabilities.py
> 
> Review of attachment 8881793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update the commit message for the latest changes. With that r=me.

Thanks, yes I noticed that just after I uploaded it. :-)

I've filed bug 1376900 for the follow-up.
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9120668e5a82
Part 1: Resolve to the real path names when comparing the profile paths in test_capabilities.py. r=whimboo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1983642406a
Part 2: On Windows resolve junction points and symlinks in any paths that are used for sandbox policy rules. r=jimm
https://hg.mozilla.org/mozilla-central/rev/9120668e5a82
https://hg.mozilla.org/mozilla-central/rev/c1983642406a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
we don't enable level 3 sandbox in beta55
status-firefox55: affected → disabled
The issue is no longer reproducible on Firefox 56.0a1(2017-07-06).
Tests were performed under Windows 7x64, with the security.sandbox.content.level set to 3 and with Users folder as a junction point.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Depends on: 1513172
No longer depends on: 1513172
Regressions: 1513172
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: