Closed paulgevers closed 6 years ago
Paul, I've discussed this with Tony and he has asked for some time to respond. I have proposed a solution that can be optional for people who are concerned about this vector. As you can imagine, this is at the very edge of what Tony referred to as "laser focused spear pfishing", with a very low probability of success. However, as we have to support a very large community of users, we will get this thing finally addressed.
I'm not certain you will get a response this weekend. But hey this has been around for 8 years.
@cigamit I am not in a rush at all :) And I agree you certainly want to think this over and don't implement anything to hastily. It isn't worth it (haste).
Sorry, wrong issue number should have been resolving issue #1071
Here is the proposal Paul. We will set a config option that points to a white list file in a location selected by the packager (my guess /etc/cacti). If that setting is present, then cacti will behave as documented below. Otherwise, it will behave as it does today.
First, the white list file will include name value pairs as follows:
1) The hash of each Data Input Method. 2) The the base64 encoded value of the Data Input Methods command/input string.
If at any time, during Graph/Data Source creation, poller cache re-population, etc, the following is true:
1) The base64 encoded value does not match the value in the database 2) There is no entry in the white list file matching the hash of the input method.
Data sources will not be permitted to be created, the poller item table will remain touched, etc.. This will essentially disable the Data Input Method.
Additionally, we will validate the white list file each polling cycle and generate warnings when we find that has issues.
I'm guessing you will need a CLI script to be able to generate and update the white list automatically at install time. So, we will do that as well.
Lastly, we will provide a Utility pick for the Cacti Administrator to re-generate and review the white list contents. However, for security reasons, the Cacti Administrator will have to manually cut and paste the contents into the white list file by hand.
How does that sound?
@cigamit
First, thanks a lot for coming back to this and spending time on finally fixing this.
Otherwise, it will behave as it does today.
This, I like. :)
I don't see why you need to base64 encode the command, but you'll probably have a reason, so I'm fine with it. Plain text is probably easier to audit by the system administrator, but if there are tools (as you "promise") this is no big deal.
I'm guessing you will need a CLI script to be able to generate and update the white list automatically at install time. So, we will do that as well.
If you mean to migrate an existing setup, that would be awesome and more than I expected to get. To be honest, I was preparing to back-port your changes to existing releases of Cacti in Debian, while not enabling it. So if this CLI script is blocking progress or taking too much time, feel free to leave it out.
How does that sound?
As far as I can tell, sounds good.
Paul, I have committed a fix for this. Please test and provide feedback.
@paulgevers, Please let us know if the last commit works for you we found some issues.
Not sure if you want to backport this to 1.1.x releases, your option, but we are trying to get 1.2.x moving forward and anticipate it early sometime in 2018 (maybe sooner, no promises).
@ronytomen I backported the 5 relevant patches to 1.1.28, which only had a conflict in docs/CHANGELOG. I am now running a local 1.1.28 Debian package with those changes applied.
It must be more involved. Because while I now see "White List Verification Succeeded." in the Data Input Method tab, nothing seems to happen when I add the $input_whitelist option to the configuration. Also not when I add the whitelist file, but leaving it empty. I even changed one input method to "touch /tmp/CVE-2009-4112" and I see it appearing on every cron run of Cacti. I see quite some errors in the cacti.log however, so I'll dive into that first.
Sorry, no results from me thus.
Seems to be not working. More testing needed on our side, sorry!
This is my fault. I covered the create case, but not the re-populate case. Easy fix. Be in shortly.
Paul, try again. If the whitelist is invalid, the poller items will not be updated to the new values. They will instead be removed from the database until the problem is corrected. I also fixed an issue where half of the code was using the old whitelist format, and the rest was using the new. Move things to associative arrays from objects for consistency.
@cigamit works better. Can it be that the current code doesn't accept a Data Input Method again after it got whitelisted (after first being invalidated)? The current code does successfully block, but doesn't seem to unblock after I accept it. I currently have to first add the command, update the whitelist manually or with the CLI command and then save the Data Input Method again it seems.
The Data Input Method page ¹ always says "White List Verification Succeeded." whether or not the command is accepted or rejected. That is confusing and looking at the code also not intended.
¹ http://localhost/cacti/data_input.php?action=edit&id=4
Also, cacti.log doesn't mention anything at low logging level, except a reduced amount of RRD's processed. Did I miss logging (at higher logging level) or is this something that should/could be improved at level "low" already? << never mind this remark, I now find
2017/11/24 10:41:50 - CMDPHP ERROR: Whitelist entry failed validation for Data Input: Unix - Get Load Average[ 4 ]. Data Collection will not run. Run CLI command input_whitelist.php --audit and --update to remediate.
2017/11/24 10:41:50 - CMDPHP WARNING: Data Input 4 failing validation check.```
By the way, I am seeing the following errors in my cacti.log. I don't think they are related to my backport or the changes under discussion, but I'd like your opinion.
2017/11/24 10:38:26 - CMDPHP PHP ERROR NOTICE Backtrace: (/data_input.php: 64 data_edit)(/data_input.php: 473 draw_edit_form)(/lib/html_form.php: 53 CactiErrorHandler)(/lib/functions.php: 4408 cacti_debug_backtrace)
2017/11/24 10:38:26 - ERROR PHP NOTICE: Undefined index: method in file: /usr/share/cacti/site/lib/html_form.php on line: 57
2017/11/24 10:38:26 - CMDPHP PHP ERROR NOTICE Backtrace: (/data_input.php: 64 data_edit)(/data_input.php: 473 draw_edit_form)(/lib/html_form.php: 57 CactiErrorHandler)(/lib/functions.php: 4408 cacti_debug_backtrace)
2017/11/24 10:38:26 - ERROR PHP NOTICE: Undefined index: method in file: /usr/share/cacti/site/lib/html_form.php on line: 61
2017/11/24 10:38:26 - CMDPHP PHP ERROR NOTICE Backtrace: (/data_input.php: 64 data_edit)(/data_input.php: 473 draw_edit_form)(/lib/html_form.php: 61 CactiErrorHandler)(/lib/functions.php: 4408 cacti_debug_backtrace)
2017/11/24 10:38:26 - ERROR PHP NOTICE: Undefined index: friendly_name in file: /usr/share/cacti/site/lib/html_form.php on line: 81
2017/11/24 10:38:26 - CMDPHP PHP ERROR NOTICE Backtrace: (/data_input.php: 64 data_edit)(/data_input.php: 473 draw_edit_form)(/lib/html_form.php: 81 CactiErrorHandler)(/lib/functions.php: 4408 cacti_debug_backtrace)
2017/11/24 10:38:26 - ERROR PHP NOTICE: Undefined index: method in file: /usr/share/cacti/site/lib/html_form.php on line: 129
2017/11/24 10:38:26 - CMDPHP PHP ERROR NOTICE Backtrace: (/data_input.php: 64 data_edit)(/data_input.php: 473 draw_edit_form)(/lib/html_form.php: 110 draw_edit_control)(/lib/html_form.php: 129 CactiErrorHandler)(/lib/functions.php: 4408 cacti_debug_backtrace)
Paul, update your global_form.php. Everything should work at this point.
@cigamit the errors are indeed gone. My other remarks still hold though:
@cigamit works better. Can it be that the current code doesn't accept a Data Input Method again after it got whitelisted (after first being invalidated)? The current code does successfully block, but doesn't seem to unblock after I accept it. I currently have to first add the command, update the whitelist manually or with the CLI command and then save the Data Input Method again it seems.
The Data Input Method page¹ always says "White List Verification Succeeded." whether or not the command is accepted or rejected. That is confusing and looking at the code also not intended.
Is it possible that I missed a commit? I now have 7 commits back-ported: 365730d, 59e8199, 5f052cc, af4f440, 6732e2d, cd8618f and 8263164
Paul, relative to, i tried to reduce the logging by only logging once per data input method on save/create/repopulate. Double checking the other stuff now.
Okay Paul. Thank goodness for the weekends. I definitely moved to fast on this one. Everything is working as expected now.
The page now correctly shows if it validates, true. But after CLI --update I still need to save once before the input method enables again (see the amount of RRDsProcessed). I ran CLI before the 10:55 run (by the way, the command I whitelist below is "touch /tmp/CVE-2009-4112", so that may be why the "Invalid Responses" are there).
2017/12/03 11:00:03 - SYSTEM STATS: Time:1.6631 Method:cmd.php Processes:1 Threads:N/A Hosts:1 HostsPerProcess:1 DataSources:15 RRDsProcessed:10
2017/12/03 11:00:03 - POLLER: Poller[1] WARNING: Invalid Response(s), Errors[1] Device[testavoira] Graphs[testavoira - Load Average, testavoira - Load Average] DS[3]
2017/12/03 10:55:03 - SYSTEM STATS: Time:1.4316 Method:cmd.php Processes:1 Threads:N/A Hosts:1 HostsPerProcess:1 DataSources:14 RRDsProcessed:9
2017/12/03 10:52:33 - CMDPHP WARNING: Data Input 4 failing validation check.
2017/12/03 10:52:33 - CMDPHP ERROR: Whitelist entry failed validation for Data Input: Unix - Get Load Average[ 4 ]. Data Collection will not run. Run CLI command input_whitelist.php --audit and --update to remediate.
Are you thinking we need another flag to push out changes if one had previously failed validation and then we correct the validation. I think that this should be an exception. Maybe '--push-updates' option or something. I would not want to push out an entire system as this can be hundreds of thousands of entries in many systems. Let me know what you think.
I don't fully understand what you mean here, but if you think the current behavior is the right one, than we may try to improve the documentation. Maybe the string about validation failure could mention that one has to save the input method again after fixing the issue by running the CLI. Then at least I would know what to expect. Or a third "verification successful, but not used yet" string.
I think I wouldn't mind the extra option either.
Paul, I added a cli option called --push to update the poller items when a validation fails and the --update option is being executed. This only affects existing entries. So, it should never have to be used on a new system as it should not have graphs. Another enhancement might be to add this to the installation as we do attempt to discover the localhost device and add graphs. Something else to think about I guess.
@cigamit sorry that it took so long to get back to you. I should have tested earlier, but I expected it to be all set now.
I just tested with patch 11b559b added to the patch series that I already had. I now indeed had the option to push while updating. However, although the audit now showed the command was accepted, and also http://localhost/cacti/data_input.php?action=edit&id=4 showed the command as accepted, it was still not executed. I still had to save one more time to get the command to run.
Being that we have fixed this in the 1.2 branch. I'm closing this.
@cigamit did you fix the issue I mentioned in my comment from Dec 22?
Ope, thanks for reminding me. Forgot all about it. Just doing some house cleaning. Running out of bugs. So, now we can get back on the 1.2 train.
Do I need to do anything in the new installer with regards to the whitelisting? Even if it's to display a warning about how to add new methods on a whitelist-enabled system?
Yes, there is a CLI script in the cli directory that will create the whitelist file. You also have to point to in in config.php.
Do we want to give people that option in the installer?
Marking this resolved. Testing in my local environment.
Reminder: @cigamit did you fix the issue I mentioned in my comment from Dec 22?
Due to the migration of cacti development to github, one old bug that I care a bit about got lost.
CVE-2009-4112¹ still exists:
The issue is of course broader, as I can create a new Data Input Method, I don't need to change an existing one.
I find suggestions² that at the time the item was reported that cacti developers (at least one) were considering implementing a whitelist as a solution. I also find suggestions³ that this issue would be considered during design of 0.8.8, but that ship has sailed long time ago.
I do recognize that fixing this issue is difficult due to the design and philosophy of Data Input Methods. But maybe an (optional) whitelist could be implemented to only allow commands that are stored by the system administrator (via a configuration file on the file system). I.e. if the file is there, whitelisting is effective, and if it is not, than all commands are allowed.
The reason why I post this issue is because of recent other security issues (#1057 and #1066) in this tracker. It seems that as long as this old issue isn't fixed, fixing the other issues is not really improving security.
¹ https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-4112 ² https://security-tracker.debian.org/tracker/CVE-2009-4112 (in the notes) ³ http://www.securityfocus.com/archive/1/archive/1/508129/100/0/threaded (bottom of the page)