1979050 - Import map JSON parsing code can be tricked by overwriting Object.prototype properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tom Schuster (Away until Aug 31)]

Attachment: Testcase

The import map parsing code first uses JS_ParseJSON to parse the JSON string into a JS object. Afterwards it uses JS_GetProperty to lookup the properties of the object like "imports". Unfortunately JS_GetProperty behaves like Reflect.get in JS, this means it will walk the prototype chain. By modifying the Object.prototype, e.g. by adding an imports or integrity property, before the import map is parsed, the code becomes confused.

In the attached testcase, I can trick Firefox into treating "square" as a valid module path, even though the import map is empty. This doesn't reproduce in Chrome. I am not quite sure if this is also a specification issue.

Comment 1

[Tom Schuster (Away until Aug 31)]

I just realized that if we add support for multiple import maps, it's probably going to be easier to manipulate the imports, even in a sanctioned way. This is probably low severity, unless there is something bad you could do with a getter instead of a data property.

Comment 2

[Tooru Fujisawa [:arai]]

The relevant part of the spec would be the following, especially the step 1 and step 4:

https://html.spec.whatwg.org/multipage/webappapis.html#parse-an-import-map-string

To parse an import map string, given a string input and a URL baseURL:

  1. Let parsed be the result of parsing a JSON string to an Infra value given input.
  2. If parsed is not an ordered map, then throw a TypeError indicating that the top-level value needs to be a JSON object.
  3. Let sortedAndNormalizedImports be an empty ordered map.
  4. If parsed["imports"] exists, then:
    1. If parsed["imports"] is not an ordered map, then throw a TypeError indicating that the value for the "imports" top-level key needs to be a JSON object.

The step 1 parses the JSON into an "Infra value", which does JSON.parse and then convert it into "Infra value".

https://infra.spec.whatwg.org/#parse-a-json-string-to-an-infra-value

To parse a JSON string to an Infra value, given a string string:

  1. Let jsValue be ? Call(%JSON.parse%, undefined, « string »).
  2. Return the result of converting a JSON-derived JavaScript value to an Infra value, given jsValue.

The conversion is done in the following.
The property access is done with regular "Get", but only for properties returned by [[OwnPropertyKeys]]().

https://infra.spec.whatwg.org/#convert-a-json-derived-javascript-value-to-an-infra-value

To convert a JSON-derived JavaScript value to an Infra value, given a JavaScript value jsValue:

  1. If jsValue is null, jsValue is a Boolean, jsValue is a String, or
     jsValue is a Number, then ...
  2. If IsArray(jsValue) is true:
    ...
  3. Let result be an empty ordered map.
  4. For each key of ! jsValue.[[OwnPropertyKeys]]():
    1. Let jsValueAtKey be ! Get(jsValue, key).
    2. Let infraValueAtKey be the result of converting a JSON-derived
       JavaScript value to an Infra value, given jsValueAtKey.
    3. Set result[key] to infraValueAtKey.
  5. Return result.

The remaining operations are done with the Infra value, which doesn't cause
any side-effect.

Thus, this isn't a spec bug.

https://infra.spec.whatwg.org/#map-exists

An ordered map contains an entry with a given key if there exists an entry with that key. We can also denote this by saying that, for an ordered map map and key key, "map[key] exists".

https://infra.spec.whatwg.org/#map-get

To get the value of an entry in an ordered map map given a key key and an optional default:

  1. If map does not contain key and default is given, then return default.
  2. Assert: map contains key.
  3. Return the value of the entry in map whose key is key.

Our implementation directly uses the value returned by JSON.parse.
So, there are 2 problems:

• (a) properties on the prototypes can be reflected to the result (this bug)
• (b) accessors on the prototypes can be triggered on the access

Then, given that the target is the result of JSON.parse, which is a plain object without any accessor on its own,
as long as we use "GetOwn"-variant, the problem (b) doesn't happen.

So, for example JS_GetOwnPropertyDescriptor could be used.

We could also use js::GetOwnPropertyPure, which isn't affected by (a) nor (b). (but currently it's internal API).

Another option is to use JS::ParseJSONWithHandler, which allow performing the "parse a JSON string to an Infra value" operations at once. Bug 1953305 has a prototype patch that implements general-purpose handler that returns C++ object tree.

Comment 3

[Tom Schuster (Away until Aug 31)]

Thank you arai for the analysis. Maybe we could also use the WebIDL bindings and parse the JSON into a dictionary (i.e. [GenerateInitFromJSON])? Not sure if that would cause observable differences.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Tom Schuster (MoCo) from comment #3)

Thank you arai for the analysis. Maybe we could also use the WebIDL bindings and parse the JSON into a dictionary (i.e. [GenerateInitFromJSON])? Not sure if that would cause observable differences.

This uses a separate clean global. Why does the current import map code use the global from bug 1761938 instead of using a clean throw-away global?

