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

[FIXED JENKINS-46088] Stop double-transforming casts in declarations #37

Merged
merged 2 commits into from Aug 16, 2017

Conversation

@abayer
Copy link
Member

abayer commented Aug 9, 2017

JENKINS-46088

We were, well, double-transforming here. Transforming the
DeclarationExpression after adding the transformed CastExpression to
it resulted in a checkedStaticCall to Checker.checkedCast! That...was
not desirable. I believe this caused some other issues where you could
get mysterious Collections.toArray, Checker.checkedStaticCall, and
Checker.checkedCall related RejectedAccessExceptions, based on
inspecting the resulting AST from the double-transform approach.

So let's just transform the whole DeclarationExpression once. Gets us
the goal we want without the double transformation.

cc @reviewbybees

We were, well, double-transforming here. Transforming the
DeclarationExpression after adding the transformed CastExpression to
it resulted in a checkedStaticCall to Checker.checkedCast! That...was
not desirable. I believe this caused some other issues where you could
get mysterious Collections.toArray, Checker.checkedStaticCall, and
Checker.checkedCall related RejectedAccessExceptions, based on
inspecting the resulting AST from the double-transform approach.

So let's just transform the whole DeclarationExpression once. Gets us
the goal we want without the double transformation.
@abayer

This comment has been minimized.

Copy link
Member Author

abayer commented Aug 9, 2017

Downstream PR up at jenkinsci/script-security-plugin#139 - note that there is no groovy-cps or workflow-cps downstream PR, because this code path is literally never touched by CPS code, since CPS code doesn't use SandboxTransformer from here.

Copy link
Member

svanoort left a comment

🐝 with limited understanding

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Aug 9, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link

alexanderrtaylor left a comment

🐝 as well if I am understanding correctly

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Aug 10, 2017

@abayer Are we sure this retains the security fix?

@abayer

This comment has been minimized.

Copy link
Member Author

abayer commented Aug 10, 2017

Yes. checkedCast is still getting called. Once I'm a bit more awake, I can paste copies of what the transformed AST looked like before and after.

Copy link
Member

jglick left a comment

I have low confidence in this fix. Clearly the original fix was wrong somehow and this corrects a regression; what the impact on other cases is, I am less sure.

BTW @kohsuke comments on the hackish SECURITY-566 fix in SandboxCpsTransformer.visitCastExpression (in groovy-cps, but touching related code):

Builder.sandboxCast makes Builder aware of sandboxing. I think more aligned approach is to change Builder.cast from a macro to a full blown CastBlock, which invokes ScriptBytecodeAdapter.asType() through Invoker, which allows SandboxInvoker to perform different things.

and

ideally instead of generating a direct call into checkedCast into block tree, one would create CastBlock, add a new method to Invoker to does a coercion cast, and then SandboxInvoker implementation call into Checker.checkedCast while the default Invoker will just call ScripteBytecodeAdapter or whatever.

@@ -704,7 +704,7 @@ public void visitExpressionStatement(ExpressionStatement es) {
if (mightBePositionalArgumentConstructor((VariableExpression) leftExpression)) {
CastExpression ce = new CastExpression(leftExpression.getType(), de.getRightExpression());
ce.setCoerce(true);
es.setExpression(transform(new DeclarationExpression(leftExpression, de.getOperation(), transform(ce))));
es.setExpression(transform(new DeclarationExpression(leftExpression, de.getOperation(), ce)));

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Are you positive this is still transforming de.rightExpression? Are there some tests demonstrating that a declaration expression matching these conditions will still intercept methods calls and the like in the initializer?

This comment has been minimized.

Copy link
@abayer

abayer Aug 15, 2017

Author Member

I am positive, from debug testing and inspecting the resulting expressions, but yeah, what we really need, IMO, both here and in groovy-cps, is AST transformation testing, not just testing of the resulting code.

This comment has been minimized.

Copy link
@jglick

jglick Aug 16, 2017

Member

Well that is probably too low level. You care about the results, not the mechanism. I am just asking whether there is some automated test proving that the RHS is checked in the sandbox after this fix.

This comment has been minimized.

Copy link
@abayer

abayer Aug 16, 2017

Author Member

I'm assuming there already was such a test when this was added in the first place, but I'll check.

This comment has been minimized.

Copy link
@abayer

abayer Aug 16, 2017

Author Member

@jglick - so there are existing tests in script-security and workflow-cps for SECURITY-580 - do we need more than that?

This comment has been minimized.

Copy link
@abayer

abayer Aug 16, 2017

Author Member

What I think are appropriate tests have been added to jenkinsci/script-security-plugin#139 and jenkinsci/workflow-cps-plugin#164

This comment has been minimized.

Copy link
@abayer

abayer Aug 16, 2017

Author Member

and fwiw, when I removed the transformation of the entire DeclarationExpression, the new tests failed due to jenkins.model.Jenkins.getInstance() not being rejected, so...looks like we're good.

@@ -316,7 +316,7 @@ x.plusOne(5)
* See issue #9
*/
void testAnd() {
assertIntercept('Checker:checkedCast(Class,null,Boolean,Boolean,Boolean)', false, """
assertIntercept('', false, """

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

This suggests to me that this entry may be obsolete.

This comment has been minimized.

Copy link
@abayer

abayer Aug 15, 2017

Author Member

Probably right.

This comment has been minimized.

Copy link
@jglick
@jglick jglick requested review from kohsuke and removed request for kohsuke Aug 15, 2017
@abayer

This comment has been minimized.

Copy link
Member Author

abayer commented Aug 15, 2017

@jglick I am confident that this fixes the problem in question without ramifications, but I do agree that a more thorough change may be best.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 16, 2017

If the test coverage were strengthened to ensure that your change does not drop sandbox transformation where it is needed, I could approve this.

@jglick
jglick approved these changes Aug 16, 2017
Copy link
Member

jglick left a comment

Sounds OK then.

@abayer abayer merged commit ef51992 into jenkinsci:master Aug 16, 2017
@abayer

This comment has been minimized.

Copy link
Member Author

abayer commented Aug 16, 2017

Merged - could you cut a release, @jglick? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.