[macOS] Crash under AuxiliaryProcess::initializeSandbox

• Component: WebKit Misc.
• Status: REOPENED ---

[Per Arne Vollan]

When a WebKit client provides a user directory suffix in the process initialization parameters, confstr with the new user suffix applied will fail to create the full directory path if it does not exist, and return an empty result. This will lead to empty paths in the sandbox parameters, which will cause the sandbox to fail to compile, which will eventually crash the WebKit process.

[Per Arne Vollan]

<rdar://problem/74261611>

[Per Arne Vollan]

Created attachment 421102 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 421102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421102&action=review

> Source/WebKit/ChangeLog:13
> +        eventually crash the WebKit process. This patch addresses this by making sure the confstr temp and cache directories
> +        exist before calling confstr. Additionally, this patch reverts r271417, which was the first attempt at fixing this

What does the path end up being? I'm very skeptical about respecting attacker provided paths in the suffix, for security and overall sanity reasons.

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 421102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421102&action=review
> 
> > Source/WebKit/ChangeLog:13
> > +        eventually crash the WebKit process. This patch addresses this by making sure the confstr temp and cache directories
> > +        exist before calling confstr. Additionally, this patch reverts r271417, which was the first attempt at fixing this
> 
> What does the path end up being? I'm very skeptical about respecting
> attacker provided paths in the suffix, for security and overall sanity
> reasons.

For example, the cache patch can be:

/var/folders/dd/wjn_0cz5593389p25ybpbml00000gn/C/com.apple.mediaremoted/com.apple.WebKit.Networking

This is when the client specifies the user dir suffix 'com.apple.mediaremoted'.

All paths will be under the confstr base path (/var/folders/dd/wjn_0cz5593389p25ybpbml00000gn/C for the cache path). The WebKit client is only allowed to specify the user suffix, which will be appended to the base confstr path, so it is not possible for any client to use this to gain read and write access to arbitrary paths in the WebContent process.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 421102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421102&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:671
> +    setenv("DIRHELPER_USER_DIR_SUFFIX", FileSystem::fileSystemRepresentation(sandboxParameters.userDirectorySuffix()).data(), 1);

I assume this is the bit of the old change being reverted?

[Per Arne Vollan]

Created attachment 421112 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 421102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421102&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:671
> > +    setenv("DIRHELPER_USER_DIR_SUFFIX", FileSystem::fileSystemRepresentation(sandboxParameters.userDirectorySuffix()).data(), 1);
> 
> I assume this is the bit of the old change being reverted?

Yes.

Thanks for reviewing!

[Alexey Proskuryakov]

Comment on attachment 421112 [details]
Patch

This is exactly what I repeatedly said was unacceptable, no?

[Per Arne Vollan]

Created attachment 421118 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 421118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421118&action=review

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:-129
> -    if (!isClientSandboxed()) {
> -        String userDirectorySuffix = xpc_dictionary_get_string(extraDataInitializationDataObject, "user-directory-suffix");
> -        if (!userDirectorySuffix.isEmpty())
> -            extraInitializationData.add("user-directory-suffix"_s, userDirectorySuffix);
> -    }
> -

Hmm, this was added for a reason (see svn blame).

[Per Arne Vollan]

Created attachment 421120 [details]
Patch

[Per Arne Vollan]

Created attachment 421121 [details]
Patch

[Sam Weinig]

Comment on attachment 421121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421121&action=review

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:66
> +#if PLATFORM(COCOA)
> +    String bundleID = WebCore::applicationBundleIdentifier();
> +    return bundleID == "com.apple.WebKit.TestWebKitAPI"_s || bundleID == "com.apple.WebKit.WebKitTestRunner"_s;
> +#else
> +    return false;
> +#endif

Rather than making this based on bundle ID, I think we should add SPI to enable this behavior. Hard coding bundle IDs when we don't need to tends to bite us in the long run when we add something else that needs it.

[Per Arne Vollan]

