1018988 - Change windows environment variables when running in a low integrity process.

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Bob Owen (:bobowen)]

When starting a low integrity process or changing a process to low integrity, we should amend the environment variables used for creating temporary files, so that they are created in a low integrity directory.

This should definitely include TMP and TEMP, probably APPDATA and LOCALAPPDATA and maybe others.

If this is done later on in the process we may have to invalidate any caches that already have these directories stored.

Comment 1

[Bob Owen (:bobowen)]

Here's a try push for a very rough first patch:
https://tbpl.mozilla.org/?tree=Try&rev=43d99508d846

I've just dumped some code in after the call to start the sandbox, but as hoped, providing a low integrity temp directory gives us some green test suites. \o/

This is just setting the TmpD property in the directory service at the moment.
So I need to:
* Move the code to get the LocalAppDataLow (.../AppData/LocalLow) directory into the directory service, as this already uses SHGetKnownFolderPath.
  Actually maybe the directory service should own the name of the temp directory as well and return the full path.
* Set TEMP and TMP env vars, I don't think I should worry about others at the moment.
* Only make the change when the policy is actually INTEGRITY_LEVEL_LOW.
* Decide where this code should really live.

Comment 2

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=7f44f6a0e764

Comment 3

[Bob Owen (:bobowen)]

Attachment: Set up a low integrity temp directory when using the Windows content sandbox.

After starting the Windows content sandbox creates a new directory under AppData\LocalLow\Mozilla called MozTemp-{<uuid>} and changes the directory service TmpD directory to this.
Also updates TEMP and TMP environment variables.

During process shutdown adds a call into ContentProcess::CleanUp to delete the temp directory that was created.

Going to look at cleaning up directories that get left by crashes in a follow-up bug.

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8484987 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox.

Nathan, thanks for looking at this.

Tim, there aren't many actually sandboxing changes in here, but I want to make sure you're happy overall.
Particularly given that fact that I am assuming that the delayed integrity level is low.
It's a pain to get the integrity level in xul.dll with the dual linking that we still have.
I think the comment in sandboxBroker.cpp serves the purpose just as well.

Just need to find someone for the ContentChild/Process changes now. :)

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8484987 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox.

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

::: xpcom/io/SpecialSystemDirectory.cpp
@@ +787,5 @@
> +        return rv;
> +      }
> +
> +      nsID uuid;
> +      rv = uuidgen->GenerateUUIDInPlace(&uuid);

Should you be caching whatever file you create here, since the UUID will be different everytime you call into the directory service?

Also, given that there's no good way to cache stuff in SpecialSystemDirectory, perhaps all this code should move into the directory service, and the cached nsIFile can just become a member variable of the service.

::: xpcom/io/nsDirectoryServiceDefs.h
@@ +130,5 @@
>    #define NS_WIN_DOCUMENTS_DIR                "Docs"
>    #define NS_WIN_PICTURES_DIR                 "Pict"
>    #define NS_WIN_MUSIC_DIR                    "Music"
>    #define NS_WIN_VIDEOS_DIR                   "Vids"
> +  #define NS_WIN_LOW_INTEGRITY_TEMP           "LowTmpD"

This definition needs a separate comment, since the comment above about save-to locations is not valid for this definition.

Comment 6

[Bob Owen (:bobowen)]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Comment on attachment 8484987 [details] [diff] [review]

> > +      rv = uuidgen->GenerateUUIDInPlace(&uuid);
> 
> Should you be caching whatever file you create here, since the UUID will be
> different everytime you call into the directory service?
> 
> Also, given that there's no good way to cache stuff in
> SpecialSystemDirectory, perhaps all this code should move into the directory
> service, and the cached nsIFile can just become a member variable of the
> service.

nsDirectoryService::Get already has a caching mechanism for the files.
It would only give you a different directory if GetSpecialSystemDirectory were to be called directly for this file ID.
I suppose I should make that clear in a comment in GetSpecialSystemDirectory and above the #define for Win_LowIntegrityTemp in SpecialSystemDirectory.h.
 
> ::: xpcom/io/nsDirectoryServiceDefs.h
> @@ +130,5 @@
> >    #define NS_WIN_DOCUMENTS_DIR                "Docs"
> >    #define NS_WIN_PICTURES_DIR                 "Pict"
> >    #define NS_WIN_MUSIC_DIR                    "Music"
> >    #define NS_WIN_VIDEOS_DIR                   "Vids"
> > +  #define NS_WIN_LOW_INTEGRITY_TEMP           "LowTmpD"
> 
> This definition needs a separate comment, since the comment above about
> save-to locations is not valid for this definition.

Ah yes, thanks I'll fix that.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Bob Owen (:bobowen) from comment #6)
> (In reply to Nathan Froyd (:froydnj) from comment #5)
> > Comment on attachment 8484987 [details] [diff] [review]
> 
> > > +      rv = uuidgen->GenerateUUIDInPlace(&uuid);
> > 
> > Should you be caching whatever file you create here, since the UUID will be
> > different everytime you call into the directory service?
> > 
> > Also, given that there's no good way to cache stuff in
> > SpecialSystemDirectory, perhaps all this code should move into the directory
> > service, and the cached nsIFile can just become a member variable of the
> > service.
> 
> nsDirectoryService::Get already has a caching mechanism for the files.

Ah, so it does.  My fault for not looking far enough!

> It would only give you a different directory if GetSpecialSystemDirectory
> were to be called directly for this file ID.
> I suppose I should make that clear in a comment in GetSpecialSystemDirectory
> and above the #define for Win_LowIntegrityTemp in SpecialSystemDirectory.h.

WDYT about simply moving all the code that's currently in SpecialSystemDirectory.{h,cpp} to nsDirectoryService.cpp?  That way we remove the calling-GetSpecialSystemDirectory-twice footgun.

Actually, hm.  nsDirectoryService is extendable by JS in the form of providers.  Do we have to worry about a malicious addon getting to register the LowTmpD directory before we do?  Or is trying to protect the sandbox from malicious addons out of scope?

Comment 8

[Bob Owen (:bobowen)]

(In reply to Nathan Froyd (:froydnj) from comment #7)

> WDYT about simply moving all the code that's currently in
> SpecialSystemDirectory.{h,cpp} to nsDirectoryService.cpp?  That way we
> remove the calling-GetSpecialSystemDirectory-twice footgun.

Maybe ... SpecialSystemDirectory already has the GetKnownFolder function, so I'd need to add that or something similar to nsDirectoryService.
Although it looks like there is a widget/windows/WinUtils I can use that I'd not seen before, so maybe that won't be too painful.

> Actually, hm.  nsDirectoryService is extendable by JS in the form of
> providers.  Do we have to worry about a malicious addon getting to register
> the LowTmpD directory before we do?  Or is trying to protect the sandbox
> from malicious addons out of scope?

I'll have to look into that, thanks for spotting this.
On first thought I'm hoping that the standard provider is always first, but if the JS can set things directly that would be a problem.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Bob Owen (:bobowen) from comment #8)
> (In reply to Nathan Froyd (:froydnj) from comment #7)
> 
> > WDYT about simply moving all the code that's currently in
> > SpecialSystemDirectory.{h,cpp} to nsDirectoryService.cpp?  That way we
> > remove the calling-GetSpecialSystemDirectory-twice footgun.
> 
> Maybe ... SpecialSystemDirectory already has the GetKnownFolder function, so
> I'd need to add that or something similar to nsDirectoryService.

Ah, that would be a roadblock, then.

> > Actually, hm.  nsDirectoryService is extendable by JS in the form of
> > providers.  Do we have to worry about a malicious addon getting to register
> > the LowTmpD directory before we do?  Or is trying to protect the sandbox
> > from malicious addons out of scope?
> 
> I'll have to look into that, thanks for spotting this.
> On first thought I'm hoping that the standard provider is always first, but
> if the JS can set things directly that would be a problem.

Yeah, the directory service is QI'able to nsIProperties, which permits clients to set key/value directly.  So something can twiddle with things even without registering as a provider.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Set up a low integrity temp directory when using the Windows content sandbox. v2

(In reply to Nathan Froyd (:froydnj) from comment #9)

> > Maybe ... SpecialSystemDirectory already has the GetKnownFolder function, so
> > I'd need to add that or something similar to nsDirectoryService.
> 
> Ah, that would be a roadblock, then.

I was just being dim on Friday evening, I've added just the get of LocalAppDataLow to SpecialSystemDirectory, which is where it belongs and uses GetKnownFolder.
(I've added this to nsDirectoryService as well just to match the existing pattern.)

I've then moved the bit that tacks on "Mozilla" (actually using MOZ_USER_DIR now) and the temp directory into nsDirectoryService.
 
> Yeah, the directory service is QI'able to nsIProperties, which permits
> clients to set key/value directly.  So something can twiddle with things
> even without registering as a provider.

Once I've spoken to a couple of people about it, if this causes a problem, I'm going to handle this in a follow-up.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8485697 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox. v2

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

This looks good.  Did you mean to have Tim review this too?

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8485697 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox. v2

(In reply to Nathan Froyd (:froydnj) from comment #11)

> This looks good.  Did you mean to have Tim review this too?

Thanks and yes, but I thought I'd check you were happy with my changes, so as to not spam Tim with another review cancellation. :)

Comment 13

[Bob Owen (:bobowen)]

Comment on attachment 8485697 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox. v2

Adding an r? for mrbkap for the ContentChild/Process changes ... thanks Blake.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8485697 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox. v2

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

Exciting stuff! Glad to see us getting more tests passing!

::: dom/ipc/ContentChild.cpp
@@ +928,5 @@
> +static void
> +SetUpSandboxEnvironment()
> +{
> +    // Set up a low integrity temp directory. This only makes sense if the
> +    // delayed integrity level for the content process is INTEGRITY_LEVEL_LOW.

Do we know if it is possible to use this directory if the integrity level is INTEGRITY_LEVEL_UNTRUSTED?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8485697 [details] [diff] [review]
Set up a low integrity temp directory when using the Windows content sandbox. v2

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

::: dom/ipc/ContentChild.cpp
@@ +944,5 @@
> +        return;
> +    }
> +
> +    // Undefine returns a failure if the property is not already set.
> +    (void) directoryService->Undefine(NS_OS_TEMP_DIR);

|unused <<| would be nicer.

Comment 16

[Bob Owen (:bobowen)]

Attachment: Set up a low integrity temp directory when using the Windows content sandbox. v3

r=froydnj - from comment 11 for xpcom/io/*
r=tabraldes - from comment 14 for security/sandbox/* and in general
r=mrbkap - from comment 15 for dom/ipc/*

Thanks to everyone for the reviews.
Here's a try push with this latest patch:
https://tbpl.mozilla.org/?tree=Try&rev=3f737b193361

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #14)

> Do we know if it is possible to use this directory if the integrity level is
> INTEGRITY_LEVEL_UNTRUSTED?

No, I don't think you're allowed write access to anywhere then, not sure about read.

I think the plan is to move all write file access from the content process eventually and then we would be able to move to UNTRUSTED, at least from a filesystem point of view.
So, this may only be a temporary solution, but at least it moves us forwards.

It also controls access to other things including things like the registry and COM interfaces.
I think it is this COM interfaces restriction that is causing us problems with some media tests.

(In reply to Blake Kaplan (on vacation 9/9-9/23) from comment #15)

> > +    (void) directoryService->Undefine(NS_OS_TEMP_DIR);
> 
> |unused <<| would be nicer.

Much nicer, thanks.
I hadn't seen this before.

It was a good job I had to make a change because this patch didn't compile.
I'd removed the include of nsIUUIDGenerator.h from SpecialSystemDirectory.cpp thinking that I'd forgotten to remove it after copying it to nsDirectoryService.cpp.
Actually I hadn't moved it at all and it was only compiling before because of unified sources.
I've now added it to nsDirectoryService.cpp.

Comment 17

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=3f737b193361

Thanks.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db0bf3d919b0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db0bf3d919b0