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

Enforce Snow integration with CSP #118

Merged
merged 20 commits into from Jul 17, 2023
Merged

Enforce Snow integration with CSP #118

merged 20 commits into from Jul 17, 2023

Conversation

weizman
Copy link
Member

@weizman weizman commented Jul 3, 2023

tl;dr

  • From this PR forward, to get full security by Snow you must use CSP so that:
    • unsafe-inline is not allowed!
      • Can be achieved by specifying the script-src directive to anything as long as it doesn't include the phrase unsafe-inline
    • object-src to self is not allowed!
      • Can be achieved by specifying the object-src directive to anything as long as it doesn't include the phrase self

Motivation

  • This attempts to address the question raised in Is Snow useless without CSP? #109 (visit it for further info on the topic)
    • Too many vulnerabilities were found in Snow's current approach, making us realize we might not be able to keep up without some help from CSP
  • After some research, it seems to come down into 2 specific CSP directives we'd like help from
    • unsafe-inline & unsafe-eval - These 2 are allowing most of the vulns that are too hard to patch, string-JS based attacks are difficult to hermetically defend against.
    • object-src - objects and embeds are also very problematic in their behaviour around contentWindow access and load event emitters
  • Both being very reasonable directives to have installed in a web app are a baseline we feel comfortable with requiring for better Snow protection (especially comparing to the amount of work and energy needed to defend Snow otherwise)

In this PR

  • Update README to make sure it is clear that starting next version this is the case
  • Update demo app to enforce script-src 'self'; object-src 'none';
  • Significant changes to the testing infra:
    • Update WDIO to support switching WDIO to puppeteer in runtime
    • Switch to puppeteer for Chrome tests
    • Use puppeteer to register to the securitypolicyviolation event in all realms by default
    • Set the event listener to report to the test that it has passed thanks to sufficient CSP protection
      • example, test running document.body.innerHTML = '<iframe src="javascript:alert()" />' will pass with CSP-script-src-elem detected
    • We're using puppeteer because it's the only way to register this event to all realms automatically BEFORE Snow
    • For tests that use document.write we have to integrate a static check instead of expecting the event listener to fire - this is because document.write kills event listeners on the document by definition, including securitypolicyviolation
    • Because of us being dependent on puppeteer for this, the CSP test only applies to Chrome - for FireFox and Safari we run the same tests as before without CSP applied
    • In order to apply CSP dynamically on demand we moved from example.com as the same origin to a new app https://weizman.github.io/CSPer/ which accepts a query param csp and sets its value as the CSP of the document using the meta tag.

Notes

  • In Is Snow useless without CSP? #109 it was discussed to adopt a more flexible approach regarding CSP by allowing all sorts of directive and allowing users to test and experiment with the security of Snow based on a given set of CSP directives to their liking,
    • For example, if facebook.com want to use Snow, provide them with an easy way to take their current CSP and test it with Snow's current tests to see if it cuts it.
  • After realizing it really all comes down to just 2 very specific and very legitimate to have CSP directive, we no longer see a need for that.
    • In other words - (1) Use Snow; (2) forbid unsafe-inline and unsafe-eval attacks; (3) forbid object-src by pointing it to either none or to any domain that isn't the same domain as your app
    • As simple as that - a very reasonable request for potential Snow users.

@weizman
Copy link
Member Author

weizman commented Jul 10, 2023

858059e is a big one (with e19c70e also):

  • we can narrow down the minimum needed CSP for Snow that most web apps can agree on to work very well in favor of snow, especially around the most problematic Snow vulns we are aware of currently.
    • That is specifically unsafe-{inline/eval}.
  • So we introduce in this PR that CSP into the Snow demo app and to website we use for testings
  • Integrating CSP requires us to adjust the tests. We now pass them under 2 separate scenarios:
    • if Snow successfully prevented bypass, or
    • if CSP successfully prevented bypass
  • In order to determine the latter, we register to the onsecuritypolicyviolation event to learn of CSP violations, and if any occurred, to populate that to the webdriver test to determine if the CSP violation was a correct defense attempt against Snow bypass.
  • Two problems with this approach:
    • The event is being overwritten when document.write is called, which is a common technique to attempt to bypass Snow in the tests. For those that specifically use document.write, we first call a dedicated function to tell us if such an operation would even work with current CSP. If not, we consider the test as passed.
    • The event is per document, effectively per realm. Since we're dealing with realms, we have to register to that event on all realms. The only way to do that before Snow is by leveraging webdriverio. Luckily, it supports transitioning to puppeteer, which allows us to ask it to execute JS code for every new realm automatically. That required some minor updates in the tests infra as well as the version of webdriverio itself.
  • To allow all of this, we now introduce to all realms a new object called TEST_UTILS which exports functions and values helpful to the tests infra (couldn't have done this otherwise)

In 60dfa89 we discover some FF issues that seem to be bigger than this change. This makes it even clearer that in addition to the decided CSP above we should introduce object-src 'none' also, as object is a problematic one and also shouldn't be used by websites really

@weizman
Copy link
Member Author

weizman commented Jul 11, 2023

Continuing #118 (comment), in 20aae8b we introduce CSP support for object-src too

@weizman weizman changed the title start thinking about snow with csp Enforce Snow integration with CSP Jul 17, 2023
@weizman weizman marked this pull request as ready for review July 17, 2023 08:39
@weizman weizman merged commit 3684e04 into main Jul 17, 2023
2 checks passed
@weizman weizman deleted the snow-csp branch July 17, 2023 09:35
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.

None yet

1 participant