1274540 - Record sandboxing status in crash reports

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Can salvage most of the implementation from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1098428

I presume content yes/no is the flag we want (probably don't need to know media yes/no?).

Do we need to record sandboxing levels?

Comment 1

[Jim Mathies [:jimm]]

hey gcp, is this linux specific or are you planning on coverage the other platforms too?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Bug 1098428 was the Linux specific stuff, this can cover everything else. I think the agreement so far was that we only need a yes/no flag for content, and we don't care for levels yet?

Comment 3

[Haik Aftandilian [:haik]]

Looks like we'll want to call AnnotateCrashReport::AnnotateCrashReport() in both the parent and the content process so we'll always know from a crash whether or not Sandboxing was enabled.

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Bug 1274540 - Record sandboxing status in crash reports;

Adds content sandbox metadata to parent and child crash reports:
Includes the value of pref security.sandbox.content.level,
whether or not the system is capable of sandboxing, whether or
not content received message to turn on the sandbox, if the
sandbox was successfully turned on, and (on Linux systems)
the sandbox capabilities flags.

New crash report keys:
"ContentSandboxLevel" in parent and content
"ContentSandboxCapable" in parent
"ReceivedSetProcessSandbox" in content
"ContentSandboxEnabled" in content
"ContentSandboxCapabilities" in content on Linux

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

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://reviewboard.mozilla.org/r/63482/#review60804

The Linux parts look reasonable, but:

::: security/sandbox/linux/common/SandboxInfo.h:60
(Diff revision 1)
>    bool CanSandboxMedia() const
>    {
>      return !Test(kEnabledForMedia) || Test(kHasSeccompBPF);
>    }
>  
> +  void AnnotateCrashReport() const;

FYI, I think this will break the B2G build if it's not MOZ_EXPORT'ed.

Comment 6

[Bob Owen (:bobowen)]

https://reviewboard.mozilla.org/r/63482/#review61216

::: toolkit/xre/nsAppRunner.cpp:3960
(Diff revision 1)
> +  nsCString levelString;
> +  levelString.AppendInt(level);
> +
> +  CrashReporter::AnnotateCrashReport(
> +    NS_LITERAL_CSTRING("ContentSandboxLevel"), levelString);
> + 

nit: whitespace

::: toolkit/xre/nsAppRunner.cpp:3965
(Diff revision 1)
> +  if (IsVistaOrLater()) {
> +    sandboxCapable = true;

Given that this is about capability, this should probably be true on all supported versions for Windows as well.

It's true that the main part of our level 1 sandbox is low integrity, which is only available on Vista and above (we are user the USER_NON_ADMIN token as well, but that probably doesn't help much in practise).
Later levels should provide some better protection for XP.

::: toolkit/xre/nsAppRunner.cpp:3970
(Diff revision 1)
> +  if (IsVistaOrLater()) {
> +    sandboxCapable = true;
> +  }
> +#elif defined(XP_MACOSX)
> +  // All supported OS X versions are capable
> +  sandboxCapable = true; 

nit: whitespace

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8770243 [details]
Bug 1274540 - Record sandboxing status in crash reports;

https://reviewboard.mozilla.org/r/63482/#review61552

Mostly r- because I'd like to see some things cleared up before landing (it's possible that the code is fine).

