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.
<rdar://problem/66551986>
Created attachment 415213 [details] Patch
Created attachment 415214 [details] Patch
Created attachment 415222 [details] Patch
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.
Created attachment 415283 [details] Patch
(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!
Created attachment 415288 [details] Patch
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
Comment on attachment 415288 [details] Patch Whoops! Not quite right.
Created attachment 427162 [details] Patch
Comment on attachment 427162 [details] Patch r=me
Comment on attachment 427162 [details] Patch Thanks for reviewing!
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].