Created attachment 421183 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #13)
> Comment on attachment 421121 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421121&action=review
> 
> > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:66
> > +#if PLATFORM(COCOA)
> > +    String bundleID = WebCore::applicationBundleIdentifier();
> > +    return bundleID == "com.apple.WebKit.TestWebKitAPI"_s || bundleID == "com.apple.WebKit.WebKitTestRunner"_s;
> > +#else
> > +    return false;
> > +#endif
> 
> Rather than making this based on bundle ID, I think we should add SPI to
> enable this behavior. Hard coding bundle IDs when we don't need to tends to
> bite us in the long run when we add something else that needs it.

That is a good point. I wonder though, if we should try to avoid having the UI process have any control over the decision to allow overriding the user directory suffix. I have made a new patch, where the decision is made in the WebKit process, based on whether WebKit is running in a testing context. This should prevent the client from having control over the decision. What do you think?

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 421184 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 421184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421184&action=review

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:140
> +    // directories of the WebKit process. The WebKit test clients are not sandboxed, and are not running tests against system WebKit,
> +    // which can be used to determine if this should be allowed.

Is it right to intentionally prevent the ability to run tests against system WebKit? Honestly, I don't even fully understand what "platform binary" is, and whether tested WebKit ever becomes one under any internal scenarios (let's not discuss the answer here, though).

But stepping back, I still struggle with understanding what the issue is. Did we confirm that it was actually mediaremoted using WebKit (why)? That one is indeed not sandboxed, but IIRC that's not what we are seeing in crash logs as responsible process.

Seeing that we only use the directory suffix for unsandboxed processes, doing things based on it is not a security problem I thought it was. Maybe creating the directory structure is not so bad. But the root cause of the problem is not that the client isn't sufficiently trusted, but that it's passing a path as a suffix. Perhaps we should fix exactly that?

Something like:
- Ignore user-directory-suffix it it contains "/".
- Put it into system log that we are ignoring it.

What do you think?

[Brent Fulgham]

Comment on attachment 421121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421121&action=review

> Source/WebKit/ChangeLog:17
> +        which was the first attempt at fixing this crash, but was unsuccessful in doing so.

If creating these custom directories is only ever used by our test infrastructure, I wonder if we should require our test harness to create the directories. WebContent process could set the user directory suffix and attempt to use it, but would be unable to make file system changes.

What if we simply crashed with a meaningful error message if we attempt to set user directory suffix?

[Brent Fulgham]

Comment on attachment 421184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421184&action=review

>> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:140
>> +    // which can be used to determine if this should be allowed.
> 
> Is it right to intentionally prevent the ability to run tests against system WebKit? Honestly, I don't even fully understand what "platform binary" is, and whether tested WebKit ever becomes one under any internal scenarios (let's not discuss the answer here, though).
> 
> But stepping back, I still struggle with understanding what the issue is. Did we confirm that it was actually mediaremoted using WebKit (why)? That one is indeed not sandboxed, but IIRC that's not what we are seeing in crash logs as responsible process.
> 
> Seeing that we only use the directory suffix for unsandboxed processes, doing things based on it is not a security problem I thought it was. Maybe creating the directory structure is not so bad. But the root cause of the problem is not that the client isn't sufficiently trusted, but that it's passing a path as a suffix. Perhaps we should fix exactly that?
> 
> Something like:
> - Ignore user-directory-suffix it it contains "/".
> - Put it into system log that we are ignoring it.
> 
> What do you think?

Can you explain what you mean by ignoring u-d-s if it contains '/'? Do you mean we only want a suffix that cannot traverse directories (e.g., "dogfood" is okay, but "dogfood/test" is not)?

