Issue 10181043: Added repair for Function constructor bug (Closed)
|
Created:
12 years, 9 months ago by MarkM Modified:
12 years, 8 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionRepairs verification errors when Function constructor fails to verify body
Patch Set 1 #Patch Set 2 : Added repair for Function constructor bug #
Total comments: 2
Patch Set 3 : Added repair for Function constructor bug #
Total comments: 2
Patch Set 4 : Added repair for Function constructor bug #Patch Set 5 : Added repair for Function constructor bug #
Total comments: 4
Patch Set 6 : Added repair for Function constructor bug #
Total comments: 1
Patch Set 7 : Added repair for Function constructor bug #
Total comments: 12
Patch Set 8 : Added repair for Function constructor bug #Patch Set 9 : Added repair for Function constructor bug #Patch Set 10 : Added repair for Function constructor bug #
MessagesTotal messages: 22
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
This is ready for review. On Mon, Jun 10, 2013 at 8:57 PM, <erights@gmail.com> wrote: > Repairs verification errors when Function constructor fails to verify > body > > https://codereview.appspot.**com/10181043/<https://codereview.appspot.com/101... > -- Text by me above is hereby placed in the public domain Cheers, --MarkM
Sign in to reply to this message.
LGTM https://codereview.appspot.com/10181043/diff/3001/src/com/google/caja/ses/sta... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/10181043/diff/3001/src/com/google/caja/ses/sta... src/com/google/caja/ses/startSES.js:301: if (opt_mitigateOpts.parseProgram === false) { Isn't the point of this CL that we can now stop unconditionally parsing here?
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/3001/src/com/google/caja/ses/sta... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/10181043/diff/3001/src/com/google/caja/ses/sta... src/com/google/caja/ses/startSES.js:301: if (opt_mitigateOpts.parseProgram === false) { On 2013/06/11 16:57:21, kpreid2 wrote: > Isn't the point of this CL that we can now stop unconditionally parsing here? Done. Although you already LGTMed this, please reapprove as removing parseProgram correctly is clearly security critical. Thanks. https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/mit... File src/com/google/caja/ses/mitigateGotchas.js (right): https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/mit... src/com/google/caja/ses/mitigateGotchas.js:206: options.rewriteFunctionCalls || Added this even though it is not yet implemented, in order to avoid a future mistake.
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/sta... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/sta... src/com/google/caja/ses/startSES.js:923: ses.verifyStrictProgram(body); Hmm. Already found one bug. This fix to https://code.google.com/p/google-caja/issues/detail?id=1616#c13 comment 13, if done using Ankur's trick, will reject function bodies with return statements, since these are not valid programs. Not sure what the best way to fix this is, without reverting to the problem explained in comment 13.
Sign in to reply to this message.
On 2013/06/12 16:03:37, MarkM wrote: > https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/sta... > File src/com/google/caja/ses/startSES.js (right): > > https://codereview.appspot.com/10181043/diff/8001/src/com/google/caja/ses/sta... > src/com/google/caja/ses/startSES.js:923: ses.verifyStrictProgram(body); > Hmm. Already found one bug. This fix to > https://code.google.com/p/google-caja/issues/detail?id=1616#c13 comment 13, if > done using Ankur's trick, will reject function bodies with return statements, > since these are not valid programs. > > Not sure what the best way to fix this is, without reverting to the problem > explained in comment 13. This CL is on hold pending an understanding of the question raised at https://code.google.com/p/v8/issues/detail?id=2728
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/ex... File src/com/google/caja/ses/exportsToSES.js (left): https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/ex... src/com/google/caja/ses/exportsToSES.js:36: delete global.exports; Because createExports.js is creating the global "exports" with a top level "var", it is non-configurable and so the delete above was always failing. Only when I habitually made the enclosing function strict did I notice this failure. https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3382: } else if (typeof ses.mitigateSrcGotchas === 'function') { Unfortunately, at this point in the initialization process ses.mitigateSrcGotchas is not yet initialized. What is the right way to fix Caja so this is already initialized by the time we get here?
Sign in to reply to this message.
Note that this cannot be submitted until I know what to do about the ses.mitigateSrcGotchas issue I raise below. On Sat, Jul 6, 2013 at 1:34 PM, <erights@gmail.com> wrote: > > https://codereview.appspot.**com/10181043/diff/27001/src/** > com/google/caja/ses/**exportsToSES.js<https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/exportsToSES.js> > File src/com/google/caja/ses/**exportsToSES.js (left): > > https://codereview.appspot.**com/10181043/diff/27001/src/** > com/google/caja/ses/**exportsToSES.js#oldcode36<https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/exportsToSES.js#oldcode36> > src/com/google/caja/ses/**exportsToSES.js:36: delete global.exports; > Because createExports.js is creating the global "exports" with a top > level "var", it is non-configurable and so the delete above was always > failing. Only when I habitually made the enclosing function strict did I > notice this failure. > > https://codereview.appspot.**com/10181043/diff/27001/src/** > com/google/caja/ses/repairES5.**js<https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/repairES5.js> > File src/com/google/caja/ses/**repairES5.js (right): > > https://codereview.appspot.**com/10181043/diff/27001/src/** > com/google/caja/ses/repairES5.**js#newcode3382<https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/repairES5.js#newcode3382> > src/com/google/caja/ses/**repairES5.js:3382: } else if (typeof > ses.mitigateSrcGotchas === 'function') { > Unfortunately, at this point in the initialization process > ses.mitigateSrcGotchas is not yet initialized. What is the right way to > fix Caja so this is already initialized by the time we get here? > > https://codereview.appspot.**com/10181043/<https://codereview.appspot.com/101... > > -- > > ---You received this message because you are subscribed to the Google > Groups "Google Caja Discuss" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to google-caja-discuss+**unsubscribe@googlegroups.com<google-caja-discuss%2Bunsu... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > > -- Cheers, --MarkM
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/32001/src/com/google/caja/plugin... File src/com/google/caja/plugin/caja.js (right): https://codereview.appspot.com/10181043/diff/32001/src/com/google/caja/plugin... src/com/google/caja/plugin/caja.js:456: 'SYNTAX_ERRORS_ARENT_ALWAYS_EARLY': { 'permit': true } All the other differences between patch 5 and 6 are just merging with the current Caja. But this line is a policy decision we need to think about.
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3382: } else if (typeof ses.mitigateSrcGotchas === 'function') { On 2013/07/06 20:34:11, MarkM wrote: > Unfortunately, at this point in the initialization process > ses.mitigateSrcGotchas is not yet initialized. What is the right way to > fix Caja so this is already initialized by the time we get here? The hookup of mitigateSrcGotchas is in caja.js, function trySES, and in ses-frame-group.js. If you need to supply the mitigator before initSES runs then the thing to do would be to move it into the frameInit function inside trySES, which is already concerned with configuring SES. This will require moving the sesMaker inside of the utility-frame loading, which will cause the two frame loads to be serial rather than parallel network requests, which is unfortunate. Therefore I recommend you avoid needing this dependency; if it is unavoidable, then perhaps you can add a configuration flag that mitigateSrcGotchas _will be available_, and supply the actual mitigator in the current way (with inconsistency a fatal error)? (That last suggestion is basically "supply a promise for the mitigator", except that we don't have promises at this stage.) A third option would be to make initSES take no action (including no repairs) until a function is invoked with the mitigator as a parameter; basically removing hookupSES and making repairES5 invoked by startSES(). But that's a radical change.
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10181043/diff/27001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3382: } else if (typeof ses.mitigateSrcGotchas === 'function') { On 2013/07/06 22:04:57, kpreid2 wrote: > On 2013/07/06 20:34:11, MarkM wrote: > > Unfortunately, at this point in the initialization process > > ses.mitigateSrcGotchas is not yet initialized. What is the right way to > > fix Caja so this is already initialized by the time we get here? > > The hookup of mitigateSrcGotchas is in caja.js, function trySES, and in > ses-frame-group.js. > > If you need to supply the mitigator before initSES runs then the thing to do > would be to move it into the frameInit function inside trySES, which is already > concerned with configuring SES. This will require moving the sesMaker inside of > the utility-frame loading, which will cause the two frame loads to be serial > rather than parallel network requests, which is unfortunate. > > Therefore I recommend you avoid needing this dependency; if it is unavoidable, > then perhaps you can add a configuration flag that mitigateSrcGotchas _will be > available_, and supply the actual mitigator in the current way (with > inconsistency a fatal error)? > > (That last suggestion is basically "supply a promise for the mitigator", except > that we don't have promises at this stage.) > > A third option would be to make initSES take no action (including no repairs) > until a function is invoked with the mitigator as a parameter; basically > removing hookupSES and making repairES5 invoked by startSES(). But that's a > radical change. Done. Essentially your suggestion to install a flag that it will be available, but using instead a placeholder function.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/ex... File src/com/google/caja/ses/exportsToSES.js (right): https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/ex... src/com/google/caja/ses/exportsToSES.js:30: "use strict"; single quotes https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:730: // This case is likely sympomatic of an attack. But the "symptomatic" https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:756: newSrc = ses.mitigateSrcGotchas(funcBodySrc, Somewhere it should be documented that caja.js swaps out this function and therefore uses of it must not close over the value at init time. (And if that weren't the case, I would tell you to close over the value so we're not doing an extra property lookup at runtime, and so that the non-frozen 'ses' object is irrelevant after initialization.) https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:764: safeError = new Error(error.message); Would it be worthwhile to copy SyntaxErrors as SyntaxErrors here? https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:771: if (newSrc !== funcBodySrc) { Please add a comment explaining why this comparison is meaningful. It's not obvious: If ses.mitigateSrcGotchas (with these parameters) is expected to return the input source, under what circumstances would it _not_ do so, and what does it return instead? If it is expected to transform the input source, under what circumstances would it return a result equal to the input? https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:772: throw new SyntaxError("Failed to parse program"); single quotes
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/ex... File src/com/google/caja/ses/exportsToSES.js (right): https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/ex... src/com/google/caja/ses/exportsToSES.js:30: "use strict"; On 2013/07/09 16:08:48, kpreid2 wrote: > single quotes Done. But sometime we should argue about this. "use strict"; doesn't mean what a string literal normally means. Rather, it is a pragma that happens to have the syntax of a string literal in order not to break on ES3 browsers. Given that we always use single quotes for normal string literals, I think we should consistently use double quotes for these pragmas. (Also for "use asm".) https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:730: // This case is likely sympomatic of an attack. But the On 2013/07/09 16:08:48, kpreid2 wrote: > "symptomatic" Done. https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:756: newSrc = ses.mitigateSrcGotchas(funcBodySrc, On 2013/07/09 16:08:48, kpreid2 wrote: > Somewhere it should be documented that caja.js swaps out this function and > therefore uses of it must not close over the value at init time. > > (And if that weren't the case, I would tell you to close over the value so we're > not doing an extra property lookup at runtime, and so that the non-frozen 'ses' > object is irrelevant after initialization.) Done. https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:764: safeError = new Error(error.message); On 2013/07/09 16:08:48, kpreid2 wrote: > Would it be worthwhile to copy SyntaxErrors as SyntaxErrors here? Because of how ses.mitigateSrcGotchas works, unfortunately not. If the actual parsing throws at all, ses.mitigateSrcGotchas simply logs the text of the error and returns a string that, if evaluated, will throw a SyntaxError with a non-specific diagnostic. The better plan would be to improve the API of ses.mitigateSrcGotchas. But not in this CL. https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:771: if (newSrc !== funcBodySrc) { On 2013/07/09 16:08:48, kpreid2 wrote: > Please add a comment explaining why this comparison is meaningful. It's not > obvious: > > If ses.mitigateSrcGotchas (with these parameters) is expected to return the > input source, under what circumstances would it _not_ do so, and what does it > return instead? If it is expected to transform the input source, under what > circumstances would it return a result equal to the input? Done. https://codereview.appspot.com/10181043/diff/41001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:772: throw new SyntaxError("Failed to parse program"); On 2013/07/09 16:08:48, kpreid2 wrote: > single quotes Done. And several others in the file. I also noticed and removed some redundant "use strict" directives. They were redundant because they were in functions already lexically nested within strict code.
Sign in to reply to this message.
Repairs verification errors when Function constructor fails to verify body
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
