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

Questions and feedback #162

Closed
steve8708 opened this issue Feb 23, 2019 · 6 comments
Closed

Questions and feedback #162

steve8708 opened this issue Feb 23, 2019 · 6 comments

Comments

@steve8708
Copy link

Hey all!

Apologies that this will be a github issue with multiple topics. @erights invited feedback and I'd love to give it and ask questions, and I wanted to move out of this thread as this is about this project and not Caja itself.

Feedback

  • I had some issues getting started, and would find a getting started guide useful. I had to try many things and ran into issues such as this in just trying to get a basic SES snippet to run. I had to fall back to importing ses/dist/ses-shim.js, but that will work browser only. Fortunately I have vm2 in the meantime for server side. I would love to be able to just import SES from 'ses' and be able to write client and server safe code with typical es/cjs modules
  • I would love to have typescript typings for this, as it makes learning a new project (and using it correctly) much easier

Questions

  • What is the full API? Is this docced anywhere?
  • What are endownments and how do I use them? I saw the term on the README and didn't know what it refers to exactly
  • What are the options that can be passed to SES.makeSESRootRealm? I see an example of allowConsole (is this just synonymous with realm.evaluate(code, { console })?)
  • What is the difference between SES and realms/frozen realms. On surface level they seem to say they do the same thing - safe code execution, so I'm curious where exactly they differ given this project depends on one realms shim implementation
  • What is Jessie and how does it fit in here? Looks very interesting, but even with the long readme I don't fully understand what it is and what it's useful for compared to this, but I imagine if it is what I think I may have a use for it, but I need a good example of what it is exactly useful for would be helpful
  • How is async code handled? I've had use cases where I want third party code to do something along the lines of safeFetch('....json').then(data => done(data)), where I pass as context a safeFetch function (that, for example, fetches from another domain via a webworker in a sandboxes iframe) and then can call a done() function I pass to receive the result. Is this blocked in SES? Or is the async code unprotected? Or somehow it is handle? It is understandable if not, there may be ways I can work around this, but just curious
  • Do I need to be safe about what I pass in? E.g. if I wanted to pass a dom node to the evaluate, would there be nothing something stopping the given code to call domNode.ownerDocument.defaultView.eval('bad code')

Additionally, this and realms combined is quite a few kb of code. I understand that there may be no way around this, but I had a naive implementation prior to this that was quite small and went along the lines of -

function naiveSafeEvaluate(cod, context = {}) {
  const ctx = new Proxy(context, {
    get(target, key) {
      if (!Reflect.has(context, key)) {
        return undefined;
      }
      return Reflect.get(context, key);
    },
    set() {
      // No setting
      return false;
    },
  });
  const isExpression = !(code.includes('\n') || code.includes(';'));

  try {
    const result = new Function(
      'context',
      `with (context) {
        ${isExpression ? `return (${code});` : code};
      }`
    ).call(ctx, ctx);
    return result;
  } catch (err) {
    console.warn('Evaluation error:', err);
  }
}

What is the major advantage of using both realms and this project on top of realms when, in my naive understanding, my reference code below accomplishes a similar end. Obviously using with() is ugly and hacky, but I can only imagine for any real polyfill of SES/realms for today's ES5 environments there is some arguably hacky stuff in there too

Additionally, is there any way to have protection against references? I can imagine in my code I'd always keep it safe by only passing in POJOs as context. But it would be interesting if I could still be protected with proxies, e.g. for every get request or function call, the return value (if an object) is wrapped in a proxy, that for every get, checks if the type of what is being returned is safe (e.g. plain object) and is not, say, a reference to the window object

const toString = Object.prototype.toString;
function naiveIsSafe(val) {
  return !['[object Window]', '[object Document]'].includes(toString.call(val)); // + a additional checks, perhaps users can provide hooks to make certain things flagged safe or unsafe too
}

