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

Remove legacy plugin hook that presents potential 3rd party security issues #3177

Closed
jsegitz opened this issue Jan 10, 2020 · 4 comments
Closed
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@jsegitz
Copy link

jsegitz commented Jan 10, 2020

We (SUSE) are currently reviewing all cron jobs we package for security issues. cacti packages a cron job and that lead us to have a look. During this I noticed that
in poller.php
575 $extra_args = api_plugin_hook_function('poller_command_args', $extra_args);
allows plugins to add arguments to the commands being executed. These arguments are not escaped, so it's left to the plugins to do this. I checked the default plugins and none of them are problematic.

IMHO (and I'm not really familiar with the code base) it might make sense to ensure proper escaping here to ensure defense in depth. Currently the code relies on every plugin getting this right.

@bmfmancini
Copy link
Member

hey hey!

Nice to see that OS devs are contributing to make Cacti even better
I am sure @cigamit or @netniV can chime in here

@cigamit
Copy link
Member

cigamit commented Jan 11, 2020

Well, I'm going to see if this plugin is actually used anywhere. Cause I agree. Right now, it's the plugins responsibility to perform any escaping of the arguments, as they can be pretty free form.

I would suggest that maybe in 2020, we don't even need this hook. But we have to review it first.

@cigamit
Copy link
Member

cigamit commented Jan 11, 2020

Just finished the audit. I'd be happy to remove this as a bug. The two legacy plugins that used it really were misusing it to modify memory limits for the data collectors and not really messing with command line arguments. In other words, they were hacks by the uber-hacker himself, he who shall remain nameless.

@cigamit cigamit added the bug Undesired behaviour label Jan 11, 2020
@cigamit cigamit changed the title Escape arguments added by plugins Remove legacy plugin hook that presents potential 3rd party security issues Jan 11, 2020
cigamit added a commit that referenced this issue Jan 11, 2020
Remove legacy plugin hook that presents potential 3rd party security issues
@cigamit cigamit added the resolved A fixed issue label Jan 11, 2020
@cigamit cigamit added this to the v1.2.9 milestone Jan 11, 2020
@cigamit cigamit closed this as completed Jan 12, 2020
@jsegitz
Copy link
Author

jsegitz commented Jan 14, 2020

Thank you for the quick reaction. We'll remove it from our packages too

@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

3 participants