218479 – [WebAuthn] [iOS] WebAuthn process doesn't start on iOS devices

Bug 218479 - [WebAuthn] [iOS] WebAuthn process doesn't start on iOS devices

Summary: [WebAuthn] [iOS] WebAuthn process doesn't start on iOS devices

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebKit Misc. (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Jiewen Tan

URL:
Keywords:	InRadar

Depends on:
Blocks:	181943
	Show dependency tree / graph

Reported:	2020-11-02 22:23 PST by Jiewen Tan
Modified:	2020-11-06 17:39 PST (History)
CC List:	3 users (show)

See Also:

Attachments
Patch (43.44 KB, patch) 2020-11-02 22:30 PST, Jiewen Tan	no flags	Details \| Formatted Diff \| Diff
Patch (37.33 KB, patch) 2020-11-05 00:00 PST, Jiewen Tan	no flags	Details \| Formatted Diff \| Diff
Patch (37.97 KB, patch) 2020-11-05 19:39 PST, Jiewen Tan	bfulgham: review+	Details \| Formatted Diff \| Diff
Patch for landing (33.10 KB, patch) 2020-11-06 17:10 PST, Jiewen Tan	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Jiewen Tan 2020-11-02 22:23:55 PST

WebAuthn process doesn't start on iOS devices.

Comment 1 Jiewen Tan 2020-11-02 22:24:16 PST

<rdar://problem/70560399>

Comment 2 Jiewen Tan 2020-11-02 22:30:20 PST

Created attachment 413004 [details]
Patch

Comment 3 Brent Fulgham 2020-11-04 14:01:35 PST

Comment on attachment 413004 [details]
Patch

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

This initial sandbox is far too lax. You should pare it down before we land this.

I also expect to see macOS sandboxing for this process, is that happening in a separate bug?

> Source/WebKit/ChangeLog:12
> +        Copied from the GPU process. Will strip it down once the process is fully functional.

You will need a sandbox on macOS, too.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:199
> +)

I'll bet we do not need media-remote at all.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:219
> +)

I doubt we need media-capture at all.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:230
> +)

We might need this if we are showing UI through this XPC service, but I suspect that authd actually does that, so we probably don't need this.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:240
> +)

I doubt we need media-accessibility-support.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:290
> +)

I don't we need the OpenGL support.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:383
> +)

I doubt we need speech-synthesis-and-voiceover.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:434
> +)

We probably do not need dictionary-support.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:579
> +    (global-name "com.apple.tccd"))

I don't think we need this, but since it's generating telemetry you can keep it so we can double-check.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:665
> +(play-media)

I doubt we need these.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:668
> +(media-remote)

Delete this please.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:674
> +    (global-name "com.apple.TextInput"))

I doubt we need this.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:678
> +(speech-synthesis-and-voiceover)

Delete this please.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:681
> +    (global-name "com.apple.audio.AudioComponentRegistrar"))

Delete this please.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:688
> +    "/systemgroup.com.apple.nsurlstoragedresources/Library/dafsaData.bin")

Seems very unlikely we need this.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:707
> +)

It seems very unlikely we need these.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:724
> +    (global-name "com.apple.iconservices"))

It seems unlikely we need this stuff.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:738
> +(dictionary-support)

Delete this, please.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:748
> +(framebuffer-access)

Seems unlikely

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:751
> +(opengl)

Seems very unlikely

Comment 4 Brent Fulgham 2020-11-04 16:51:46 PST

Comment on attachment 413004 [details]
Patch

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:544
> +           (extension "com.apple.security.exception.shared-preference.read-write"))

Please also remove this rule (see Bug 218594). We don't issue this extension to WebKit, ever.

Comment 5 Brent Fulgham 2020-11-04 17:05:36 PST

Comment on attachment 413004 [details]
Patch

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:538
> +           (extension "com.apple.security.exception.iokit-user-client-class"))

