Bug 195342 - [iOS] Block the accessibility server when accessibility is not enabled.
Summary: [iOS] Block the accessibility server when accessibility is not enabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 14:47 PST by Per Arne Vollan
Modified: 2019-03-17 19:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (13.18 KB, patch)
2019-03-05 15:27 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.28 KB, patch)
2019-03-06 11:39 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.22 KB, patch)
2019-03-06 12:06 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.22 KB, patch)
2019-03-06 12:16 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2019-03-06 13:00 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2019-03-06 13:06 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.17 KB, patch)
2019-03-07 09:11 PST, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (15.28 KB, patch)
2019-03-14 14:34 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.48 KB, patch)
2019-03-15 10:58 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.06 KB, patch)
2019-03-15 20:41 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.06 KB, patch)
2019-03-15 21:14 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-03-05 14:47:47 PST
There is no need to allow connections to the accessibility server when accessibility is turned off.
Comment 1 Radar WebKit Bug Importer 2019-03-05 14:48:36 PST
<rdar://problem/48615720>
Comment 2 chris fleizach 2019-03-05 14:49:07 PST
might depend how we're checking accessibility. this could be a bit risky given all the ways AX can be turned on
Comment 3 Radar WebKit Bug Importer 2019-03-05 14:51:34 PST
<rdar://problem/48615865>
Comment 4 Per Arne Vollan 2019-03-05 15:27:26 PST
Created attachment 363694 [details]
Patch
Comment 5 chris fleizach 2019-03-05 15:30:48 PST
Comment on attachment 363694 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
> +    SandboxExtension::Handle handle;

