Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use global.location.host over '*' in global.postMessage #542

Merged
merged 1 commit into from
May 10, 2019
Merged

Use global.location.host over '*' in global.postMessage #542

merged 1 commit into from
May 10, 2019

Conversation

tannerlyons
Copy link

This addresses a security concern where messages can be intercepted and/or sent to
any window that uses '*' as the 2nd parameter (the origin) to postMessage.

Per the OWASP HTML5 Security Cheat Sheet:

When posting a message, explicitly state the expected origin as the second argument to postMessage rather than * in order to prevent sending the message to an unknown origin after a redirect or some other means of the target window's origin changing.

MDN's docs also state:

Always provide a specific targetOrigin, not *, if you know where the other window's document should be located. Failing to provide a specific target discloses the data you send to any interested malicious site

I see 2 issues with this PR and would appreciate your feedback:

  1. Testing this doesn't seem to be an option in the current framework, as none of the browsers in the test framework allow location.host modification to actually reproduce this. This is a good thing for those browsers :)
  2. window.location isn't actually part of the ES3 standard. It's implemented a bit differently in each browser. Since this code branch is only run in browsers that support postMessage, we can assume window.location.host is implemented because it was adopted by major browsers at the time of postMessage. I've not been able to find any great resource proving this, it's mostly from memory which feels bad.

The reason this is important for me is that to use this fantastic library in the USA government cloud (aws/azure/etc.), you must adhere to certain security scanners. The scanner has been able to exploit this to pass messages between different hosts. I'm guessing this is not using a major browser, but some other browser with security features turned off. I'm guessing that merging this PR would allow many more people here to use this library.

One last thing: clearly in reading this code there's not REALLY a big vulnerability. a message is posted, and then immediately executed on the next loop, then forgotten. Other windows can really only see the unique id of the deferred function. You can imagine, however, that someone might be able to do something with those, but I cannot think of a reasonable scenario where this would actually affect anything. The biggest concern here is the government scanner.

Thank you for this excellent lib!!!

This addresses a security concern where messages can be intercepted and/or sent to
any window that uses '*' as the 2nd parameter (the origin) to postMessage.
@zloirock
Copy link
Owner

IIRC initially (before core-js publishing) I used something like proposed in this PR, but for some reason, it was changed to '*', I don't remember why. I know that it's not recommended way, but I don't see any possible vulnerabilities here. I think that this PR does not cause problems, so I'll try to accept it with some minor changes.

@zloirock zloirock merged commit dc1aa75 into zloirock:master May 10, 2019
@tannerlyons
Copy link
Author

Thanks!!

zloirock added a commit that referenced this pull request May 13, 2019
@ddillard
Copy link

Is a CVE going to be obtained for this issue? I can get one assigned if that would help.

@dominykas
Copy link

dominykas commented May 14, 2019

Is a CVE going to be obtained for this issue?

I don't think a CVE is relevant here. All that is contained in the messages that are sent is the numerical counter and the number is then checked against the local queue of functions to be called.

So there of course can be bugs, resulting in a DoS or something (if you have a really wild imagination), if a random number comes in over the channel at just just the very right time (a SUPER short timespan), and the function is called too early. But it is meant to be called as soon as possible, so I don't think even that is a concern.

This is a solid implementation and I see no way to exploit that.

The bigger concern is that for every deferral a listener is added and never removed, so that might be something worth fixing up (by ensuring addListener is only called once?) to avoid memory leaks or something, but that is outside of the original scope and unrelated to the original problem.

@dominykas
Copy link

dominykas commented May 14, 2019

I suppose the broadcast would be sending numbers to other websites using other libraries, which can then interpret those numbers in a weird manner, which is definitely a bug, and might cause other apps misbehave, but I'm not sure this can be classified as a security issue - a malicious website can do that in a much easier way without even bothering with core-js and a website which uses core-js is unlikely to be easily forced to send such messages in a malicious fashion.

@ddillard
Copy link