Please also remove this rule (See Bug 218596). We don't issue this extension to WebKit, ever.

Comment 6 Jiewen Tan 2020-11-04 23:58:30 PST

Comment on attachment 413004 [details]
Patch

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

Thanks Brent for reviewing this patch.

>> Source/WebKit/ChangeLog:12
>> +        Copied from the GPU process. Will strip it down once the process is fully functional.
> 
> You will need a sandbox on macOS, too.

The macOS process already has one.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:199
>> +)
> 
> I'll bet we do not need media-remote at all.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:219
>> +)
> 
> I doubt we need media-capture at all.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:230
>> +)
> 
> We might need this if we are showing UI through this XPC service, but I suspect that authd actually does that, so we probably don't need this.

Springboard will probably do that for us. Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:240
>> +)
> 
> I doubt we need media-accessibility-support.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:290
>> +)
> 
> I don't we need the OpenGL support.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:383
>> +)
> 
> I doubt we need speech-synthesis-and-voiceover.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:434
>> +)
> 
> We probably do not need dictionary-support.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:538
>> +           (extension "com.apple.security.exception.iokit-user-client-class"))
> 
> Please also remove this rule (See Bug 218596). We don't issue this extension to WebKit, ever.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:544
>> +           (extension "com.apple.security.exception.shared-preference.read-write"))
> 
> Please also remove this rule (see Bug 218594). We don't issue this extension to WebKit, ever.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:579
>> +    (global-name "com.apple.tccd"))
> 
> I don't think we need this, but since it's generating telemetry you can keep it so we can double-check.

Got you.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:665
>> +(play-media)
> 
> I doubt we need these.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:668
>> +(media-remote)
> 
> Delete this please.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:674
>> +    (global-name "com.apple.TextInput"))
> 
> I doubt we need this.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:678
>> +(speech-synthesis-and-voiceover)
> 
> Delete this please.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:681
>> +    (global-name "com.apple.audio.AudioComponentRegistrar"))
> 
> Delete this please.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:688
>> +    "/systemgroup.com.apple.nsurlstoragedresources/Library/dafsaData.bin")
> 
> Seems very unlikely we need this.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:707
>> +)
> 
> It seems very unlikely we need these.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:724
>> +    (global-name "com.apple.iconservices"))
> 
> It seems unlikely we need this stuff.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:738
>> +(dictionary-support)
> 
> Delete this, please.

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:748
>> +(framebuffer-access)
> 
> Seems unlikely

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:751
>> +(opengl)
> 
> Seems very unlikely

Removed.

Comment 7 Jiewen Tan 2020-11-05 00:00:08 PST

Created attachment 413264 [details]
Patch

Comment 8 Jiewen Tan 2020-11-05 19:39:04 PST

Created attachment 413387 [details]
Patch

Comment 9 Brent Fulgham 2020-11-06 16:20:08 PST

Comment on attachment 413387 [details]
Patch

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

Can you please file a radar to audit the sandbox? I think both WebAuthn sandoxes contain tons of stuff we don't need, and we should clean that up before we turn this on for people. But r=me for this work in progress.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:111
> +           (xpc-service-name "com.apple.audio.toolbox.reporting.service")))

I'll bet we don't need "play-audio"

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:189
> +)

I'll bet we don't need this "play-media" bit at all.

Comment 10 Jiewen Tan 2020-11-06 16:27:26 PST

Comment on attachment 413387 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:111
>> +           (xpc-service-name "com.apple.audio.toolbox.reporting.service")))
> 
> I'll bet we don't need "play-audio"

Removed.

>> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb:189
>> +)
> 
> I'll bet we don't need this "play-media" bit at all.

Removed.

Comment 11 Jiewen Tan 2020-11-06 17:10:19 PST

Created attachment 413500 [details]
Patch for landing

Comment 12 EWS 2020-11-06 17:38:58 PST

Committed r269554: <https://trac.webkit.org/changeset/269554>

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