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

MetaClass delegate handling #49

Merged
merged 4 commits into from
Apr 22, 2016
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Mar 30, 2016

Found that a trick used to bind synthetic methods to existing classes was being rejected by the sandbox. Here it is assumed that the closure itself is implicitly whitelisted; Whitelist does not have any mechanism for listing non-Java “methods” like this.

@reviewbybees esp. @kohsuke

jglick added 2 commits March 30, 2016 18:06
Does no ACL check, but generally safe, since most AuthorizationStrategy’s only let you see a child if you can see its parent first.
@@ -2,6 +2,7 @@
new hudson.EnvVars
method hudson.model.AbstractBuild getEnvironments
staticMethod hudson.model.Environment create hudson.EnvVars
method hudson.model.Item getParent
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related but came up as a desired whitelist entry in the same context.

@ghost
Copy link

ghost commented Mar 30, 2016

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.

@kohsuke
Copy link
Member

kohsuke commented Mar 30, 2016

IIUC, this allows any closure invocation, such as those obtained via method pointer operation. Is this safe? Looking at the test case, it seems like this case should be checked against String.getAnswer() method.

🐝 so as not to block Jesse in case I got it all wrong.

@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick
Copy link
Member Author

jglick commented Mar 31, 2016

I could check whether it applies also to method pointers. I am not too concerned even if it does, since DefaultGroovyMethod.getMetaClass and MetaClass.setProperty will still be rejected, so you could not use that to mount an attack—a trusted source would have to have installed the metamethod to begin with. Or are you saying something else @kohsuke?

@kohsuke
Copy link
Member

kohsuke commented Apr 4, 2016

I think I confused myself with earlier question about allowing any closure invocation, so I take that back.

@andresrc
Copy link

andresrc commented Apr 6, 2016

🐝

@kohsuke
Copy link
Member

kohsuke commented Apr 12, 2016

I consider my previous 🐝 standing, but here's another one because process!!

🐝

@jglick jglick merged commit 5f74dc3 into jenkinsci:master Apr 22, 2016
@jglick jglick deleted the metaClassDelegate branch April 22, 2016 15:52
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

Successfully merging this pull request may close these issues.

4 participants