Per Snyk this issue can cause XSS, see https://snyk.io/vuln/SNYK-JS-COREJS-174627. If that's correct, that's a security issue.

@dominykas
Copy link

@ddillard snyk have amended this and they no longer classify this as a vulnerability:

This was deemed not a vulnerability.

@tannerlyons
Copy link
Author

@zloirock do you have an eta on deploying 3.0.2? My team is hoping for an estimate.

@ddillard
Copy link

@ddillard snyk have amended this and they no longer classify this as a vulnerability:

This was deemed not a vulnerability.

Good news! Ok, never mind then! :-)

@zloirock
Copy link
Owner

@tannerlyons it will be 3.1.0. After some days. I have no concrete date since now I have some much more serious problems.

@tannerlyons
Copy link
Author

@zloirock Thank you for the update. We'll react accordingly here 😄

xmo-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 21, 2022
Our current version is getting rather outdated. Replace by
the *legacy* bundle: pdfjs now provides a non-polyfilled version of
the library, which is probably faster though it doesn't save overly
much (for the entire bundle anyway). Use polyfilled version for
safety, though it's unclear whether non-chromium Edge is still
supported by Odoo. If not, we could just use the non-polyfilled
version.

The difference is quite large for pdf.js (+32.5%), however it is much
less consequential for the sandbox (+1%) or for the much larger
worker (+5.6%). The biggest difference is likely performances
but... who knows?

Notes:

- The main reason for this change is that automated vulnerability
  scanner have apparently started scanning for `postMessage(..., '*')`
  and the previous bundles includes a version of corejs polyfills
  without zloirock/core-js#542), therefore triggering those scans. As
  the PR notes this is almost certainly not a concern because of the
  innocuous payload, but there is no reason to waste time on those
  reports if we don't *have* to.
- The bundle now includes "standard fonts", those were removed as
  they're heavy and may not be necessary for our usage (?).
- All the bitmap images were dropped and replaced by svgs, which is
  nice.
- The local changes since the previous update were *not* impacted in
  this, the entire thing was just reset to upstream. This means
  changes which were backported (922c7c7, 6943714) are superseded
  but more odoo-specific changes will have to be reapplied in further
  commits.
xmo-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 25, 2022
Our current version is getting rather outdated. Replace by
the *legacy* bundle: pdfjs now provides a non-polyfilled version of
the library, which is probably faster though it doesn't save overly
much (for the entire bundle anyway). Use polyfilled version for
safety, though it's unclear whether non-chromium Edge is still
supported by Odoo. If not, we could just use the non-polyfilled
version.

The difference is quite large for pdf.js (+32.5%), however it is much
less consequential for the sandbox (+1%) or for the much larger
worker (+5.6%). The biggest difference is likely performances
but... who knows?

Notes:

- The main reason for this change is that automated vulnerability
  scanner have apparently started scanning for `postMessage(..., '*')`
  and the previous bundles includes a version of corejs polyfills
  without zloirock/core-js#542), therefore triggering those scans. As
  the PR notes this is almost certainly not a concern because of the
  innocuous payload, but there is no reason to waste time on those
  reports if we don't *have* to.
- The bundle now includes "standard fonts", those were removed as
  they're heavy and may not be necessary for our usage (?).
- All the bitmap images were dropped and replaced by svgs, which is
  nice.
- The local changes since the previous update were *not* impacted in
  this, the entire thing was just reset to upstream. This means
  changes which were backported (922c7c7, 6943714) are superseded
  but more odoo-specific changes will have to be reapplied in further
  commits.
robodoo pushed a commit to odoo/odoo that referenced this pull request Oct 26, 2022
Our current version is getting rather outdated. Replace by
the *legacy* bundle: pdfjs now provides a non-polyfilled version of
the library, which is probably faster though it doesn't save overly
much (for the entire bundle anyway). Use polyfilled version for
safety, though it's unclear whether non-chromium Edge is still
supported by Odoo. If not, we could just use the non-polyfilled
version.

