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

Snow stops playing nice - security first at the cost of everything else #133

Merged
merged 24 commits into from
Aug 3, 2023

Conversation

weizman
Copy link
Member

@weizman weizman commented Jul 30, 2023

Changes

  • Revert Enforce Snow integration with CSP #118 tests infra and excessive dependency on CSP (read further @ Motivation).
  • No more BLOCKED_BLOB_MSG - when encountering a malicious URL creation, throw and break program.
  • No more trying to wrap all possible string-html based bypasses - this vector was proven the most problematic to defend against, from now on when encountering suspicious string-html injection attempts, throw and break program.
    • This allows us to avoid double HTMLing, which saves us the trouble in fighting mXSS off.
    • No more iframes with srcdoc prop - saves so much trouble.
  • No more best-trying to defend against string-JS attacks - fully counting on unsafe-inline CSP.
  • No more document.write calls inside iframes - can't trust them. Instead throw and break program.
  • Refactor error handling so that when a forbidden action occurs Snow actually throws an exception and intentionally breaks the program.
    • Snow stops playing nice and shifts over to a "fully secure environment or nothing" mode (read more below @ Motivation).
  • Update tests infra to catch and expect Snow exceptions when are rightfully thrown.

Motivation

Taken from Snow stops playing nice - security first at the cost of everything else

In hindsight, the #118 effort was wrongly looked at partially.

Yes, requiring Snow users to integrate CSP (specifically disable unsafe-inline and same origin object-srcs) to achieve full protection was the right move, but #118 was assuming that with such help from CSP Snow could continue the same "we do anything in our power to allow all sorts of ways of creating same origin realms" vibe.

Quickly after #76, which was heavily based on #118 thesis, it became clear that the implementation was wrong (thanks you @mmndaniel) and that in fact #118 did more damage than good and still allowed most of the sophisticated vulns bypass Snow.

This brought us to the unfortunate realization that the approach Snow kicked off with 1y ago is no longer viable knowing what we know today about same origin realms, and that it's time to adopt a less permissive but more promising approach.

The dream originally was to create a shim that anyone can install taking 2 core rules into account:

  1. Protect ALL same origin realms (aka "1st rule") - no JS code should be able whatsoever to introduce a new same origin realm that goes under Snow's radar.

  2. Require little to zero adaptation (aka "2nd rule") - The website/web app to adopt Snow should do NOTHING more than installing it and passing it a callback. That means Snow must work perfectly regardless of what CSP is configured, and what crazy things the app might attempt to do - Snow must not rely on any additional help from the website, nor break anything the JS in the website runs.

I was inspired to create Snow after realizing how big of an issue same origin realms are for third party vendors trying to develop JS runtime protection tools for websites. It became clear to me that all such tools will fail to accomplish their mission if they don't have full control over their same origin realms (I talk about that realization quite a lot).

The 2nd rule was inspired by my experience with developing such third party tools specifically for websites, knowing how websites allow such wacky JS code to run in their site (a behaviour that doesn't seem to go anywhere in the near future), which requires such protection tools to be as flexible and agnostic to its runtime environment as possible, regardless of how crazy it might be (and websites are CRAZY). Naturally, if I'd wanted such third party security vendors to integrate Snow, the 2nd rule was non-negotiable.

And with that in mind, Snow was created.

Another important thing that happened in the past year was my introduction to the @Agoric team and to SES.

Without getting to much into it, what you do need to know is that SES is a successful attempt to export tools that when are used correctly can harden a given JS environment against certain types of attack completely.

This specifically is something both SES and Snow share - the identification of a certain (or a set of) security concern(s) and the attempt to form an environment that is resilient to it completely using a JS shim (not to say the efforts and accomplishments of Snow and SES are comparable, that would be UNBELIEVABLY pretentious).

One distinction I had noticed though, was that SES took some choices that were naturally very limiting of the JS environment it was destined to run in, whereas this was the opposite from Snow's initial approach (aka the 2nd rule).

At first, I was rather glad that Snow didn't have to go down a similar path (SPOILER ALERT: As you can realize from reading this, that isn't the case anymore).

But now, 1y later, after being exposed to so many vulnerabilities in Snow, or more accurately - SO MANY WAYS to create same origin realms - it's more clear to me where the SES project was coming from.

