Bug 219428 – Add sandbox extension flag to specify that path contains no symlinks

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

In general, when SandboxExtension::createHandleWithoutResolvingPath is called, it is assumed that there are no symlinks in the provided path. Add a 'canonical' flag, which can be used by platform APIs to verify that this is the case.

[Per Arne Vollan]

<rdar://problem/66551986>

[Per Arne Vollan]

Created attachment 415213 [details]
Patch

[Per Arne Vollan]

Created attachment 415214 [details]
Patch

[Per Arne Vollan]

Created attachment 415222 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 415222 [details]
Patch

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

Is the API test failure real?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:97
> +        if (flags & SandboxExtension::Flags::Canonical)
> +            extensionFlags |= SANDBOX_EXTENSION_CANONICAL;

If I understand what this does correctly, then the naming is actively misleading. Putting a "canonical" flag on an extension means that it is canonical. But we cannot guarantee that, and that's the whole purpose of adding the flag!

The difference between "is canonical" and "do not canonicalize" is subtle and even non-existent when everything goes well, but maintaining this difference is how we defend against attacks.

I think that we should have another pass at the naming, and do better than mirror an SPI name.

[Per Arne Vollan]

Created attachment 415283 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 415222 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415222&action=review
> 
> Is the API test failure real?
> 

Yes, should be fixed in latest patch.

> > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:97
> > +        if (flags & SandboxExtension::Flags::Canonical)
> > +            extensionFlags |= SANDBOX_EXTENSION_CANONICAL;
> 
> If I understand what this does correctly, then the naming is actively
> misleading. Putting a "canonical" flag on an extension means that it is
> canonical. But we cannot guarantee that, and that's the whole purpose of
> adding the flag!
> 
> The difference between "is canonical" and "do not canonicalize" is subtle
> and even non-existent when everything goes well, but maintaining this
> difference is how we defend against attacks.
> 
> I think that we should have another pass at the naming, and do better than
> mirror an SPI name.

That is a good point. I have proposed a new name in the latest patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 415288 [details]
Patch

[Brent Fulgham]

Comment on attachment 415288 [details]
Patch

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

I reviewed this with Coops and I think it looks good. r=me.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:277
> +    handle.m_sandboxExtension = SandboxExtensionImpl::create(path.utf8().data(), type, WTF::nullopt,  SandboxExtension::Flags::DoNotCanonicalize);

Extra whitespace before SandboxExtension::Flags::DoNotCanonicalize

[Brent Fulgham]

Comment on attachment 415288 [details]
Patch

Whoops! Not quite right.

[Per Arne Vollan]

Created attachment 427162 [details]
Patch

[Brent Fulgham]

Comment on attachment 427162 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 427162 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r277104 (237406@main): <https://commits.webkit.org/237406@main>

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