Bug 195342 – [iOS] Block the accessibility server when accessibility is not enabled.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

There is no need to allow connections to the accessibility server when accessibility is turned off.

[Radar WebKit Bug Importer]

<rdar://problem/48615720>

[chris fleizach]

might depend how we're checking accessibility. this could be a bit risky given all the ways AX can be turned on

[Radar WebKit Bug Importer]

<rdar://problem/48615865>

[Per Arne Vollan]

Created attachment 363694 [details]
Patch

[chris fleizach]

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

[Per Arne Vollan]

(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!

[chris fleizach]

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

[Per Arne Vollan]

(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?

[chris fleizach]

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

[Per Arne Vollan]

Created attachment 363765 [details]
Patch

[Per Arne Vollan]

(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.

[EWS Watchlist]

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.

[chris fleizach]

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();

[Per Arne Vollan]

Created attachment 363769 [details]
Patch

[EWS Watchlist]

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.

[Per Arne Vollan]

Created attachment 363771 [details]
Patch

[EWS Watchlist]

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.

[chris fleizach]

LGTM, but I'm not a WK2 reviewer

[Brent Fulgham]

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?

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 363775 [details]
Patch

[Per Arne Vollan]

Created attachment 363777 [details]
Patch

[Per Arne Vollan]

Thanks for reviewing, all! I have updated the patch.

[EWS Watchlist]

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.

[Brent Fulgham]

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.

[Per Arne Vollan]

(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'?

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 363882 [details]
Patch

[Per Arne Vollan]

(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.

[EWS Watchlist]

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.

[Brent Fulgham]

Comment on attachment 363882 [details]
Patch

Looks good! r=me

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #31)
> Comment on attachment 363882 [details]
> Patch
> 
> Looks good! r=me

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 364688 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #33)
> Created attachment 364688 [details]
> Patch

Rebased.

[EWS Watchlist]

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.

[Per Arne Vollan]

Created attachment 364812 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 364812 [details]
Patch

Clearing flags on attachment: 364812

Committed r243008: <https://trac.webkit.org/changeset/243008>

[Truitt Savell]

Reverted r243008 for reason:

This revision broke High Sierra builders

Committed r243027: <https://trac.webkit.org/changeset/243027>

[Ryan Haddad]

(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

[Per Arne Vollan]

Created attachment 364905 [details]
Patch

[EWS Watchlist]

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.

[Per Arne Vollan]

Created attachment 364910 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 364910 [details]
Patch

Clearing flags on attachment: 364910

Committed r243034: <https://trac.webkit.org/changeset/243034>

[Fujii Hironori]

Committed r243054: <https://trac.webkit.org/changeset/243054>