1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Bug 1256541 changed this to return the standard temp directory when not running at low integrity / write access disabled.

This directory gets deleted on shutdown regardless (in case the pref has been changed), so we have to either revert that change or add conditions to the directory deletion.

Comment 1

[Bob Owen (:bobowen)]

Or perhaps we should just change the base directory, but always create the Mozilla\Temp-{fooid} and then we're still fine to delete.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Yeah, looks like we need to be smarter about this. Leaving ni? me to take another look.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Based on what we discussed in today's sandboxing standup, I'm going to make NS_APP_CONTENT_PROCESS_TEMP_DIR fallible and add a new entry for an infallible temp directory to be used by the crash reporter.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1d80e3a97e4

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: MozReview Request: Bug 1270018: Separate NS_APP_CONTENT_PROCESS_TEMP_DIR into fallible and infallible variants; r?bsmedberg

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

Comment 6

[Benjamin Smedberg]

Comment on attachment 8753469 [details]
MozReview Request: Bug 1270018: Separate NS_APP_CONTENT_PROCESS_TEMP_DIR into fallible and infallible variants; r?bsmedberg

https://reviewboard.mozilla.org/r/53302/#review50374

::: xpcom/io/nsAppDirectoryServiceDefs.h:92
(Diff revision 1)
>  #if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> -#define NS_APP_CONTENT_PROCESS_TEMP_DIR         "ContentTmpD"
> +
> +// This is the property that should be used for resolving the content process
> +// TEMP path 99% of the time when sandboxing is on. It will fail if
> +// sandboxing is disabled.
> +#define NS_APP_SANDBOXED_CONTENT_PROCESS_TEMP_DIR "SandboxedContentTmpD"

Also note: I'm planning on extending the machinery of crash annotations to include plugin processes, not just content, so either we should rename this key now to "child" instead of "content" or I'll rename it again shortly.

::: xpcom/io/nsAppDirectoryServiceDefs.h:96
(Diff revision 1)
> +// sandboxing is disabled.
> +#define NS_APP_SANDBOXED_CONTENT_PROCESS_TEMP_DIR "SandboxedContentTmpD"
> +
>  #endif // (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
>  
> +// This property is similar to NS_APP_SANDBOXED_CONTENT_PROCESS_TEMP_DIR but

This feels a little insane. What's the point of having two keys here? Wouldn't everyone doing this have to try one and fall back to the other? Why not just make that work out of the box, by e.g. always creating a special temp directory for this situation?

Also, it seems like you're changing the meaning of the original NS_APP_CONTENT_PROCESS_TEMP_DIR without changing the name. If we still have to have two separate keys, we should lose the original name which is ambiguous, and make the new one unambiguously scary.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)

> This feels a little insane. What's the point of having two keys here?
> Wouldn't everyone doing this have to try one and fall back to the other? Why
> not just make that work out of the box, by e.g. always creating a special
> temp directory for this situation?

We need the two separate keys for use in the main process:

NS_APP_SANDBOXED_CONTENT_PROCESS_TEMP_DIR - is used to create a temp with write access from the sandbox, when required, and also to delete it on shutdown (so it's important that this only ever returns the one we've specially created).

NS_APP_CONTENT_PROCESS_TEMP_DIR - is used to know what temp directory the content process is actually using, for retrieving log files etc. in the main process. This could be the sandboxed temp or normal temp if not sandboxed.

We could always create our own temp and delete it even when not sandboxed, but I think the concern here was that if we are using that directory for things like getting crash reporting information, then we want a fallback that we know will exist.

Comment 8

[Benjamin Smedberg]

I still don't see why that requires separate keys for the API surface. The code that *implements* the key can keep track of whether it created the directory (in the sandboxed case) and delete it at shutdown, without exposing that implementation detail to the rest of the browser.

Also, docs don't say which processes each of these keys will be valid in, or really how to use them correctly.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> I still don't see why that requires separate keys for the API surface. The
> code that *implements* the key can keep track of whether it created the
> directory (in the sandboxed case) and delete it at shutdown, without
> exposing that implementation detail to the rest of the browser.