The difference is quite large for pdf.js (+32.5%), however it is much
less consequential for the sandbox (+1%) or for the much larger
worker (+5.6%). The biggest difference is likely performances
but... who knows?

Notes:

- The main reason for this change is that automated vulnerability
  scanner have apparently started scanning for `postMessage(..., '*')`
  and the previous bundles includes a version of corejs polyfills
  without zloirock/core-js#542), therefore triggering those scans. As
  the PR notes this is almost certainly not a concern because of the
  innocuous payload, but there is no reason to waste time on those
  reports if we don't *have* to.
- The bundle now includes "standard fonts", those were removed as
  they're heavy and may not be necessary for our usage (?).
- All the bitmap images were dropped and replaced by svgs, which is
  nice.
- The local changes since the previous update were *not* impacted in
  this, the entire thing was just reset to upstream. This means
  changes which were backported (922c7c7, 6943714) are superseded
  but more odoo-specific changes will have to be reapplied in further
  commits.

Part-of: #100067
robodoo added a commit to odoo/odoo that referenced this pull request Oct 26, 2022
Our current version is getting rather outdated. Replace by the *legacy* bundle: pdfjs now provides a non-polyfilled version of the library, which is probably faster though it doesn't save overly much (for the entire bundle anyway). Use polyfilled version for safety, though it's unclear whether non-chromium Edge is still supported by Odoo. If not, we could just use the non-polyfilled version.

The difference is quite large for pdf.js (+32.5%), however it is much less consequential for the sandbox (+1%) or for the much larger worker (+5.6%). The biggest difference is likely performances but... who knows?

Notes:

- The main reason for this change is that automated vulnerability scanner have apparently started scanning for `postMessage(..., '*')` and the previous bundles includes a version of corejs polyfills without zloirock/core-js#542), therefore triggering those scans. As the PR notes this is almost certainly not a concern because of the innocuous payload, but there is no reason to waste time on those reports if we don't *have* to.
- The bundle now includes "standard fonts", those were removed as they're heavy and may not be necessary for our usage (?).
- All the bitmap images were dropped and replaced by svgs, which is nice.
- Previously local / bespoke changes:

  - 32c8922 DOM dispatching has been removed, hook into pdfjs's event system
  - 2d5dd14 seems to be the default now
  - 74fa91e, re-apply change in default configuration
- pdfjs has migrated from lots of its objects / operations being / returning promises, to the result objects containing promises, this has required quite a few adaptations in website_slides

closes #100067

Related: odoo/enterprise#31692
Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>
bouvyd pushed a commit to odoo-dev/odoo that referenced this pull request Apr 2, 2024
Our current version is getting rather outdated. Replace by
the *legacy* bundle: pdfjs now provides a non-polyfilled version of
the library, which is probably faster though it doesn't save overly
much (for the entire bundle anyway). Use polyfilled version for
safety, though it's unclear whether non-chromium Edge is still
supported by Odoo. If not, we could just use the non-polyfilled
version.

The difference is quite large for pdf.js (+32.5%), however it is much
less consequential for the sandbox (+1%) or for the much larger
worker (+5.6%). The biggest difference is likely performances
but... who knows?

Notes:

- The main reason for this change is that automated vulnerability
  scanner have apparently started scanning for `postMessage(..., '*')`
  and the previous bundles includes a version of corejs polyfills
  without zloirock/core-js#542), therefore triggering those scans. As
  the PR notes this is almost certainly not a concern because of the
  innocuous payload, but there is no reason to waste time on those
  reports if we don't *have* to.
- The bundle now includes "standard fonts", those were removed as
  they're heavy and may not be necessary for our usage (?).
- All the bitmap images were dropped and replaced by svgs, which is
  nice.
- The local changes since the previous update were *not* impacted in
  this, the entire thing was just reset to upstream. This means
  changes which were backported (922c7c7, 6943714) are superseded
  but more odoo-specific changes will have to be reapplied in further
  commits.

Part-of: odoo#100067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants