Bug 237717 – [macOS] Image decoders should be restricted for Mail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

We already restrict image decoders for Mail on iOS. We should do so on macOS too.

[Per Arne Vollan]

<rdar://89827733>

[Per Arne Vollan]

Created attachment 454367 [details]
Patch

[Per Arne Vollan]

Created attachment 454396 [details]
Patch

[Per Arne Vollan]

Created attachment 454398 [details]
Patch

[Geoffrey Garen]

Comment on attachment 454398 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:78
> +    static bool m_enableRestrictedDecoding;

For static variables, we use the "s_" prefix. "m_" for "member", "s_" for "static".

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:450
> +        if (auto trustdExtensionHandle = SandboxExtension::createHandleForMachLookup("com.apple.trustd.agent"_s, std::nullopt))
> +            parameters.trustdExtensionHandle = WTFMove(*trustdExtensionHandle);

In a follow-up, it would be nice to disconnect Mail from trustd.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 454398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454398&action=review
> 
> r=me
> 
> > Source/WebCore/platform/graphics/cg/ImageDecoderCG.h:78
> > +    static bool m_enableRestrictedDecoding;
> 
> For static variables, we use the "s_" prefix. "m_" for "member", "s_" for
> "static".
> 

Will fix!

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:450
> > +        if (auto trustdExtensionHandle = SandboxExtension::createHandleForMachLookup("com.apple.trustd.agent"_s, std::nullopt))
> > +            parameters.trustdExtensionHandle = WTFMove(*trustdExtensionHandle);
> 
> In a follow-up, it would be nice to disconnect Mail from trustd.

That is a good point, I will follow up on this.

Thanks for reviewing!

[EWS]

Committed r291190 (248346@main): <https://commits.webkit.org/248346@main>

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

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 454514 [details]
Patch

[EWS]

Committed r291204 (248359@main): <https://commits.webkit.org/248359@main>

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

[Tim Horton]

Comment on attachment 454398 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:295
> +#if USE(APPLE_INTERNAL_SDK)

I know this was preexisting, but it's generally wrong for code to be guarded by USE(APPLE_INTERNAL_SDK); you should use SPI headers (which can be guarded by USE(APPLE_INTERNAL_SDK)) to run the same code in non-internal-SDK builds... is there a reason this code doesn't behave like the rest?

[Per Arne Vollan]

(In reply to Tim Horton from comment #11)
> Comment on attachment 454398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454398&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:295
> > +#if USE(APPLE_INTERNAL_SDK)
> 
> I know this was preexisting, but it's generally wrong for code to be guarded
> by USE(APPLE_INTERNAL_SDK); you should use SPI headers (which can be guarded
> by USE(APPLE_INTERNAL_SDK)) to run the same code in non-internal-SDK
> builds... is there a reason this code doesn't behave like the rest?

That is a good point. This should behave like the rest. I have filed a bug report to address this.

Thanks for reviewing!