function naiveSafeWrap(obj) {
  if (obj && (typeof obj === 'object' || typeof obj === 'function')) {
    return new Proxy(obj, {
      get(target, key) {
        const response = Reflect.get(obj, key);
        if (!naiveIsSafe(response)) {
          return undefined;
        }
        return naiveSafeWrap(response);
      },
      call() {
        // similar to get above for return types
      },
    });
  }
  return obj;
}

function naiveSafeEvaluate(code, context = {}) {
  const ctx = new Proxy(context, {
    get(target, key) {
      if (!Reflect.has(context, key)) {
        return undefined;
      }
      return naiveSafeWrap(Reflect.get(context, key));
    },
    set() {
      // No setting
      return false;
    },
  });
  const isExpression = !(code.includes('\n') || code.includes(';'));

  try {
    const result = new Function(
      'context',
      `with (context) {
        ${isExpression ? `return (${code});` : code};
      }`
    ).call(ctx, ctx);
    return result;
  } catch (err) {
    console.warn('Evaluation error:', err);
  }
}

In this case, my safeEvaluate function using SES calls eval safeEvaluate('node.ownerDocument.defaultView.eval("bad code")', { node: document.body })

but naiveSafeEvaluate does not. Same here goes for things like accessing cookies if someone accidentally passes something that could possibly reference the window or document even several property accesses away.

Anyway, just wanted to toss this out for thoughts, I can totally see the argument where it is user error to pass unsafe objects to untrusted code and that this is not part of the scope of this project as well.

I guess there is a possibility of having an extra safe mode where everything passed in gets cloned to plain objects, if anyone ever has any worry of issues of any chance of passing in an object with any deep reference to something unsafe

Also, apologies for my any naiveness in my understanding here. This is a very exciting subject to me as I want to safely allow 3rd party code on my website for the tool I make, Builder and I don't think I've seen anything truly safe and viable until now. I am no security expert and I apologize if I am wasting any of your time. Additionally if this is better suited for multiple separate issues just let me know

@katelynsills
Copy link
Contributor

Hi Steve, first, thank you so much for such detailed feedback. This is really fantastic!

@warner and @erights should be able to say a lot more in terms of the particulars. Just so you know, we're devoting the next week to ironing out some of these issues, so you should expect to see a lot more documentation from us as well as some big changes very soon.

@steve8708
Copy link
Author

Awesome, thanks so much @katelynsills!

@warner
Copy link
Contributor

warner commented Feb 23, 2019

Those are excellent questions, and touch on practically every aspect of SES and Caja and the object-capability world from which they come. There's a deep body of thought here, and you're following exactly the right path. We have a lot of lore that needs to get written down and organized to help folks along this path (it's currently scattered among mailing list archives, source code comments, presentations, and inside people's heads).

In particular, I think we need some docs with something like "why doesn't X work?" paired with an attack or an inflexibility about that particular approach, to motivate/justify the complexity of SES.

One concern about that naiveSafeEvaluate is that the evaluated code can modify your primordials to change how e.g. Number or Array work, which would compromise the rest of your code (I think there's a way to do that without flunking your isExpression check but I'm not positive). Doing this in a separate Realm is a good start, but by doing it inside SES is safer because SES freezes the primordials, so two different pieces of code inside the same frozen SES realm can interact with each other and not worry about such tampering.

The secondary concern is that it might use syntax to get access to the original function constructor ((() => 0).__proto__.constructor) and then use that to build functions that access the global scope, bypassing your with block. Your Proxy/with construct is pretty similar to what Realms does internally (see https://github.com/tc39/proposal-realms/blob/5626dcd350392dc09eeeb17669d7f0795395fe34/shim/src/evaluators.js#L59 and the Proxy defined later in that file), but Realms does extra work to protect eval and Function.

Limiting the evaluated code to an expression is kind of a drag too.. you can get a lot of functionality out of that, but it'd be even nicer to be able to define multi-line named functions. Realms makes it safe to evaluate entire programs (including function bodies), which opens up some larger use cases.

I'm not sure if naiveSafeEvaluate is meant to run on Realms or not. SES runs on top of Realms (but there's work underway on a "RealmCompartment" which would basically apply all the protection of Realms to the current environment, instead of making a brand new one, which should be faster/easier at the cost of compatibility with libraries that expect to augment Array or modify other primordials).