If the directory service code can do this sort of shutdown clean-up then yes that would be much better.

Comment 10

[Benjamin Smedberg]

The directory service itself is just a dumb API. It's the directory service providers that do any special work. In this case probably nsXREDirServiceProvider would maintain that kind of state, since that code knows about the shutdown sequence.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> The directory service itself is just a dumb API. It's the directory service
> providers that do any special work. In this case probably
> nsXREDirServiceProvider would maintain that kind of state, since that code
> knows about the shutdown sequence.

I'm pretty swamped with a11y at the moment, so Haik is going to take over and carry out this implementation.

Comment 12

[Haik Aftandilian [:haik]]

Attachment: Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

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

Comment 13

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59053d5af416

Hitting some failures on try OS X XPCShell due to crashtest toolkit/crashreporter/test/unit_ipc/test_content_exception_time_annotation.js, but those are reproducible locally on a clean mozilla-central.

Comment 14

[Bob Owen (:bobowen)]

https://reviewboard.mozilla.org/r/60584/#review58076

Comments aside, the interplay between the Load and Create functions feels a bit confusing to me.
Maybe splitting that out a bit more would help.

Something like this (apologies for the not-even-pseudo code brain dump):

Add nsCOMPtr<nsIFile> mContentProcessSandboxTempDir instead of mDeleteContentTempOnShutdown bool.

Add static (File scope) functions to cpp instead of member ones:
- IsContentSandboxDisabled - similar to IsSandboxDisabled - should probably check for e10s (BrowserTabsRemoteAutostart()) as well.
- GetContentProcessSandboxTempDir - similar to LoadContentProcessSandboxTempDir, but returns already_AddRefed<nsIFile> instead of setting member and does IsContentSandboxDisabled() return null at start.
- DeleteDirIfExists - similar to DeleteContentProcessSandboxTempDir, but takes an nsIFile argument and just returns NS_OK if null.
- CreateContentProcessSandboxTempDir - similar to now but returns already_AddRefed<nsIFile> and does the following:
-- IsContentSandboxDisabled() return null
-- Create pref if needed
-- d = GetContentProcessSandboxTempDir()
-- DeleteDirIfExists(d)
-- d->Create(...)
-- return d.forget()

LoadContentProcessTempDir then becomes
- mContentTempDir = GetContentProcessSandboxTempDir()
- if (mContentTempDir) return NS_OK
- return get NS_OS_TEMP_DIR

In DoStartup
- MOZ_ASSERT or if parent
- mContentProcessSandboxTempDir = CreateContentProcessSandboxTempDir()

In DoShutDown
- MOZ_ASSERT or if parent
- DeleteDirIfExists(mContentProcessSandboxTempDir)

::: toolkit/xre/nsXREDirProvider.cpp:136
(Diff revision 1)
>  #ifdef MOZ_B2G
>    LoadAppBundleDirs();
>  #endif
>  
> +#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> +  mDeleteContentTempOnShutdown = false;

Perhaps we should just have | = false;| on the member definition.

::: toolkit/xre/nsXREDirProvider.cpp:736
(Diff revision 1)
>  #endif
>  }
>  
> +//
> +// Create a temporary directory for use from sandboxed content processes.
> +// Only called in the parent.

Maybe add a MOZ_ASSERT for this.

::: toolkit/xre/nsXREDirProvider.cpp:738
(Diff revision 1)
> +// Stores the directory in mContentTempDir. This directory will be deleted
> +// on browser shutdown. If the content sandbox is disabled due to

I think losing the "This directory will be deleted on browser shutdown. ", makes this clearer as you explain the deletion policy in detail afterwards anyway.

::: toolkit/xre/nsXREDirProvider.cpp:1182
(Diff revision 1)
>      mozilla::Telemetry::Accumulate(mozilla::Telemetry::SAFE_MODE_USAGE, mode);
>  
>      obsSvc->NotifyObservers(nullptr, "profile-initial-state", nullptr);
> +
> +#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> +    NS_ASSERTION(XRE_IsParentProcess(), "Child creating temp dir!");

