Labs/Jetpack/Reboot FAQ: Difference between revisions
(Copied from https://wiki.mozilla.org/User:Adw/Jetpack_reboot_questions) |
No edit summary |
||
Line 19: | Line 19: | ||
is suddenly subject to exploit because <tt>this</tt> could be set to something the client code passes in, and which they can "fake" to do malicious things. While it's certainly possible to treat <tt>this</tt> suspiciously and write secure code with it, I'd personally much rather just avoid the use of <tt>this</tt> to spare my brain the extra paranoia, unless there's some significant advantage to allowing the client to specify <tt>this</tt> themselves. | is suddenly subject to exploit because <tt>this</tt> could be set to something the client code passes in, and which they can "fake" to do malicious things. While it's certainly possible to treat <tt>this</tt> suspiciously and write secure code with it, I'd personally much rather just avoid the use of <tt>this</tt> to spare my brain the extra paranoia, unless there's some significant advantage to allowing the client to specify <tt>this</tt> themselves. | ||
''adw: OK, I say we outlaw it then, outside of constructors, for all code accepted into the Jetpack platform, including capabilities | ''adw: OK, I say we outlaw it then, outside of constructors, for all code accepted into the Jetpack platform, including capabilities, exceptions where appropriate.'' | ||
'''How should capabilities throw exceptions? Need to do something special to show a proper stack? A common <tt>JetpackError</tt> prototype?''' | '''How should capabilities throw exceptions? Need to do something special to show a proper stack? A common <tt>JetpackError</tt> prototype?''' |
Revision as of 02:05, 12 January 2010
Note: The file capability mentioned below is here. Code is messy.
Where is the connection between the params passed to a capability and the "danger rating" of those params? Like with a file capability, it can define whatever params or defaults it wants ("sandboxed", "all files read-only", "all files read-write"), but how does it communicate what is dangerous and what isn't?
The only 'danger rating', currently, is the human-readable string passed back from the capability factory's describe() function. We could potentially add a rating from 'DoS' to 'Critical' as specified by the Security_Severity_Ratings page, though my only concern about that is that I'm not sure if it's possible for e.g. 5 capabilities with a 'Low' severity rating to actually present a 'Critical' severity rating when used together.
adw: Are we still planning on using some simple, intelligent UI that communicates the aggregate danger of a feature? (The stoplight, e.g.?) If so, what implications does that have here?
How does a capability get info about the feature that's using it? In my sandboxed file capability, I'd like the feature's ID to create a directory for it.
Heh, this is why a 'jetpack' parameter is currently passed-in as the "undocumented" second parameter of the capability factory's create() method. The only problem is that the 'jetpack' object/class hasn't yet been documented; need to do that soon!
Atul has mentioned the need to use closures and the need to be careful about exposing props to untrusted code. But don't COWs mitigate that problem? If not, should we follow Caja and bar the use of this outside of constructors?
Good question. Basically, while COWs do mitigate the problem, I'd personally not code with this simply because it gives the client control over something that I have to constantly defend against. The basic mindset you have to have when using this in trusted code that will communicate with untrusted code is that this is effectively just another parameter passed-in from untrusted code. So code like this:
this.foo = function foo(a) { this._submitData(a, gPassword); }
is suddenly subject to exploit because this could be set to something the client code passes in, and which they can "fake" to do malicious things. While it's certainly possible to treat this suspiciously and write secure code with it, I'd personally much rather just avoid the use of this to spare my brain the extra paranoia, unless there's some significant advantage to allowing the client to specify this themselves.
adw: OK, I say we outlaw it then, outside of constructors, for all code accepted into the Jetpack platform, including capabilities, exceptions where appropriate.
How should capabilities throw exceptions? Need to do something special to show a proper stack? A common JetpackError prototype?
You just need to throw Error(reason) for now, where reason is a string explaining the rationale for the exception; this will give a nice traceback.
It might be prudent to use an exception type hierarchy similar to Python's and Ruby's, though I haven't seen many other JS frameworks/libraries do this so I don't know how useful it'd be.
Should we try to hide all XPCOM exceptions or wrap them in "Jetpack" exceptions? That's a lot of exceptions.
Well, it sort of depends on the situation, I think. One possibility is to just map the standard NS_ERROR_* constants to more readable, less loud names in Cuddlefish's traceback module.
But depending on the XPCOM exception at hand, there are some places where it makes more sense to provide additional information outside of just the error code. For instance, in Ubiquity, I created a NiceConnection wrapper around mozIStorageConnection that detected if the last operation failed and automatically returned the connection's lastErrorString, which was far more useful than the unhelpful NS_ERROR_FAILURE returned by failing methods.
Similarly, there are places where Python adds additional information to error messages to ease debugging, which inspire our own library design. In Cuddlefish's file module, for instance, we report the name of files that don't exist when throwing errors, as opposed to simply using static "file does not exist" text and forcing the developer to figure out the filename on their own (which is effectively what NS_ERROR_FILE_NOT_FOUND communicates).
I get full tracebacks for errors, but they appear to be missing the portion of the stack inside my capability code. Here's one such stack:
error: An exception occurred. Traceback (most recent call last): File "resource://cuddlefish-jetpack-cuddlefish-jetpack-cuddlefish-jetpack/main.js", line 39, in dump(f.read() + "\n"); File "resource://cuddlefish-jetpack-secure-membrane-lib/secure-membrane.js", line 414, in call return this.callOrConstruct(wrappee, wrapper, thisObj, args, false); File "resource://cuddlefish-jetpack-secure-membrane-lib/secure-membrane.js", line 393, in callOrConstruct throw SecureMembrane.wrapTrusted(e); File "resource://cuddlefish-jetpack-secure-membrane-lib/secure-membrane.js", line 84, in wrapTrusted return this.binary.wrap(thing, new this.TrustedWrapper(thing)); File "resource://cuddlefish-jetpack-secure-membrane-lib/secure-membrane.js", line 145, in TrustedWrapper safeThrow("object has no __exposedProps__!"); File "resource://cuddlefish-jetpack-secure-membrane-lib/secure-membrane.js", line 2, in safeThrow throw SecureMembrane.wrapTrusted(new Error(message)); File "", line 0, in Error Error: object has no __exposedProps__!
It looks like the wrong stack actually. The first line (dump(f.read() + "\n")) is in the toy feature I've been using to test my capability, and it calls into the capability. I'm sure the error originates inside the capability, but it's not visible here. Is that by design?
I think this may indeed be a bug, primarily from looking at the frame coming from line 393 in callOrConstruct(); if the thing being thrown from the trusted code one of JS's standard built-in exception types (e.g., Error, ReferenceError), then the exception should automatically pass through. However, if some special other type of exception is thrown (e.g. a custom exception class, and possibly an nsIException, I'm not sure) then it needs to be given __exposedProps__ in order to propagate through untrusted code. I guess for exception-based control flow we may want to allow objects to just "pass-through" with no exposed properties, so that this kind of annoyance doesn't occur.
adw: Yeah, it's an XPCOM exception. Would it be possible to catch XPCOM exceptions at the boundary, attach an __exposedProps__ prop, and send them on their way?
Any examples for unit-testing my capability?
See the examples in JEP 31 for examples of how to unit test CommonJS modules via CFX in general. For testing Jetpacks in particular, this needs to be documented--but for now, you can take a look at the test suite for the sample notifications capability.
I'd like to keep my capability factory code in one file, and the capability code itself in another. How do I "include" the latter in the former?
Use the CommonJS module standard to separate different kinds of functionality like this.