Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(700)

Issue 4249052: First checkin of SES to Caja source tree. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by MarkM
Modified:
12 years, 10 months ago
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Turns an ES5 environment (e.g., browser frame) into a SES environment following object-capability rules.

Patch Set 1 #

Total comments: 6

Patch Set 2 : First checkin of SES to Caja source tree. #

Total comments: 75

Patch Set 3 : First checkin of SES to Caja source tree. #

Patch Set 4 : First checkin of SES to Caja source tree. #

Patch Set 5 : First checkin of SES to Caja source tree. #

Patch Set 6 : First checkin of SES to Caja source tree. #

Patch Set 7 : First checkin of SES to Caja source tree. #

Patch Set 8 : First checkin of SES to Caja source tree. #

Patch Set 9 : First checkin of SES to Caja source tree. #

Patch Set 10 : First checkin of SES to Caja source tree. #

Patch Set 11 : First checkin of SES to Caja source tree. #

Total comments: 66

Patch Set 12 : First checkin of SES to Caja source tree. #

Total comments: 2

Patch Set 13 : First checkin of SES to Caja source tree. #

Patch Set 14 : First checkin of SES to Caja source tree. #

Patch Set 15 : First checkin of SES to Caja source tree. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2114 lines, -1 line) Patch
M build.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +22 lines, -0 lines 0 comments Download
M src/com/google/caja/parser/js/Block.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -1 line 0 comments Download
A src/com/google/caja/ses/WeakMap.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +401 lines, -0 lines 0 comments Download
A src/com/google/caja/ses/atLeastFreeVarNames.js View 1 2 3 4 5 6 7 1 chunk +98 lines, -0 lines 0 comments Download
A src/com/google/caja/ses/es5shim.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +347 lines, -0 lines 0 comments Download
A src/com/google/caja/ses/hookupSES.js View 1 chunk +24 lines, -0 lines 0 comments Download
A src/com/google/caja/ses/startSES.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +816 lines, -0 lines 0 comments Download
A src/com/google/caja/ses/whitelist.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +404 lines, -0 lines 0 comments Download

Messages