The Realms shim is pretty big, but the hope is that it will be turned into a native platform feature (https://github.com/tc39/proposal-realms is tracking the standardization process), so that code will "go away" eventually. SES sits on top of Realms (except maybe for the RealmCompartment project, once that is done), and is mostly concerned with freezing everything and making it easier to evaluate things safely.

For "protection against references", you'll want to read up about "Membranes". The SES/Realms evaluate() is sufficient to execute untrusted code safely, but of course you actually want to interact with the result somehow, and you need to be able to do that safely. Passing a Plain Old Javascript Object is not safe, alas, because they can use it to climb your inheritance chain, get access to your Object constructor, then either modify how Object.prototype works or use it to get an unconfined Function constructor and then access the global from there.

The SES approach is to run almost all your code under SES, both the "trusted" code that you write and the "untrusted" code you get from somewhere else. And to use Harden (https://github.com/Agoric/Harden) on everything before you let it pass the border. Of course for this combined program to actually do anything, you need your trusted code to close over "real" outside/"primal"-Realm objects (which aren't frozen and so aren't safe to reveal to untrusted code), and part of the job of your trusted code is to safely mediate access to these powerful exterior things. Every single object that passes this boundary needs to be wrapped somehow, hence the notion of a "membrane" (think of a glovebox, like in a chemistry lab, and you can take objects out of the box but they automatically get wrapped in a new glove as they exit).

This is a hassle and takes a lot of thought, so the easier approach is to limit the API available to the untrusted code so you don't have to manage all that (strings are safe to pass, so one hack is to JSON-serialize everything and then unserialize it on the other side, which obviously prohibits references). There's a Caja project named "Domado" that is all about applying this "taming" process to the DOM. The code for that will be larger than Realms and SES combined (it's a big gnarly job). We'll be working on Domado in some new form eventually, but it's a big job so it's not going to happen right away.

Hope that gets you started. We should add some pointers to relevant talks here as a jumping off point for more questions, and build some of this into docs/ for future reference.

Welcome to the community!

@steve8708
Copy link
Author

@warner this is incredibly helpful, thank you so much!

All of the additional documentation you describe would be amazing. Knowing more about the internals and how and why things are done helps me be more confident in making the choice for SES as opposed to another alternative and understanding what exactly I'm getting in return for that trade for some extra kb in our distributed bundle size.

It also helps a lot in understanding what we should and should not do by knowing more specifically about what is going on under the hood.

So the gist here is as of right now I need to be sure not to pass any objects into evaluate but only strings (or other primitives like numbers). Would it make sense then to have a validation step that when you call evaluate it should iterate over the values passed and throw an error if any object is passed, to avoid accidental loopholes?

Or potentially to do something like the postmessage API does automatically could be really nice, i.e. clone all objects (effectively JSON.stringify/JSON.parse), and warn if anything unclonable is passed. Likewise other projects like pupeteer that passes objects across environments does this automatically as well - in their case it seems they simply JSON.stringify/parse all arguments which I think would be sufficient for this project as well

I do have a few more Qs drifting around but I'll wait for those docs to get built out before taking up any more of your time duplicating that work.

@katelynsills
Copy link
Contributor

Hi @steve8708, we just released SES 0.5, which should have fixed many (hopefully all!) of the issues that you were facing. I hope that helps, and please let us know if you have any further questions or problems. I am going to close this issue for now, but please feel free to open up a new issue for anything that comes up.

We've also added more documentation and will be writing up more soon.

@steve8708
Copy link
Author

Great, thanks @katelynsills!

@jfparadis jfparadis transferred this issue from Agoric/SES Feb 21, 2020
kriskowal pushed a commit that referenced this issue Jan 12, 2022
Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7.
- [Release notes](https://github.com/jbgutierrez/path-parse/releases)
- [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7)

---
updated-dependencies:
- dependency-name: path-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

No branches or pull requests

3 participants