Bug 155923 – Audit WebCore builtins for user overridable code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[youenn fablet]

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

[Saam Barati]

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?

[Xabier Rodríguez Calvar]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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

[youenn fablet]

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

[Joseph Pecoraro]

Created attachment 275061 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 275062 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

[youenn fablet]

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.

[youenn fablet]

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

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 275062 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 275062

Committed r198776: <http://trac.webkit.org/changeset/198776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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

[Joseph Pecoraro]

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