Total messages: 20
MarkM
13 years ago (2011-03-03 09:25:42 UTC) #1
mzero
Some thoughts on the Unicode source issue http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastFreeVarNames.js File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastFreeVarNames.js#newcode62 src/com/google/caja/ses/atLeastFreeVarNames.js:62: var SHOULD_MATCH_IDENTIFIER ...
13 years ago (2011-03-04 22:03:02 UTC) #2
mzero
Two more findings. Also: UNIT TESTS?!?!?! This code needs unit tests! http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastFreeVarNames.js File src/com/google/caja/ses/atLeastFreeVarNames.js (right): ...
13 years ago (2011-03-04 23:12:19 UTC) #3
mzero
I think the TOLERATE_MISSING_DESCRIPTOR logic is wrong. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/startSES.js#newcode207 src/com/google/caja/ses/startSES.js:207: if (!desc ...
13 years ago (2011-03-07 23:26:40 UTC) #4
ihab.awad
In general, this is really cool! Some notes: * As noted throughout, there needs to ...
13 years ago (2011-03-16 21:17:11 UTC) #5
MikeSamuel
http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/startSES.js#newcode67 src/com/google/caja/ses/startSES.js:67: // "use strict"; // not here because of an ...
12 years, 11 months ago (2011-04-26 01:25:37 UTC) #6
ihab.awad
Even better! Thanks Mike! :) -- Ihab On Mon, Apr 25, 2011 at 6:25 PM, ...
12 years, 11 months ago (2011-04-26 03:29:02 UTC) #7
MarkM
Not yet ready for next review. Soon. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES.js#newcode205 src/com/google/caja/ses/startSES.js:205: for (var ...
12 years, 11 months ago (2011-04-27 01:39:44 UTC) #8
MarkM
still not ready. closer. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakMap.js File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakMap.js#newcode25 src/com/google/caja/ses/WeakMap.js:25: * retain maps indexed by ...
12 years, 11 months ago (2011-04-27 03:07:01 UTC) #9
MarkM
Still not ready for review. Separated WeakMap.js into es5shim.js and WeakMap.js according to Ihab's suggestion ...
12 years, 11 months ago (2011-05-02 04:33:30 UTC) #10
MarkM
Still not ready. Soon. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastFreeVarNames.js File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastFreeVarNames.js#newcode46 src/com/google/caja/ses/atLeastFreeVarNames.js:46: if (!((/^[\u0000-\u007f]*$/m).test(programSrc))) { On 2011/03/04 ...
12 years, 11 months ago (2011-05-03 00:12:05 UTC) #11
MarkM
Finally ready for the next round of review. Have at it. Thx. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js ...
12 years, 11 months ago (2011-05-04 05:59:23 UTC) #12
MarkM
Fixed two manifestations of the same security vulnerability, as now documented on PATCH_MUTABLE_FROZEN_DATE_PROTO and PATCH_MUTABLE_FROZEN_WEAKMAP_PROTO ...
12 years, 11 months ago (2011-05-05 01:21:42 UTC) #13
MarkM
Improved diagnostic when the new checks that prevent modifications of internal prototype checks fail. Relayered ...
12 years, 11 months ago (2011-05-05 20:33:42 UTC) #14
mzero
LGTM
12 years, 11 months ago (2011-05-06 21:03:56 UTC) #15
Jasvir
http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/startSES.js#newcode294 src/com/google/caja/ses/startSES.js:294: * <p>TODO(erights): Switch to Crock's alternate suggestion of I ...
12 years, 10 months ago (2011-05-10 23:26:38 UTC) #16
MarkM
Also found a better kludge to work around some undiagnosed bugs. See METHODS_TO_WRAP_ON_V8 in es5shim.js, ...
12 years, 10 months ago (2011-05-13 18:24:42 UTC) #17
Mark S. Miller
[+ankur, +douglas] New code implementing Crock's trick at http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/startSES.js#newcode273 Thanks to both of you! On ...
12 years, 10 months ago (2011-05-13 18:31:52 UTC) #18
ihab.awad
LGTM++ http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/WeakMap.js File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/WeakMap.js#newcode44 src/com/google/caja/ses/WeakMap.js:44: * FF6.0a1 is the presence on non enumerable ...
12 years, 10 months ago (2011-05-13 21:49:07 UTC) #19
MarkM
12 years, 10 months ago (2011-05-14 00:29:09 UTC) #20
http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
File src/com/google/caja/ses/WeakMap.js (right):

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:44: * FF6.0a1 is the presence on non
enumerable {@code get___, has___,
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "of non" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:107: * MUST either have different identities,
or at least one of their
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "or both of their" ... -- right?

No. If only one of their identities is NO_IDENT, then they are still not egal.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:162: *     randomness provided by {@code
Math.random}. This not
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "This is not" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:218: identifyFirst(Object, 'freeze');
On 2011/05/13 21:49:08, ihab.awad wrote:
> This should not induce a change in the CL, but it just occurred to me that a
> common application pattern may be to create a whole bunch of small "value"
> objects and freeze them -- like (x, y) coordinates or whatever. We are making
> that nontrivially more expensive here.

Yes. Without built-in WeakMaps, I don't see that we have any other choice.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:323: var last = ids.length -1;
On 2011/05/13 21:49:08, ihab.awad wrote:
> Space after minus operator.

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak...
src/com/google/caja/ses/WeakMap.js:332: delete vals[last];
On 2011/05/13 21:49:08, ihab.awad wrote:
> delete does not shorten the array length; did you mean to use
ids.splice(last)?

Done. Wow. Somehow it had escaped my notice that delete on the end does not
update length. I thought it did.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe...
File src/com/google/caja/ses/atLeastFreeVarNames.js (right):

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe...
src/com/google/caja/ses/atLeastFreeVarNames.js:86: while ((a =
regexp.exec(programSrc))) {
On 2011/05/13 21:49:08, ihab.awad wrote:
> Extra parens around expr.

That's on purpose, as a widespread convention saying "yes, even though it's a
conditional expression, I really did mean assignment rather than an equality
test." I think this convention started in C but I've seen it at least in Java as
well. I think I've also seen linting tools that recognize it -- though not yet
for JavaScript AFAIK.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
File src/com/google/caja/ses/es5shim.js (right):

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:15: var RegExp;
On 2011/05/13 21:49:08, ihab.awad wrote:
> Why declare this here? (Was not present in older revs.)

Just to be clearer that this is a global that might be assigned to by this
module. The older code was doing "global.RegExp" and so needed to bind "global"
in this scope to the global object, which it no longer does. Of course, neither
is actually necessary, but a simple local assignment to an identifier doesn't
stand out sufficiently as a potentially global assignment.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:142: var classTester =
Object.prototype.toString;
On 2011/05/13 21:49:08, ihab.awad wrote:
> I found the renaming confusing.

Done. Renamed to objToString. Clearer about what it is, but slightly less clear
about what it is for,

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:144: var getParent = Object.getPrototypeOf;
On 2011/05/13 21:49:08, ihab.awad wrote:
> I found the renaming confusing.

Done. Renamed to getPrototypeOf

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:168: switch (arguments.length) {
On 2011/05/13 21:49:08, ihab.awad wrote:
> Please indent case-es 1 step.

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:237: * Make a function suitable for using as
a forEach argument on a
On 2011/05/13 21:49:08, ihab.awad wrote:
> Describing in terms of foreach is a bit confusing ... maybe just describe in
> terms of what it returns? Not a big deal either way.

Done. "Make a function..." -> "Return a function..."

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:246: * objects of this [[Class]], only the
prototypes directly inherits
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "only the prototype" ...

I meant the plural to emphasize the cross-frame issue. However, "inherits"
should therefore be "inherit" which is now fixed.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:267: classString + '.prototype');
On 2011/05/13 21:49:08, ihab.awad wrote:
> Indentation.

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:280: // Note: coordinate this list with
maintanence of whitelist.js
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "maintenance" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
File src/com/google/caja/ses/startSES.js (right):

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:90: * <li>Since {@code Q.post} can be used
for asynchronously involing
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "invoking" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:118: * evaluation operations) takes care to
uphold object-capability
On 2011/05/13 21:49:08, ihab.awad wrote:
> My code says:
> 
> alert('I pledge allegiance to the Grannovetter diagram,\nAnd to the
Introduction
> for which it stands.')
> 
> Does that mean it upholds object-capability rules? If not, what must my code
do?

I'm not sure what to say, so I added a TODO reminder to explain this.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:138: *        <tt>global</tt>. These
properties are also duplicated onto a
On 2011/05/13 21:49:08, ihab.awad wrote:
> s/ a$//

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:152: *        potentially offensive code but
not supporting defensive
On 2011/05/13 21:49:08, ihab.awad wrote:
> Perhaps clarify that this implies one trust domain per frame? Not a big deal
> either way.

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:273: /** Repair phase -- monkey patch safe
replacements */
On 2011/05/13 21:49:08, ihab.awad wrote:
> This comment misled me. Actually, what's starting in the below anonymous
> function is the construction of the guts of "global.cajaVM"....

Done. Confusing comment removed.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:387: (function(name) {
On 2011/05/13 21:49:08, ihab.awad wrote:
> 2nd level of lambda unnecessary, right?

Done. Good catch.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:467: var requirePattern =
(/^(?:\w*\s*(?:\w|\$|\.)*\s*=\s*)?\s*require\s*\(\s*['"]((?:\w|\$|\.|\/)+)['"]\s*\)$/m);
On 2011/05/13 21:49:08, ihab.awad wrote:
> Is there an extra "\s*"? Harmless anyway but wouldn't this also work?
> 
> ...\s*=)?\s*require...

I don't know. Since it's harmless, I'm skipping this issue for now but adding a
TODO.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:647: })();
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... and here is where we're done building "global.cajaVM".

Yup. Nothing done here since offending comment removed above.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:700: writable: true,
On 2011/05/13 21:49:08, ihab.awad wrote:
> Why writable?

A baby step towards preparing for confined-ES5.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:710: writable: true,
On 2011/05/13 21:49:08, ihab.awad wrote:
> Why writable?

A baby step towards preparing for confined-ES5.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:737: fail('primordial reachable through
multiple paths');
On 2011/05/13 21:49:08, ihab.awad wrote:
> Seems benign; why is that a problem? Just because it would make your attempt
to
> limit access to it via the whitelist more fragile?

If this happens, it means that the heap doesn't have the structure I assumed it
had when I constructed the whitelist, so fail early and safe rather than proceed
under possibly invalid assumptions.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:750: * Should the property names {@code
name} be whitelisted on the
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "named" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:766: if (permit && hop.call(permit, name)) {
On 2011/05/13 21:49:08, ihab.awad wrote:
> Why not just use permit[name] as before? Why the "hop" call?

Because the permits inherit from object.prototype, so, for example,
permit['constructor'] will be truthy even for a permit that does not say
anything about a 'constructor' property. I actually ran into precisely that bug
on that earlier version.

This is a great example of JS hazards that even experienced JS programmers will
miss, as both of us have now done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:820: for (var i = 0, len =
EARLY_FREEZES.length; i < len; i++) {
On 2011/05/13 21:49:08, ihab.awad wrote:
> Variables "i" and "len" leak to a large scope; maybe put in closure?

Done. No longer a problem since the kludge migration from startSES.js to
es5shim.js removed the need for this loop.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star...
src/com/google/caja/ses/startSES.js:855: }
On 2011/05/13 21:49:08, ihab.awad wrote:
> Else throw, as an extra safety feature?

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit...
File src/com/google/caja/ses/whitelist.js (right):

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit...
src/com/google/caja/ses/whitelist.js:30: *     "Object"} may have and how each
of such property, if present,
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "each such property" ...

Done.

http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit...
src/com/google/caja/ses/whitelist.js:313: // Note: coordinate this list with
maintanence of es5shim.js
On 2011/05/13 21:49:08, ihab.awad wrote:
> ... "maintenance" ...

Done.

http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s...
File src/com/google/caja/ses/es5shim.js (right):

http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s...
src/com/google/caja/ses/es5shim.js:165: * not frozen.
On 2011/05/13 21:49:08, ihab.awad wrote:
> Why would Confined-ES5 change things?

It might allow the untrusted code to monkey patch its own
Function.prototype.apply, in which case "return original.apply(this,
arguments);" is no longer a transparent emulation of the internal
"original.[[Call]](this, arguments)". Also, the munkey-patched apply obtains
access to original, which should be denied to untrusted code.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b