are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification
Comment 6 Per Arne Vollan 2019-03-05 15:36:15 PST
(In reply to chris fleizach from comment #5)
> Comment on attachment 363694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363694&action=review
> 
> > Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
> > +    SandboxExtension::Handle handle;
> 
> are you handling when AX turns off after process launched by monitoring
> kAXSApplicationAccessibilityEnabledNotification

Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off.

Thanks for reviewing!
Comment 7 chris fleizach 2019-03-05 15:37:34 PST
Comment on attachment 363694 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
>>> +    SandboxExtension::Handle handle;
>> 
>> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification
> 
> Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off.
> 
> Thanks for reviewing!

I was thinking if AX turns on after the web process has already started
Comment 8 Per Arne Vollan 2019-03-05 15:40:53 PST
(In reply to chris fleizach from comment #7)
> Comment on attachment 363694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363694&action=review
> 
> >>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
> >>> +    SandboxExtension::Handle handle;
> >> 
> >> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification
> > 
> > Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off.
> > 
> > Thanks for reviewing!
> 
> I was thinking if AX turns on after the web process has already started

Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient?
Comment 9 chris fleizach 2019-03-05 16:23:25 PST
Comment on attachment 363694 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
>>>>> +    SandboxExtension::Handle handle;
>>>> 
>>>> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification
>>> 
>>> Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off.
>>> 
>>> Thanks for reviewing!
>> 
>> I was thinking if AX turns on after the web process has already started
> 
> Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient?

no I don't think so. if you launch safari, then you turn on AX, this will fail.. you need to listen for that notification too and unblock
Comment 10 Per Arne Vollan 2019-03-06 11:39:31 PST
Created attachment 363765 [details]
Patch
Comment 11 Per Arne Vollan 2019-03-06 11:40:07 PST
(In reply to chris fleizach from comment #9)
> Comment on attachment 363694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363694&action=review
> 
> >>>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56
> >>>>> +    SandboxExtension::Handle handle;
> >>>> 
> >>>> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification
> >>> 
> >>> Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off.
> >>> 
> >>> Thanks for reviewing!
> >> 
> >> I was thinking if AX turns on after the web process has already started
> > 
> > Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient?
> 
> no I don't think so. if you launch safari, then you turn on AX, this will
> fail.. you need to listen for that notification too and unblock

Thanks, Chris! I have updated the patch.
Comment 12 EWS Watchlist 2019-03-06 11:42:20 PST
Attachment 363765 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 chris fleizach 2019-03-06 11:49:46 PST
Comment on attachment 363765 [details]
Patch

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

looks ok from ax perspective.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:431
> +            WebProcessProxy* process = m_processes[i].get();

could probably reduce this to one line

m_processes[I]->unblockAccessibilityServerIfNeeded();
Comment 14 Per Arne Vollan 2019-03-06 12:06:13 PST
Created attachment 363769 [details]
Patch
Comment 15 EWS Watchlist 2019-03-06 12:08:06 PST
Attachment 363769 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:432:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Per Arne Vollan 2019-03-06 12:16:25 PST
Created attachment 363771 [details]
Patch
Comment 17 EWS Watchlist 2019-03-06 12:18:16 PST
Attachment 363771 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 chris fleizach 2019-03-06 12:20:08 PST
LGTM, but I'm not a WK2 reviewer
Comment 19 Brent Fulgham 2019-03-06 12:23:52 PST
Comment on attachment 363771 [details]
Patch

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

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:93
> +            return sandbox_extension_issue_mach_to_process_by_pid("com.apple.webkit.extension.mach", path, 0, pid.value());

Should this be "com.apple.webkit.extensions.mach"_s, since it's a String being passed?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:429
> +    m_accessibilityEnabledObserver = [[NSNotificationCenter defaultCenter] addObserverForName:(__bridge id)kAXSApplicationAccessibilityEnabledNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *note) {

Should we add an UNUSED_PARAM(note) here?
Comment 20 Brent Fulgham 2019-03-06 12:24:30 PST
iOS build failure:

/Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:431:108: error: use of undeclared identifier 'kAXSApplicationAccessibilityEnabledNotification'
    m_accessibilityEnabledObserver = [[NSNotificationCenter defaultCenter] addObserverForName:(__bridge id)kAXSApplicationAccessibilityEnabledNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *note) {
                                                                                                           ^
1 error generated.
Comment 21 Per Arne Vollan 2019-03-06 13:00:06 PST
Created attachment 363775 [details]
Patch
Comment 22 Per Arne Vollan 2019-03-06 13:06:45 PST
Created attachment 363777 [details]
Patch
Comment 23 Per Arne Vollan 2019-03-06 13:07:24 PST
Thanks for reviewing, all! I have updated the patch.
Comment 24 EWS Watchlist 2019-03-06 13:10:14 PST
Attachment 363777 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Brent Fulgham 2019-03-06 13:20:43 PST
Comment on attachment 363777 [details]
Patch

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

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65
> +            LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno);

Perhaps this should an early return?

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:69
> +        return m_handle != -1;

Is m_handle == 0 a valid response? It didn't used to be.
Comment 26 Per Arne Vollan 2019-03-06 13:35:48 PST
(In reply to Brent Fulgham from comment #25)
> Comment on attachment 363777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363777&action=review
> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65
> > +            LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno);
> 
> Perhaps this should an early return?
> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:69
> > +        return m_handle != -1;
> 
> Is m_handle == 0 a valid response? It didn't used to be.

According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'?
Comment 27 Brent Fulgham 2019-03-06 16:54:05 PST
Comment on attachment 363777 [details]
Patch

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

>>> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65
>>> +            LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno);
>> 
>> Perhaps this should an early return?
> 
> According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'?

I suggest you do an early return here with 'false', and leave the "return m_handle" below unchanged from existing.
Comment 28 Per Arne Vollan 2019-03-07 09:11:18 PST
Created attachment 363882 [details]
Patch
Comment 29 Per Arne Vollan 2019-03-07 09:13:09 PST
(In reply to Brent Fulgham from comment #27)
> Comment on attachment 363777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363777&action=review
> 
> >>> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65
> >>> +            LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno);
> >> 
> >> Perhaps this should an early return?
> > 
> > According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'?
> 
> I suggest you do an early return here with 'false', and leave the "return
> m_handle" below unchanged from existing.

Sounds good, thanks! I have updated the patch.
Comment 30 EWS Watchlist 2019-03-07 09:14:31 PST
Attachment 363882 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Brent Fulgham 2019-03-11 09:51:30 PDT
Comment on attachment 363882 [details]
Patch

Looks good! r=me
Comment 32 Per Arne Vollan 2019-03-11 12:00:30 PDT
(In reply to Brent Fulgham from comment #31)
> Comment on attachment 363882 [details]
> Patch
> 
> Looks good! r=me

Thanks for reviewing!
Comment 33 Per Arne Vollan 2019-03-14 14:34:43 PDT
Created attachment 364688 [details]
Patch
Comment 34 Per Arne Vollan 2019-03-14 14:35:51 PDT
(In reply to Per Arne Vollan from comment #33)
> Created attachment 364688 [details]
> Patch

Rebased.
Comment 35 EWS Watchlist 2019-03-14 14:37:26 PDT
Attachment 364688 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Per Arne Vollan 2019-03-15 10:58:57 PDT
Created attachment 364812 [details]
Patch
Comment 37 EWS Watchlist 2019-03-15 11:00:58 PDT
Attachment 364812 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 WebKit Commit Bot 2019-03-15 13:27:30 PDT
Comment on attachment 364812 [details]
Patch

Clearing flags on attachment: 364812

Committed r243008: <https://trac.webkit.org/changeset/243008>
Comment 39 Truitt Savell 2019-03-15 16:49:22 PDT
Reverted r243008 for reason:

This revision broke High Sierra builders

Committed r243027: <https://trac.webkit.org/changeset/243027>
Comment 40 Ryan Haddad 2019-03-15 16:56:25 PDT
(In reply to Truitt Savell from comment #39)
> Reverted r243008 for reason:
> 
> This revision broke High Sierra builders
> 
> Committed r243027: <https://trac.webkit.org/changeset/243027>

Undefined symbols for architecture x86_64:
  "_sandbox_extension_issue_mach_to_process_by_pid", referenced from:
      WebKit::SandboxExtensionImpl::sandboxExtensionForType(char const*, WebKit::SandboxExtension::Type, WTF::Optional<int>) in UnifiedSource16-mm.o
ld: symbol(s) not found for architecture x86_64

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%28Build%29/builds/13030/steps/compile-webkit/logs/stdio
Comment 41 Per Arne Vollan 2019-03-15 20:41:14 PDT
Created attachment 364905 [details]
Patch
Comment 42 EWS Watchlist 2019-03-15 20:42:45 PDT
Attachment 364905 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Per Arne Vollan 2019-03-15 21:14:04 PDT
Created attachment 364910 [details]
Patch
Comment 44 EWS Watchlist 2019-03-15 21:15:52 PDT
Attachment 364910 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44:  _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 WebKit Commit Bot 2019-03-15 22:44:27 PDT
Comment on attachment 364910 [details]
Patch

Clearing flags on attachment: 364910

Committed r243034: <https://trac.webkit.org/changeset/243034>
Comment 46 Fujii Hironori 2019-03-17 19:08:43 PDT
Committed r243054: <https://trac.webkit.org/changeset/243054>