If so, I think that sounds like a good plan.

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #17)
> Comment on attachment 421184 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421184&action=review
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:140
> > +    // directories of the WebKit process. The WebKit test clients are not sandboxed, and are not running tests against system WebKit,
> > +    // which can be used to determine if this should be allowed.
> 
> Is it right to intentionally prevent the ability to run tests against system
> WebKit? Honestly, I don't even fully understand what "platform binary" is,
> and whether tested WebKit ever becomes one under any internal scenarios
> (let's not discuss the answer here, though).
> 

This patch will not prevent you from running tests with system WebKit. It will just not share the confstr temp and cache paths between the UI process and the WebKit processes, which should not cause any issues.

I should perhaps have chosen a different name. What I meant to say was whether the WebKit process was system WebKit or not. During testing this will not be the case, since we are testing a locally built WebKit which is not located in the system directory.

> But stepping back, I still struggle with understanding what the issue is.
> Did we confirm that it was actually mediaremoted using WebKit (why)? That
> one is indeed not sandboxed, but IIRC that's not what we are seeing in crash
> logs as responsible process.
> 

Yes, I am quite confident that this is the case, but I am not sure why it does not show up in the crash log as responsible process.

> Seeing that we only use the directory suffix for unsandboxed processes,
> doing things based on it is not a security problem I thought it was. Maybe
> creating the directory structure is not so bad. But the root cause of the
> problem is not that the client isn't sufficiently trusted, but that it's
> passing a path as a suffix. Perhaps we should fix exactly that?
> 
> Something like:
> - Ignore user-directory-suffix it it contains "/".
> - Put it into system log that we are ignoring it.
> 
> What do you think?

The client is actually never passing a suffix containing a path. It is actually us that create the path :) For example, if the client passes the suffix "foo", we will add the bundle identifier to this, ending up with a suffix of "foo/com.apple.WebKit.Networking", which will make confstr fail.

We can revive the patch which creates the directories, if you think that's a better option?

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #18)
> Comment on attachment 421121 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421121&action=review
> 
> > Source/WebKit/ChangeLog:17
> > +        which was the first attempt at fixing this crash, but was unsuccessful in doing so.
> 
> If creating these custom directories is only ever used by our test
> infrastructure, I wonder if we should require our test harness to create the
> directories. WebContent process could set the user directory suffix and
> attempt to use it, but would be unable to make file system changes.
> 
> What if we simply crashed with a meaningful error message if we attempt to
> set user directory suffix?

I think there are other clients that end up using this feature, which then causes a crash when the sandbox does not compile due to the user directory suffix containing a path.

I think one option is to revive the original patch that creates these directories if they do not exist.

Thanks for reviewing!

[Alexey Proskuryakov]

> Can you explain what you mean by ignoring u-d-s if it contains '/'? Do you
> mean we only want a suffix that cannot traverse directories (e.g., "dogfood"
> is okay, but "dogfood/test" is not)?
> 
> If so, I think that sounds like a good plan.

Yes, that was my idea exactly.

[Per Arne Vollan]

Created attachment 421219 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #22)
> > Can you explain what you mean by ignoring u-d-s if it contains '/'? Do you
> > mean we only want a suffix that cannot traverse directories (e.g., "dogfood"
> > is okay, but "dogfood/test" is not)?
> > 
> > If so, I think that sounds like a good plan.
> 
> Yes, that was my idea exactly.

Sounds good, I have updated the patch.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 421219 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 421219 [details]
Patch

Thanks for reviewing!

[Alexey Proskuryakov]

Comment on attachment 421219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421219&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:644
> +            auto firstPathSeparator = suffix.find("/");

This could use a comment explaining why.

And I still think that there should be system logging about non-trivial behavior choices like this.

[EWS]

Committed r273271: <https://commits.webkit.org/r273271>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421219 [details].

[Brent Fulgham]

