Bug 214669

Summary: Web Inspector: developerExtrasEnabled should be respected when opening local Web Inspector (part 2)
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=214573
Attachments:
Description Flags
Patch
none
Patch none

Description BJ Burg 2020-07-22 18:07:04 PDT
.
Comment 1 BJ Burg 2020-07-22 18:14:24 PDT
Created attachment 405004 [details]
Patch
Comment 2 BJ Burg 2020-07-22 18:15:13 PDT
<rdar://65885126>
Comment 3 Joseph Pecoraro 2020-07-22 18:43:14 PDT
Comment on attachment 405004 [details]
Patch

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

> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:671
> +    if (!m_inspectedPage->preferences().developerExtrasEnabled())
> +        return;

How would it be possible to get here?
Comment 4 BJ Burg 2020-07-23 13:43:11 PDT
Comment on attachment 405004 [details]
Patch

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

>> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:671
>> +        return;
> 
> How would it be possible to get here?

A compromised WebContent process may try to trick UIProcess into using Inspector functionality even if it's disabled. We don't want that to happen.
Comment 5 Joseph Pecoraro 2020-07-23 13:45:51 PDT
Comment on attachment 405004 [details]
Patch

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

>>> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:671
>>> +        return;
>> 
>> How would it be possible to get here?
> 
> A compromised WebContent process may try to trick UIProcess into using Inspector functionality even if it's disabled. We don't want that to happen.

Sounds good. Is this the only command then? It seems `WebInspectorProxy::append` could be concerning as well.
Comment 6 Devin Rousso 2020-07-23 13:46:39 PDT
Comment on attachment 405004 [details]
Patch

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

>>>> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:671
>>>> +        return;
>>> 
>>> How would it be possible to get here?
>> 
>> A compromised WebContent process may try to trick UIProcess into using Inspector functionality even if it's disabled. We don't want that to happen.
> 
> Sounds good. Is this the only command then? It seems `WebInspectorProxy::append` could be concerning as well.

Based on this logic there should probably be a check for `WebInspectorProxy::append` too.
Comment 7 Devin Rousso 2020-07-23 13:47:11 PDT
ah lol @Joe beat me to it :P

r=me as well :)
Comment 8 BJ Burg 2020-07-23 15:07:14 PDT
Comment on attachment 405004 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:671
>>>>> +        return;
>>>> 
>>>> How would it be possible to get here?
>>> 
>>> A compromised WebContent process may try to trick UIProcess into using Inspector functionality even if it's disabled. We don't want that to happen.
>> 
>> Sounds good. Is this the only command then? It seems `WebInspectorProxy::append` could be concerning as well.
> 
> Based on this logic there should probably be a check for `WebInspectorProxy::append` too.

I'll address ::append as well.
Comment 9 BJ Burg 2020-07-23 15:15:47 PDT
Created attachment 405082 [details]
Patch
Comment 10 EWS 2020-07-23 15:48:33 PDT
Committed r264803: <https://trac.webkit.org/changeset/264803>

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