If my original thinking was that at first priority the adapting websites must remain fully agnostic to the existence of Snow (2nd rule), I now understand that this comes in the cost of security (1st rule). And just like SES, I believe the 1st rule is far more important than the 2nd rule (at least in this case), and that it's time to change the prioritization of this project.

This is yet another (quite long) way of saying:

With this PR, Snow officially puts less effort in making sure websites don't break, and more effort in securing all same origin realms from each other in the website.

This means that:

  1. Bypassing Snow should become significantly harder (hopefully)
  2. Snow might no longer allow certain operations to take place and instead will throw an error to protect the page completely. Most of these operations will still be rather unique and unneeded for the average website, but when they happen, Snow will bail on the operation in sake of keeping the app protected.
    2.1. Also, Snow's protection will still require CSP integration, there's no escape from that (Enforce Snow integration with CSP #118 was right about that).

This also means Snow becomes less of a fit for traditional websites (e.g. booking.com) and third party vendors aiming to be integrated in all sorts of wacky JS envs (e.g. PX CodeDefender), but a better fit for those who can tolerate some initial Snow integration pain in order to become a truly same-origin-realms-attacks resilient app.

Which is unfortunate considering this was the main target audience of Snow in the beginning, but this is what's right - security is more important.

Because on the other hand, this move will allow apps that can adopt Snow to become truly secured against the same origin realms problem. We can already see this approach being vital in protecting the @MetaMask app (which is the sole maintainer of Snow atm).

IMPORTANT NOTE: this does not mean websites and third party tools will fail to adopt Snow into them, it just means it MIGHT be potentially harder. This version for example works just fine on most major websites in the world.

Thank you @Agoric for the inspiration with SES to think better about security.

@weizman
Copy link
Member Author

weizman commented Aug 2, 2023

Progress

  • revert entire CSP based tests infra
  • change html.js so that instead of trying to secure string-based attacks, drop them completely
    • this means zero types of frame are allowed via innerHTML/outerHTML/srcdoc
    • malignmark,mglyph are forbidden to prevent mXSS
    • declarative shadows are still not allowed
  • adjust tests infra accordingly
    • fix tests that use string based attacks switch to DOM insertions instead (except for test/html.js and some of test/edge.js of course)
    • specify and programmatically skip tests that Snow can't handle without CSP
  • test Snow live
    • against popular websites to see if this breaks anything
    • against MetaMask
  • refactor error/warning handleing
    • a lot of mess there, it's about time we:
      • throw errors instead of console.error them
      • never throw and never bail on warns
      • recategorize which are errors and which are warns
      • lose "bail" pattern (either throw to stop or warn and don't stop)
  • communicate
    • update README and docs so that it's clear that Snow no longer plays nice, but instead intentionally breaks anything it can't actually protect - mission from now on is to shape a perfectly secure environment in the cost of losing basic features
    • still need to make sure it is clear CSP is needed, regardless of this change
  • MAYBE consider allowing frames via inner/outer HTML, as long as srcdoc is still removed?
  • Add support in "strict testing" where the new config object can be configured via env vars, then make ci run both cases, making the non-strict required and the strict strictly informative but non blocking
  • Release this version, ideally deprecate Release 2.0.0 #76 because it's shit and roll this out instead

@weizman weizman temporarily deployed to github-pages August 2, 2023 15:45 — with GitHub Pages Inactive
@weizman weizman temporarily deployed to github-pages August 2, 2023 17:42 — with GitHub Pages Inactive
@weizman weizman changed the title attempt to remove csp effort Snow stops playing nice - security first at the cost of everything else Aug 2, 2023
@weizman weizman marked this pull request as ready for review August 2, 2023 17:45
@weizman weizman temporarily deployed to github-pages August 3, 2023 10:09 — with GitHub Pages Inactive
@weizman weizman mentioned this pull request Aug 3, 2023
@weizman weizman temporarily deployed to github-pages August 3, 2023 10:32 — with GitHub Pages Inactive
@weizman weizman temporarily deployed to github-pages August 3, 2023 10:35 — with GitHub Pages Inactive
@weizman weizman temporarily deployed to github-pages August 3, 2023 10:40 — with GitHub Pages Inactive
@weizman weizman merged commit 952fb5f into main Aug 3, 2023
4 of 5 checks passed
@weizman weizman deleted the revert-csp-effort branch August 3, 2023 15:44
@weizman weizman mentioned this pull request Aug 3, 2023
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.

1 participant