(In reply to Tooru Fujisawa [:arai] from comment #2)

Bug 1953305 has a prototype patch that implements general-purpose handler that returns C++ object tree.

It looks like the purpose of bug 1953305 is to get rid of jsoncpp in Toolkit. Toolkit also seems to use parsing to serde_json::Value, and I take it that we're not trying to get rid of serde_json.

Is there a reason prefer bug 1953305 plus a tree walk in C++ to put data into a domain-specific struct over parsing to serde_json::Value plus a tree walk in Rust to put data into a domain-specific struct other than not having to make the domain-specific struct travel over FFI with cbindgen?

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Henri Sivonen (:hsivonen) from comment #4)

(In reply to Tom Schuster (MoCo) from comment #3)

Thank you arai for the analysis. Maybe we could also use the WebIDL bindings and parse the JSON into a dictionary (i.e. [GenerateInitFromJSON])? Not sure if that would cause observable differences.

This uses a separate clean global. Why does the current import map code use the global from bug 1761938 instead of using a clean throw-away global?

The spec's algorithm is not affected by polluted prototypes.
So, if we properly implement it, this can be done with single global.

Bug 1953305 has a prototype patch that implements general-purpose handler that returns C++ object tree.

It looks like the purpose of bug 1953305 is to get rid of jsoncpp in Toolkit. Toolkit also seems to use parsing to serde_json::Value, and I take it that we're not trying to get rid of serde_json.

Reducing the number of C++ JSON implementations and also improving the performance are the goal of JS::ParseJSONWithHandler API.
I don't have strong opinion for rust impl.

Is there a reason prefer bug 1953305 plus a tree walk in C++ to put data into a domain-specific struct over parsing to serde_json::Value plus a tree walk in Rust to put data into a domain-specific struct other than not having to make the domain-specific struct travel over FFI with cbindgen?

To be clear, the bug 1953305 patch is not required for this bug.

The JS::ParseJSONWithHandler itself allows consuming JSON data without creating a intermediate JS::Value object tree, but with SAX-like callbacks. This is mostly for performance reason.
(There are some other consumers of JS::ParseJSONWithHandler, without intermediate object tree, but more-target-specific data store and state management.)

But given that the state management of the callback is a bit complex, bug 1953305's struct could be used for not-performance-sensitive cases as an alternative option.

If the input data structure is simple, directly using JS::ParseJSONWithHandler will also work here.

Anyway, just using "Own" or "Pure" variant API could be a simpler fix.

Comment 6

[Daniel Veditz [:dveditz]]

I'm not entirely sure how you could abuse this, but loading unintended files because we were tricked seems like an innocent little bomb that could explode later

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b110d7daf94d
https://hg.mozilla.org/integration/autoland/rev/c5750b078653
Part 1: Patch. r=arai
https://github.com/mozilla-firefox/firefox/commit/cb1eed4cb569
https://hg.mozilla.org/integration/autoland/rev/44a06fe68854
Part 2: test. r=arai

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2865881fa7e4
https://hg.mozilla.org/mozilla-central/rev/972886fa447e
Part 1: Patch. r=arai
https://github.com/mozilla-firefox/firefox/commit/4fd7ef4e78ac
https://hg.mozilla.org/mozilla-central/rev/4c1fb8ac1297
Part 2: test. r=arai

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c5750b078653
https://hg.mozilla.org/mozilla-central/rev/44a06fe68854

Comment 12

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:allstars.chh, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox146 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D271970

Comment 14

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: If there's a JS injecting code that defines "imports", or "scopes" or "integrity" property on Object.prototype, later for webpages that using import maps, the module loading can fetch an unexpected module.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Use GetOwnPropertyDescriptor to get the parsed json object's property instead of Object.prototype.
• String changes made/needed: no
• Is Android affected?: yes

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D271971

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D271970

Comment 17

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: If there's a JS injecting code that defines "imports", or "scopes" or "integrity" property on Object.prototype, later for webpages that using import maps, the module loading can fetch an unexpected module.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Use GetOwnPropertyDescriptor to get the parsed json object's property instead of Object.prototype.
• String changes made/needed: no
• Is Android affected?: yes

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D271971

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: (secure)

Comment 23

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/133750c4bfa6
https://hg.mozilla.org/releases/mozilla-beta/rev/c62d1ac573f3
https://github.com/mozilla-firefox/firefox/commit/c8147a75b517
https://hg.mozilla.org/releases/mozilla-beta/rev/52efc4a8e61d

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/57b884750afb
https://hg.mozilla.org/releases/mozilla-esr140/rev/ced3c0a7fbe5

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/b43c7532bc7a
https://hg.mozilla.org/releases/mozilla-esr115/rev/fb4d055a4b61

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/0766687d93bb
https://hg.mozilla.org/releases/mozilla-esr115/rev/f3766750312d

Comment 27

[Dianna Smith [:diannaS]]

backed out of esr 115 for causing bustage

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Dianna Smith [:diannaS] from comment #27)

backed out of esr 115 for causing bustage

I just noticed a typo in the ESR115 patch, I'll fix it and update the patch.

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] from comment #28)

(In reply to Dianna Smith [:diannaS] from comment #27)

backed out of esr 115 for causing bustage

I just noticed a typo in the ESR115 patch, I'll fix it and update the patch.

Fixed and updated the ESR115 patches,
try https://treeherder.mozilla.org/jobs?repo=try&revision=a0d003000daa2e833b9d67fe241bde3a634a49aa

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4cdc7ec9c0d6
https://hg.mozilla.org/releases/mozilla-esr115/rev/6794ed4d561c

Comment 31

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/7d7e241aff24