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

add ParserConfiguration#setParsers to blacklist #533

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

meiswjn
Copy link

@meiswjn meiswjn commented Oct 10, 2023

Add ParserConfiguration#setParsers to blacklist. This function allows to run scripts in the agent-context without further approval by adding them as parsers in the global configuration
-> Privilege Escalation

Testing done

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options
  4. Link to relevant issues in GitHub or Jira
    Options
  5. Link to relevant pull requests, esp. upstream and downstream changes
    Options
  6. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

@meiswjn meiswjn requested a review from a team as a code owner October 10, 2023 08:23
@meiswjn
Copy link
Author

meiswjn commented Oct 10, 2023

@uhafner I thought that this might make sense

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Fine I guess, though oddly specific. What would have made you think this method would be safe to approve to begin with?

There are many thousands of signatures possible in the Java Platform and Jenkins core and various Jenkins plugins, many of which would be security risks. Admins really should never approve anything unless they are Java developers with Jenkins expertise who have studied the method in detail and concluded that it could not be used maliciously (and submitted a PR to this repo to add it to the default whitelist).

The blacklist is intended to capture a handful of signatures which are both especially dangerous, and seem like plausible candidates for approval that an admin might mindlessly approve otherwise. For example GroovyObject.setProperty might appear as a rejected signature if you try to use reflection-heavy idioms in a sandbox, and if you know nothing about Groovy internals this might sound unremarkable, but in fact it would open up a giant security hole to approve it.

Is this ParserConfiguration.setParsers something that users would commonly attempt to include in a Jenkinsfile or other sandboxed script?

@meiswjn
Copy link
Author

meiswjn commented Oct 11, 2023

@jglick Thanks for your reply!

I think it should be part of this list because the plugin is used pretty often and using this function is documented here:
https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md#creating-a-groovy-parser-programmatically

This may lead people to use this function.

@jglick
Copy link
Member

jglick commented Oct 11, 2023

can also be created using a Groovy script from within a pipeline

Ah, that is not good advice!

https://github.com/jenkinsci/script-security-plugin/pull/533/checks?check_run_id=17600899807 is because plugin-supplied signatures cannot be validated in a unit test here. They must be manually verified, then explicitly suppressed from the test as known to be impossible to validate.

@dwnusbaum
Copy link
Member

dwnusbaum commented Oct 11, 2023

This doesn't make sense to me. In jenkinsci/warnings-ng-plugin#1599 you added addParser and deleteParser methods which in combination have effectively the same behavior as setParsers, so why would we only blacklist the old method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants