Removing inline event handlers from the browser UI
Tom Schuster
Hello Firefox team!
As a joint effort of the Firefox and Security teams, we are trying to get rid of inline event handlers (e.g. onclick) from HTML code used in Firefox, primarily for everything in the parent process, including browser.xhtml.
In the past years PwnOwn exploits have leveraged the ability to dynamically add inline event handlers to gain JavaScript code execution, after exploiting the content process[1, 2]. To mitigate these attacks we want to give privileged pages a Content-Security-Policy that blocks inline event handlers from executing. In the recent months our efforts have reduced the number of inline event handlers (in browser.xhtml) from over 600 to under 200. The work is tracked in the meta bug 1890547.
We have done this by removing event handler attributes from HTML and moving them into the associated JavaScript files as addEventListener calls. There are still some unresolved issues, which we are able to address iteratively by starting small (e.g., with the “About Firefox” dialog). If you want to join the effort or are in the process of adding new event handlers, feel free to reach out.
To keep this momentum going, I would like to ask everyone landing new Firefox frontend code to avoid introducing new inline event handlers in HTML.
Thanks to everyone who helped with this effort
Tom Schuster
Nick Alexander
Gijs Kruitbosch
This project sounds worthy, so I wonder: this ask seems lint-able, but I don't see a linting ticket in the dependency tree. Is it not, in fact, lintable? (Perhaps identifying parent process chrome code is equivalent to halting?)
We don't currently have significant linting infrastructure for XHTML. Especially the pre-processed XHTML dialect in our tree has proven to be trickier to lint than one might expect. Notably prettier doesn't actually work with XHTML and IIRC when we asked they were (understandably) not particularly interested in adding support.
Switching to non-X HTML is also non-trivial because of the
prototype cache (formerly known as XUL cache) speed-ups we
currently make use of (ie it would be a bunch of work to avoid a
resulting performance regression). Plus of course we have a lot of
markup so either way switching would further increase the scope of
this already considerable project. :-)
We could try to do plaintext lint errors based on "dumb"
substring matching of `onclick="` and `onclick='` and so on,
though it feels a bit icky, and wouldn't allow fixup. And yes,
isolating parent process chrome code would be a bit difficult to
do, though I wouldn't mind having the linter rule for all non-test
markup in the tree. Practically all in-content markup we ship
should already have CSP in place (e.g. aboutNetError
as a somewhat random example) and none of these handlers will
exist there.
So adding linting is not a bad idea, but so far Tom has been providing patches so quickly, and given the HTML issues it seemed not super-easy to do it "neatly", that I hadn't thought it to be super valuable - the frontend team has been aware of this effort for a little while so I was not expecting a large number of "regressions" as such of people adding more of these.
Once windows have been purged of all the inline event handlers and we add CSP, then that will simply break any more such handlers so linting would be somewhat superfluous ("don't do this thing that doesn't work"). I suppose in theory linting (if appropriately editor-integrated, which I'm not sure we have a good way of doing for our plaintext linting) could still be a faster way for an author to realize _why_ an inline attribute wasn't working... so perhaps that means it is worth doing?
I defer somewhat to Tom's judgment here. But hopefully the context helps in terms of why we haven't "just" done this so far.
~ Gijs
Nick Alexander
I defer somewhat to Tom's judgment here. But hopefully the context helps in terms of why we haven't "just" done this so far.
Tom Schuster
Gijs answer. I am mostly driven by preventing inline event handlers
being added by exploit code, which linting wouldn't prevent. I would
rather spend my time on fixing the inline event handlers and adding
the CSP instead of trying to figure out how to make linting work for
XHTML.
Tom