Record sandboxing status in crash reports

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gcp, Assigned: haik)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

(Whiteboard: sbwc1)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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?
(Reporter)

Updated

2 years ago
Depends on: 1098428

Comment 1

2 years ago
hey gcp, is this linux specific or are you planning on coverage the other platforms too?
Flags: needinfo?(gpascutto)
(Reporter)

Comment 2

2 years ago
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?
Flags: needinfo?(gpascutto)
OS: Unspecified → All
Hardware: Unspecified → All

Updated

2 years ago
Flags: needinfo?(haftandilian)
Whiteboard: sbwc1
(Assignee)

Comment 3

2 years ago
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.
Assignee: nobody → haftandilian
Flags: needinfo?(haftandilian)
(Assignee)

Comment 4

2 years ago
Created attachment 8770243 [details]
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/
Attachment #8770243 - Flags: review?(gpascutto)
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

2 years ago
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
(Reporter)

Comment 7

2 years ago
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.
Attachment #8770243 - Flags: review?(gpascutto) → review-
(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.
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

2 years ago
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.
(Assignee)

Comment 11

2 years ago
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/
Attachment #8770243 - Flags: review- → review?(gpascutto)
(Reporter)

Comment 12

2 years ago
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.
Attachment #8770243 - Flags: review?(gpascutto) → review+
(Assignee)

Comment 13

2 years ago
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/
(Assignee)

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
Created attachment 8774480 [details] [diff] [review]
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
Attachment #8770243 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9bbe09fe561
Record sandboxing status in crash reports; r=gcp
Keywords: checkin-needed
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    ^
(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.
Depends on: 1289381

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9bbe09fe561
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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).
You need to log in before you can comment on or make changes to this bug.