Pdf.js: text layer style micro-optimization resulted in requiring style-src 'unsafe-inline' in Content Security Policy

[thestinger]

https://github.com/mozilla/pdf.js/commit/cb5f9df0c8932fe0db9f783ede7893b7efcadcdb started generating style strings automatically, which is forbidden by a standard Content Security Policy. It would be nice to have a fallback or perhaps reconsider the approach.

To reproduce, use style-src 'self' in CSP:

    <meta http-equiv="Content-Security-Policy" content="style-src 'self'" />

[Snuffleupagus]

... or perhaps reconsider the approach.

PR #7632 was necessary in order to improve performance of the enhanceTextSelection mode in particular, which is part of issue #7584, so in my opinion I don't think that reverting this should even be considered.

[thestinger]

I'm only suggesting taking other routes to optimizing it. For example, why use 4 padding attributes and then apply them with style instead of merging them?

[thestinger]

Could also still use the static array approach for setting individual style properties instead of doing it all via style. The font-family and font-size properties could be merged like padding.

[Snuffleupagus]

I'm only suggesting taking other routes to optimizing it. For example, why use 4 padding attributes and then apply them with style instead of merging them?

In general there's no guarantee that a particular textDiv needs all four padding values updated with enhanceTextSelection, and the current code provided a simple way to handle that (very common) case.

Could also still use the static array approach for setting individual style properties instead of doing it all via style.

The general line of reasoning behind updating everything at once using style, was to try and avoid unnecessarily invalidating the DOM when creating/updating the textDivs since that seemed to be one of the major contributors to bad performance of the enhanceTextSelection mode.

[thestinger]

In general there's no guarantee that a particular textDiv needs all four padding values updated with enhanceTextSelection, and the current code provided a simple way to handle that (very common) case.

That doesn't mean padding can't be used efficiently. It doesn't need to build new strings for the parts it isn't updating and the end result will be smaller. It can replace a single style attribute set with a single style.padding set and " 0 " can be reused.

The general line of reasoning behind updating everything at once using style, was to try and avoid unnecessarily invalidating the DOM when creating/updating the textDivs since that seemed to be one of the major contributors to bad performance of the enhanceTextSelection mode.

Isn't it happening in a fragment, not the actual DOM?

[yurydelendik]

That doesn't mean padding can't be used efficiently.

@thestinger agree, it will help to address the issue if benchmarks with some performance data was available.

[thestinger]

I'm not really able to measure a difference between different approaches here. The different approaches can be microbenchmarked out of context though.

I think the conflict with unsafe-inline can be reduced without a performance loss by tweaking this and then the optimization can be made conditional if keeping it is important.

It's easy for me to just revert this commit so it's not a high priority for me but I'll try to find someone to work on it. It would be great to have tests for CSP stuff so it can't break in the future (optimizations like this could still happen if important, they'd just need to be conditional).

[bgotink]

There might be a solution that allows for the optimisation as is while supporting websites using CSP.

There are three ways to set inline style as a string:

• element.setAttribute('style', someStyle), which doesn't work when there's a CSP that doesn't allow unsafe-inline
• element.style = someStyle, which is not supported in IE (tested on IE 11, I haven't tested this on Edge so it might be broken there as well)
• element.style.cssText = someStyle, which is supported as of DOM level 2, so even IE supports it (verified on IE 11)

Plunker to show the three methods: https://plnkr.co/edit/T8xrUmR5eSuqDukSEVuw?p=preview

I'd be more than willing to make a pull request changing element.setAttribute('style', ...) to element.style.cssText = ... if you agree on this solution.

[thestinger]

It seems like a browser bug if element.style.cssText = someStyle works without unsafe-eval:

• https://w3c.github.io/webappsec-csp/2/#directive-style-src
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src#Unsafe_style_expressions

[thestinger]

In case anyone else comes across this and wants a solution, this is my current patch rolling this back to the old way of doing it so style-src: 'unsafe-inline' can be avoided:

https://github.com/GrapheneOS/pdf.js/commit/021d2fddb03883054ef4399d1d3df87e0c6ab9ca

It can definitely be optimized a bit, but since that doesn't matter for my usage I'm keeping the patch as minimal as possible for ease of maintenance. I think expecting someone that wants this security issue fixed to care about optimizing the performance is a bit backwards. The performance optimization should have been constrained to what can be done without breaking basic security hygiene. It may even be faster to approach this in a different way as I mentioned above. The commit message for the optimization mentions how difficult it was to even measure a difference, but the security regression is quite easily measured.

The reason that I'm using pdf.js in the first place is security. It allows reusing the hardened / sandboxed browser rendering with all the PDF-specific rendering in a memory safe language (JavaScript). As part of doing this, I'm using CSP to mitigate dynamic code / style injection which could massively increase the attack surface to being like a web page, which is very counter to the goals. I think many other people have a similar use case for pdf.js for avoiding dangerous native PDF rendering libraries. Avoiding unsafe-inline styles is part of making sure that a bug cannot result in accidentally injecting arbitrary styles, opening up a lot of renderer attack surface. For a use case where it's embedded in an actual web site, it matters even more, since lots of evil things can be done with CSS.

I consider this on the same level as expecting native binaries / libraries to have SSP, ASLR (PIE), _FORTIFY_SOURCE=2, -fstack-clash-protection and other baseline mitigations. It's basic security hygiene vs. more advanced things like type-based CFI, trapping integer sanitizers, ShadowCallStack, etc. where I do expect larger projects to be thinking about it and perhaps already working on it but it's not part of the bare minimum. A basic CSP policy with static JavaScript / CSS qualifies as basic security hygiene in my opinion.

[timvandermeij]

Fixed by the pull request above.