Bug 155923 - Audit WebCore builtins for user overridable code
Summary: Audit WebCore builtins for user overridable code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-26 00:03 PDT by Joseph Pecoraro
Modified: 2016-04-04 11:50 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] WIP - Needs Tests (4.58 KB, patch)
2016-03-26 00:07 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.30 KB, patch)
2016-03-28 15:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.30 KB, patch)
2016-03-28 15:55 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-03-26 00:03:40 PDT
* SUMMARY
Looking over the different WebCore builtins, it seems there are a few builtins that can use user overridable code.

<script>
<!-- Fetch -->
Array.prototype.forEach = function() { throw "User overriden"; };
new Headers({a:1}) // throws exception
</script>

<script>
<!-- Streams -->
Object.defineProperty = function() { throw "User overriden"; };
new ByteLengthQueuingStrategy({}) // throws exception
</script>

<script>
<!-- Streams -->
Object.defineProperty = function() { throw "User overriden"; };
new CountQueuingStrategy({}) // throws exception
</script>

Not really sure how we can catch this when JSC first parses these scripts. For constructor functions, it seems like it might be possible (Object.defineProperty => @Object.@defineProperty) but for native prototype methods (an overridden Array.prototype.forEach) I'm not sure how feasible that is. Likewise, a catching this at build time, or scanning the source with RegExps would be brittle.
Comment 1 Joseph Pecoraro 2016-03-26 00:07:33 PDT
Created attachment 274981 [details]
[PATCH] WIP - Needs Tests

