1596360 - Monitor innerHTML calls in chrome code and on about: pages and assert we can not introduce new calls to innerHTML

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Ideally we shouldn't use .innerHTML() within chrome code or on any of our about: pages. In an effort we made sure that all calls to InnerHTML() pass through our internal fragment sanitizer which obviously reduces the risk. We do not do that for about: pages though. The fact that some about: pages are semi-privileged increases the risk of XSS in addition.

Within this bug we will add a dynamic runtime monitor of calls to innerHTML in chrome code as well as all of our about: pages. We will start by adding an allowlist which basically allows legacy code to work, but we will assert that no new instances of innerHTML can be added within chrome code or any of our about: pages.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1596360: Monitor the fragment parser in chrome code and in about: pages and assert that no new calls e.g. to innerHTML and friends can be introduced. r=jkt,gijs

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80ab0927b40
Monitor the fragment parser in chrome code and in about: pages and assert that no new calls e.g. to innerHTML and friends can be introduced. r=jkt,Gijs

Comment 3

[Oana Pop-Rus]

Backed out changeset b80ab0927b40 (bug 1596360) for dt failures in DOMSecurityMonitor.cpp

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Csuperseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&fromchange=b049edb3d213d8d23c0c4eadb519921886ba2224&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-devtools-chrome-e10s-7%2Cm%28dt7%29&selectedJob=284403363

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284403363&repo=autoland&lineNumber=1183

Backout: https://hg.mozilla.org/integration/autoland/rev/7a3be2bbce032721ec01a9b75d88cc7c6b089825

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Oana Pop-Rus from comment #3)

Backed out changeset b80ab0927b40 (bug 1596360) for dt failures in DOMSecurityMonitor.cpp

Seems like I forgot to allowlist react-dom.js :-(

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/830e263d8fca
Monitor the fragment parser in chrome code and in about: pages and assert that no new calls e.g. to innerHTML and friends can be introduced. r=jkt,Gijs

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/830e263d8fca

Comment 7

[[e-mail]]

  // check if there is a JS caller, if not, then we can can return early here
  // because we only care about calls to the fragment parser (e.g. innerHTML)
  // originating from JS code.
  nsAutoString filename;
  uint32_t lineNum = 0;
  uint32_t columnNum = 0;
  JSContext* cx = nsContentUtils::GetCurrentJSContext();
  if (!nsJSUtils::GetCallingLocation(cx, filename, &lineNum, &columnNum)) {
    return;
  }

I think other similar callers null-check cx here (it can definitely be null).

Comment 8

[[e-mail]]

(I've since worked out how to avoid that scenario.)