I think this should just be an if around the call.
If this should never happen then a MOZ_ASSERT instead.

Same goes for deletion.

::: toolkit/xre/nsXREDirProvider.cpp:1197
(Diff revision 1)
>    PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
>  
>    if (mProfileNotified) {
> +#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> +    NS_ASSERTION(XRE_IsParentProcess(), "Child deleting temp dir!");
> +    NS_WARN_IF(NS_FAILED(DeleteContentProcessSandboxTempDir()));

Should this be testing mDeleteContentTempOnShutdown?

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8766095 [details]
Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

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

Comment 16

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/60584/#review58076

Thanks for the comments. I've reworked the code to follow your suggestions. In the parent, in DoStartup, mContentProcessTempDir and mContentProcessSandboxTempDir are both set so that lookups of NS_APP_CONTENT_PROCESS_TEMP_DIR after startup won't have to rebuild the nsIFile.

> Perhaps we should just have | = false;| on the member definition.

No longer applies.

> I think losing the "This directory will be deleted on browser shutdown. ", makes this clearer as you explain the deletion policy in detail afterwards anyway.

Updated the comment.

> I think this should just be an if around the call.
> If this should never happen then a MOZ_ASSERT instead.
> 
> Same goes for deletion.

Added MOZ_ASSERT's in DoStartup and DoShutdown.

> Should this be testing mDeleteContentTempOnShutdown?

Yes, it should have been, but I removed the bool in this version.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Haik Aftandilian [:haik] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=59053d5af416
> 
> Hitting some failures on try OS X XPCShell due to crashtest
> toolkit/crashreporter/test/unit_ipc/test_content_exception_time_annotation.
> js, but those are reproducible locally on a clean mozilla-central.

Could you please file a bug for this and mark it for sandboxing triage? This is something that we should definitely look into.

Comment 18

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/60584/#review58076

> Added MOZ_ASSERT's in DoStartup and DoShutdown.

Correction: Just added the MOZ_ASSERT to the CreateContentProcessSandboxTempDir(). I saw DoShutdown() being called on the child on one occasion and need to resolve what's going on with that.

Comment 19

[Bob Owen (:bobowen)]

Comment on attachment 8766095 [details]
Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

https://reviewboard.mozilla.org/r/60584/#review58362

Thanks for that, looks good.

And thanks for moving all this into the directory service, it's a much better solution than my original one.

::: toolkit/xre/nsXREDirProvider.cpp:746
(Diff revision 2)
> +// NS_OS_TEMP_DIR is used.
> +//
>  nsresult
>  nsXREDirProvider::LoadContentProcessTempDir()
>  {
> +  if ((mContentTempDir = GetContentProcessSandboxTempDir()) != nullptr) {

nit: Personally I'd prefer (but it's possibly just me):
```c++
mContentTempDir = GetContentProcessSandboxTempDir();
if (mContentTempDir) {
  return NS_OK;
}
```

::: toolkit/xre/nsXREDirProvider.cpp:779
(Diff revision 2)
> +//
> +static already_AddRefed<nsIFile>
> +GetContentProcessSandboxTempDir()
> +{
> +  if (IsContentSandboxDisabled())
> +    return nullptr;

nit: curly brackets even for one line ifs.

::: toolkit/xre/nsXREDirProvider.cpp:795
(Diff revision 2)
>    rv = Preferences::GetString("security.sandbox.content.tempDirSuffix",
>                                &tempDirSuffix);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> -    return rv;
> +    return nullptr;
>    }
>    if (tempDirSuffix.IsEmpty()) {

nit: these ifs can be combined now they return the same.

::: toolkit/xre/nsXREDirProvider.cpp:884
(Diff revision 2)
> +
> +  return sandboxTempDir.forget();
> +}
> +
> +static nsresult
> +DeleteDirIfExists(nsIFile *dir)

nit: looks like this file is a bit inconsistent, so * to the left as per the standards I think.

::: toolkit/xre/nsXREDirProvider.cpp:899
(Diff revision 2)
> +    }
> +  }
>    return NS_OK;
>  }
>  
>  #endif // defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)

nit: not part of this patch, but I think this should be:
```
#endif // (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
```

::: toolkit/xre/nsXREDirProvider.cpp:1201
(Diff revision 2)
>  {
>    PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
>  
>    if (mProfileNotified) {
> +#if (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> +    (void) DeleteDirIfExists(mContentProcessSandboxTempDir);

nit: |`Unused <<`| not |`(void)`|

Comment 20

[Haik Aftandilian [:haik]]

(In reply to Aaron Klotz [:aklotz] (Not reviewing until July, ping on IRC if urgent) from comment #17)
> (In reply to Haik Aftandilian [:haik] from comment #13)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=59053d5af416
> > 
> > Hitting some failures on try OS X XPCShell due to crashtest
> > toolkit/crashreporter/test/unit_ipc/test_content_exception_time_annotation.
> > js, but those are reproducible locally on a clean mozilla-central.
> 
> Could you please file a bug for this and mark it for sandboxing triage? This
> is something that we should definitely look into.

Sure. Filed bug 1283496 "toolkit/crashreporter/test/unit_ipc/test_content_exception_time_annotation.js fails on OS X".

Comment 21

[Haik Aftandilian [:haik]]

Comment on attachment 8766095 [details]
Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

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

Comment 22

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/60584/#review58362

Updated code to address bobowen's comments. I removed the changes to nsExceptionHandler.cpp because those were causing the xpcshell test_content_exception_time_annotation.js test failurse. This is because the attempt to get the NS_APP_CONTENT_PROCESS_TEMP_DIR from xpcshell test context fails without ever making it to nsXREDirProvider so the fallback in nsExceptionHandler.cpp is needed. aklotz explained this in 1257098 (https://bugzilla.mozilla.org/show_bug.cgi?id=1257098#c0) and I'll check with him.

> nit: Personally I'd prefer (but it's possibly just me):
> ```c++
> mContentTempDir = GetContentProcessSandboxTempDir();
> if (mContentTempDir) {
>   return NS_OK;
> }
> ```

Fixed.

> nit: curly brackets even for one line ifs.

Fixed.

> nit: these ifs can be combined now they return the same.

Fixed.

> nit: looks like this file is a bit inconsistent, so * to the left as per the standards I think.

Fixed.

> nit: not part of this patch, but I think this should be:
> ```
> #endif // (defined(XP_WIN) || defined(XP_MACOSX)) && defined(MOZ_CONTENT_SANDBOX)
> ```

Fixed.

> nit: |`Unused <<`| not |`(void)`|

Fixed.

Comment 23

[Haik Aftandilian [:haik]]

Comment on attachment 8766095 [details]
Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

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

Comment 24

[Haik Aftandilian [:haik]]

bsmedberg had the following comments offline.

> 12:16 PM <bsmedberg> ... I'd really like you to go back and add some copious documentation here, though: http://searchfox.org/mozilla-central/source/xpcom/io/nsAppDirectoryServiceDefs.h#88
> 12:16 PM <bsmedberg> to answer these questions in one official place:
> 12:16 PM <bsmedberg> 1) what does this dirservice key represent?
> 12:17 PM <bsmedberg> 2) in what process is it valid to ask for this dirservice key?
> 12:18 PM <bsmedberg> 3) Under what configurations (sandboxing, OSes, etc) is this key valid? What should consumers do in other configs?
> 12:18 PM <bsmedberg> maybe, if this is known already: 4) Is the same directory used for all content processes, if there is more than one?

I've udpated the code on reviewboard, adding comments to nsAppDirectoryServiceDefs.h that answer those questions.

Comment 25

[Benjamin Smedberg]

Comment on attachment 8766095 [details]
Bug 1270018 - NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp

https://reviewboard.mozilla.org/r/60584/#review59426

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5136dcccfe4a
NS_APP_CONTENT_PROCESS_TEMP_DIR should only return the sandbox writeable temp. r=bobowen, r=bsmedberg

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5136dcccfe4a