Haven't yet written tests. Still have to decide where they should go. Its nice that all the test coverage for this code is in imported w3c tests directories, but these kind of tests seem like they would be ridiculous to have as part of w3c. So it'll take a bunch of boilerplate to setup some LayoutTests directories and stuff for this which I'll get around to doing in a bit.
Comment 2 youenn fablet 2016-03-26 02:18:59 PDT
(In reply to comment #1)
> Created attachment 274981 [details]
> [PATCH] WIP - Needs Tests
> 
> Haven't yet written tests. Still have to decide where they should go. Its
> nice that all the test coverage for this code is in imported w3c tests
> directories, but these kind of tests seem like they would be ridiculous to
> have as part of w3c. So it'll take a bunch of boilerplate to setup some
> LayoutTests directories and stuff for this which I'll get around to doing in
> a bit.

Thanks for taking care of that issue.
In the same vein, it might be good to check WebRTC use of JS built-ins.
IIRC, some of WebRTC built-ins are/were using Promise.then directly.

As of tests, imported/w3c might not be a very good place since these shielding-tests are really WebKit specific.
There are already some shielding-tests in LayoutTests/streams.
For fetch, it might make sense to create a new folder.
Comment 3 Saam Barati 2016-03-26 11:48:07 PDT
I wonder if we could write a rule for this.
If the base is private will we ever access a 
public property on it?
I.e can we assume that all accesses on @Object should
be private? And apply the rule recursively through the 
access chain?

Or do we have places where we really do need a private base
and public property?
Comment 4 Xabier Rodríguez Calvar 2016-03-28 03:16:54 PDT
(In reply to comment #3)
> I wonder if we could write a rule for this.
> If the base is private will we ever access a 
> public property on it?
> I.e can we assume that all accesses on @Object should
> be private? And apply the rule recursively through the 
> access chain?
> 
> Or do we have places where we really do need a private base
> and public property?

I think we can assume that all accesses to any @Object-like objects should be strictly private and protected.

And IIRC, in the Streams API code we don't access any public property, all code is based on internal slots.
Comment 5 Joseph Pecoraro 2016-03-28 10:50:49 PDT
(In reply to comment #3)
> I wonder if we could write a rule for this.
> If the base is private will we ever access a 
> public property on it?

There are instances of @RegExp.prototype in JavaScriptCore/builtins and @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that is.
Comment 6 Joseph Pecoraro 2016-03-28 10:59:38 PDT
> Thanks for taking care of that issue.
> In the same vein, it might be good to check WebRTC use of JS built-ins.
> IIRC, some of WebRTC built-ins are/were using Promise.then directly.

I believe that is actually intentional. Promise chaining works with any Promise shaped object (JS libraries that create their own promises and native Promises). As long as the value returned from a promise has a "then" function, it is treated like a promise and chained.
Comment 7 youenn fablet 2016-03-28 11:45:05 PDT
(In reply to comment #6)
> > Thanks for taking care of that issue.
> > In the same vein, it might be good to check WebRTC use of JS built-ins.
> > IIRC, some of WebRTC built-ins are/were using Promise.then directly.
> 
> I believe that is actually intentional. Promise chaining works with any
> Promise shaped object (JS libraries that create their own promises and
> native Promises). As long as the value returned from a promise has a "then"
> function, it is treated like a promise and chained.

There are cases where accessing public properties is fine, like in Promise.catch.
In the case of WebRTC, native promises are used internally, which might make the situation a bit different.

If a user script changes the "then" property of the Promise prototype, some WebRTC code may break, like in RTCPeerConnection.js: 
"""
peerConnection.@queuedAddIceCandidate(candidate).then(...)
"""
Comment 8 Joseph Pecoraro 2016-03-28 15:52:55 PDT
Created attachment 275061 [details]
[PATCH] Proposed Fix
Comment 9 Joseph Pecoraro 2016-03-28 15:55:47 PDT
Created attachment 275062 [details]
[PATCH] Proposed Fix
Comment 10 Joseph Pecoraro 2016-03-28 15:57:46 PDT
> In the case of WebRTC, native promises are used internally, which might make
> the situation a bit different.
> 
> If a user script changes the "then" property of the Promise prototype, some
> WebRTC code may break, like in RTCPeerConnection.js: 
> """
> peerConnection.@queuedAddIceCandidate(candidate).then(...)
> """

Good catch. I had just expected that this was intentional. Are you able to test this? Should we file another bug?
Comment 11 youenn fablet 2016-03-29 00:27:24 PDT
Comment on attachment 275062 [details]
[PATCH] Proposed Fix

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

Looks good to me.
Some suggestions below that could be done as a follow-up

> LayoutTests/ChangeLog:11
> +        * streams/builtin-overrides.html: Added.

We might need to add more tests like these two for fetch or streams API.
These two test filenames should probably be made more specific at some point.

For streams, the test filename is called streams-promises.html, which is probably not specific enough.
For getUserMedia, the test filename is named webkitGetUserMedia-shadowing-then.html, I quite like it.

> Source/WebCore/Modules/fetch/FetchHeaders.js:-54
> -    @Object.@getOwnPropertyNames(headersInit).forEach((name) => {

forEach is also called in line 39 on a Headers object.
This case should probably be handled as well.
Comment 12 youenn fablet 2016-03-29 00:29:37 PDT
(In reply to comment #10)
> > In the case of WebRTC, native promises are used internally, which might make
> > the situation a bit different.
> > 
> > If a user script changes the "then" property of the Promise prototype, some
> > WebRTC code may break, like in RTCPeerConnection.js: 
> > """
> > peerConnection.@queuedAddIceCandidate(candidate).then(...)
> > """
> 
> Good catch. I had just expected that this was intentional. Are you able to
> test this? Should we file another bug?

Sure, I will file a bug for it.
One case to study in details is when chaining then happens, like in RTCPeerConnectionInternals.js/enqueueOperation.
Comment 13 youenn fablet 2016-03-29 00:37:38 PDT
> Sure, I will file a bug for it.
> One case to study in details is when chaining then happens, like in
> RTCPeerConnectionInternals.js/enqueueOperation.

Filed bug 155964.
Comment 14 WebKit Commit Bot 2016-03-29 01:14:39 PDT
Comment on attachment 275062 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 275062

Committed r198776: <http://trac.webkit.org/changeset/198776>
Comment 15 WebKit Commit Bot 2016-03-29 01:14:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 youenn fablet 2016-03-29 01:42:10 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > I wonder if we could write a rule for this.
> > If the base is private will we ever access a 
> > public property on it?
> 
> There are instances of @RegExp.prototype in JavaScriptCore/builtins and
> @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that
> is.

The "prototype" property of @Promise and @RegExp are DontDelete and ReadOnly.
Comment 17 Joseph Pecoraro 2016-03-29 11:06:35 PDT
> > There are instances of @RegExp.prototype in JavaScriptCore/builtins and
> > @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that
> > is.
> 
> The "prototype" property of @Promise and @RegExp are DontDelete and ReadOnly.

Thanks for checking that!