(In reply to Alexey Proskuryakov from comment #27)
> Comment on attachment 421219 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421219&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:644
> > +            auto firstPathSeparator = suffix.find("/");
> 
> This could use a comment explaining why.
> 
> And I still think that there should be system logging about non-trivial
> behavior choices like this.

Per Arne: Could you do a follow-up patch (under a different radar) to do these two things?

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #27)
> Comment on attachment 421219 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421219&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:644
> > +            auto firstPathSeparator = suffix.find("/");
> 
> This could use a comment explaining why.
> 
> And I still think that there should be system logging about non-trivial
> behavior choices like this.

Yes, that makes sense, I will follow up on this.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #29)
> (In reply to Alexey Proskuryakov from comment #27)
> > Comment on attachment 421219 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=421219&action=review
> > 
> > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:644
> > > +            auto firstPathSeparator = suffix.find("/");
> > 
> > This could use a comment explaining why.
> > 
> > And I still think that there should be system logging about non-trivial
> > behavior choices like this.
> 
> Per Arne: Could you do a follow-up patch (under a different radar) to do
> these two things?

Will do, thanks for reviewing!

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 421252 [details]
Patch

[Sam Weinig]

(In reply to Per Arne Vollan from comment #21)
> (In reply to Brent Fulgham from comment #18)
> > Comment on attachment 421121 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=421121&action=review
> > 
> > > Source/WebKit/ChangeLog:17
> > > +        which was the first attempt at fixing this crash, but was unsuccessful in doing so.
> > 
> > If creating these custom directories is only ever used by our test
> > infrastructure, I wonder if we should require our test harness to create the
> > directories. WebContent process could set the user directory suffix and
> > attempt to use it, but would be unable to make file system changes.
> > 
> > What if we simply crashed with a meaningful error message if we attempt to
> > set user directory suffix?
> 
> I think there are other clients that end up using this feature, which then
> causes a crash when the sandbox does not compile due to the user directory
> suffix containing a path.
> 

We expect users of SPI to know what they are doing, especially if we put the word "Testing" in the name. Using bundle ID checks should really only ever be a temporary measure when dealing with Apple internal things.

[Alexey Proskuryakov]

The new patch is marked cq+, but the bug is in RESOLVED state, so commit queue won't process it.

I think that an ideal log would also say something when we decide to ignore because of a slash, and even says something if we decide to ignore because the client is sandboxed.


Sam, what safeguards would remain if we had an SPI? I mostly like this idea, but we also need to make sure that an SPI cannot be used maliciously.

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #35)
> The new patch is marked cq+, but the bug is in RESOLVED state, so commit
> queue won't process it.
> 
> I think that an ideal log would also say something when we decide to ignore
> because of a slash, and even says something if we decide to ignore because
> the client is sandboxed.
> 
> 
> Sam, what safeguards would remain if we had an SPI? I mostly like this idea,
> but we also need to make sure that an SPI cannot be used maliciously.

What attack are you considering?

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 421304 [details]
Patch

[EWS]

Committed r273304: <https://commits.webkit.org/r273304>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421304 [details].

[Alexey Proskuryakov]

> What attack are you considering?

If malicious code running in UI process has control over cache and temporary directories of WebKit XPC processes, it can affect what XPC processes do before they enter the sandbox (e.g. write something there to exploit bugs in deserialization code to gain arbitrary code execution). In general, we want to take as little input from UI process before entering sandbox as possible.

> Reopening to attach new patch.

I just noticed that the patch truncates the suffix instead of completely ignoring it when there is a slash. What is the reason for this behavior? It seems harder to reason about.

Not a terribly huge deal because most WebKit clients should be sandboxed anyway, and then we completely ignore the suffix.

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #40)
> > What attack are you considering?
> 
> If malicious code running in UI process has control over cache and temporary
> directories of WebKit XPC processes, it can affect what XPC processes do
> before they enter the sandbox (e.g. write something there to exploit bugs in
> deserialization code to gain arbitrary code execution). In general, we want
> to take as little input from UI process before entering sandbox as possible.

Gotcha. That old one (It is so frustrating that window exists at all, I need to follow up on if we can just stop doing that). 

The traditional way of handling this, and what I think we could do here, is to require an entitlement on the host to do this, and to give that entitlement to our testing apps. I would much prefer entitlement based policy over bundle ID policy as it is scalable.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222324