::: dom/ipc/ContentChild.cpp:1423
(Diff revision 1)
>  bool
>  ContentChild::RecvSetProcessSandbox(const MaybeFileDesc& aBroker)
>  {
> +#ifdef MOZ_CRASHREPORTER
> +  CrashReporter::AnnotateCrashReport(
> +    NS_LITERAL_CSTRING("ReceivedSetProcessSandbox"),

Why do we need this flag? Is it for the case where setting up the actual sandbox crashes? Wouldn't we see this from the stack?

::: dom/ipc/ContentChild.cpp:1460
(Diff revision 1)
>        MOZ_RELEASE_ASSERT(brokerFd >= 0);
>    }
> -  SetContentProcessSandbox(brokerFd);
> +  sandboxEnabled = SetContentProcessSandbox(brokerFd);
>  #elif defined(XP_WIN)
>    mozilla::SandboxTarget::Instance()->StartSandbox();
> +  sandboxEnabled = true;

Diving into the callees, it looks we terminate the process if this fails (which explains why it doesn't return failure).

::: security/sandbox/mac/Sandbox.mm:504
(Diff revision 1)
>                 aInfo.appTempDir.c_str(),
>                 getenv("HOME"));
>      } else {
>        fprintf(stderr,
>          "Content sandbox disabled due to sandbox level setting\n");
> -      return (true);
> +      return false;

Is this safe to do?

Looking at the callers, the first one I see is: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1416

::: toolkit/xre/nsAppRunner.h:28
(Diff revision 1)
>  #define MAXPATHLEN 1024
>  #endif
>  #endif
>  
> +#if defined(MOZ_CONTENT_SANDBOX)
> +#if (defined(XP_LINUX) && !defined(OS_ANDROID)) || \

Is MOZ_CONTENT_SANDBOX set on OS_ANROID? I'd think not? And if not, what's the use of this #define juggling?

Is it for B2G? Then I'd add a !(MOZ_WIDGET_GONK) at the point of use.

::: toolkit/xre/nsEmbedFunctions.cpp:311
(Diff revision 1)
> +void
> +AddContentSandboxLevelAnnotation()
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    int level = Preferences::GetInt("security.sandbox.content.level");
> +    nsCString levelString;

nsAutoCString for short, temporary strings.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> Is MOZ_CONTENT_SANDBOX set on OS_ANROID? I'd think not? And if not, what's
> the use of this #define juggling?
> 
> Is it for B2G? Then I'd add a !(MOZ_WIDGET_GONK) at the point of use.

I've been using ANDROID instead of MOZ_WIDGET_GONK for things that would tend to be true on anything Android-based.  In particular, this is what I've done in the seccomp-bpf policy for determining “desktop”-ness, because things there mostly depend on the library stacks used for hardware access and media and graphics and so on (or sometimes glibc vs. bionic).

But for getting more data in crash reports it might as well be !(MOZ_WIDGET_GONK) if anything; it's simpler and, in a hypothetical future where we have regular-Android e10s and want to apply sandboxing, we'd probably want the crash annotations there as well.

Comment 9

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/63482/#review60804

> FYI, I think this will break the B2G build if it's not MOZ_EXPORT'ed.

Added the MOZ_EXPORT, but haven't verified on B2G yet.

Comment 10

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/63482/#review61552

> Why do we need this flag? Is it for the case where setting up the actual sandbox crashes? Wouldn't we see this from the stack?

I was thinking it could be useful when looking at a Linux content process crash. If sandboxing doesn't appear to be enabled, that flag would tell you if the reason it's not enabled is that the child hasn't received the SetProcessSandbox message from the parent yet.

Instead of having a separate flag for this, I'll refactor RecvSetProcessSandbox() to always set the ContentSandboxEnabled flag. Then, absence of ContentSandboxEnabled will indicate the child hasn't received the SetProcessSandbox message yet. With the current code, ContentSandboxEnabled is not set if SandboxInfo::Get().CanSandboxContent() returns false.

> Diving into the callees, it looks we terminate the process if this fails (which explains why it doesn't return failure).

But not if SetContentProcessSandbox does an early return due to its call to SandboxInfo::Get().Test(SandboxInfo::kEnabledForContent) returning false. I added the return so that that case is detected and the crash report would indicate the sandbox isn't enabled.

> Is this safe to do?
> 
> Looking at the callers, the first one I see is: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1416

I think it's safe because the change here only applies to (type == MacSandboxType_Content) for which the caller is always ContentChild.cpp:StartMacOSContentSandbox() and I added code there to not call this if the sandbox level is < 1. I did some manual testing with level=0 and it seemed to be fine, StartMacSandbox was never called. Marking as fixed but let me know if I missed the point.

> Is MOZ_CONTENT_SANDBOX set on OS_ANROID? I'd think not? And if not, what's the use of this #define juggling?
> 
> Is it for B2G? Then I'd add a !(MOZ_WIDGET_GONK) at the point of use.

The intent there was to only add the annotations for desktop on Mac/Linux/Windows because I assumed B2G and Fennec handle crash reports differently. Will follow your and Jed's recommendation to exclude MOZ_WIDGET_GONK instead. Not sure if I understood this correctly.

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8770243 [details]
Bug 1274540 - Record sandboxing status in crash reports;

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

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8770243 [details]
Bug 1274540 - Record sandboxing status in crash reports;

https://reviewboard.mozilla.org/r/63482/#review62568

::: security/sandbox/mac/Sandbox.mm:504
(Diff revision 2)
>                 aInfo.appTempDir.c_str(),
>                 getenv("HOME"));
>      } else {
>        fprintf(stderr,
>          "Content sandbox disabled due to sandbox level setting\n");
> -      return (true);
> +      return false;

If I'm following you reasoning correctly, this code should never be called, as our only caller will early-exit when the level is insufficient, before calling us.

So this should be dead code. Can we assert that by making this crash in debug mode?

::: toolkit/xre/nsAppRunner.h:30
(Diff revision 2)
>  #endif
>  
> +#if defined(MOZ_CONTENT_SANDBOX)
> +#if (defined(XP_LINUX) && !defined(MOZ_WIDGET_GONK)) || \
> +  defined(XP_WIN) || defined(XP_MACOSX)
> +#define MOZ_CONTENT_SANDBOX_DESKTOP 1

I didn't manage to explain this well enough: if MOZ_CONTENT_SANDBOX is enabled, we'd always want the crash annotation, save for maybe specific cases where Gonk is different. So calling this "SANDBOX_DESKTOP" is odd (see also jld's comment on Android).

I think you want "defined(MOZ_CONTENT_SANDBOX) && !defined(MOZ_WIDGET_GONK)", and I'm not sure if I'd add an extra define for that, rather than spelling out those two explicitly at the points where they're needed.

Said differently, GONK is the exception, so that should be in the define, not everything else.

Comment 13

[Haik Aftandilian [:haik]]

Comment on attachment 8770243 [details]
Bug 1274540 - Record sandboxing status in crash reports;

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

Comment 14

[Haik Aftandilian [:haik]]

https://reviewboard.mozilla.org/r/63482/#review62568

> If I'm following you reasoning correctly, this code should never be called, as our only caller will early-exit when the level is insufficient, before calling us.
> 
> So this should be dead code. Can we assert that by making this crash in debug mode?

Yes, that's right. Added a MOZ_ASSERT.

> I didn't manage to explain this well enough: if MOZ_CONTENT_SANDBOX is enabled, we'd always want the crash annotation, save for maybe specific cases where Gonk is different. So calling this "SANDBOX_DESKTOP" is odd (see also jld's comment on Android).
> 
> I think you want "defined(MOZ_CONTENT_SANDBOX) && !defined(MOZ_WIDGET_GONK)", and I'm not sure if I'd add an extra define for that, rather than spelling out those two explicitly at the points where they're needed.
> 
> Said differently, GONK is the exception, so that should be in the define, not everything else.

OK, thanks. I've updated the code to use defined(MOZ_CONTENT_SANDBOX) && !defined(MOZ_WIDGET_GONK). I've removed the new MOZ_CONTENT_SANDBOX_DESKTOP I had added.

Comment 15

[Haik Aftandilian [:haik]]

Attachment: Adds content sandbox status to crash reports for chrome/content processes

Adding final version of changes as a patch attachment.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9bbe09fe561
Record sandboxing status in crash reports; r=gcp

Comment 17

[[:fabrice] Fabrice Desré]

hm, that doesn't build locally:

0:26.98 /home/fabrice/dev/mozilla-inbound/security/sandbox/linux/common/SandboxInfo.cpp: In member function ‘void mozilla::SandboxInfo::AnnotateCrashReport() const’:
 0:26.98 /home/fabrice/dev/mozilla-inbound/security/sandbox/linux/common/SandboxInfo.cpp:291:3: error: ‘CrashReporter’ has not been declared
 0:26.98    CrashReporter::AnnotateCrashReport(
 0:26.98    ^

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to [:fabrice] Fabrice Desré from comment #17)
> hm, that doesn't build locally:
> 
> 0:26.98
> /home/fabrice/dev/mozilla-inbound/security/sandbox/linux/common/SandboxInfo.
> cpp: In member function ‘void mozilla::SandboxInfo::AnnotateCrashReport()
> const’:
>  0:26.98
> /home/fabrice/dev/mozilla-inbound/security/sandbox/linux/common/SandboxInfo.
> cpp:291:3: error: ‘CrashReporter’ has not been declared
>  0:26.98    CrashReporter::AnnotateCrashReport(
>  0:26.98    ^

This is a build with --disable-crashreporter, which should still be supported.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

History note: We used to catch --disable-crashreporter breakage as a convenient side-effect of the automation ASan builds, because they happen to disable crash reporting… but I just made the ASan builds disable Linux sandboxing because it conflicts with LSan (bug 1287971).