Security issue allows to view all graphs

[george-kar]

I have created a user with no group membership. Graph permission default is DENY and I have selected all graphs Restricted instead of 1. I have device params default DENY and only set to specific one. Template perm is set to DENY. Tree Perms is also default DENY and I set to access only a specific one. When I login with that user I see only one option in tree and when I click on the device I see the graph. Everything is fine. But if I take the url

domain.com/graph_json.php?rra_id=0&local_graph_id=3205&graph_start=1569069730&graph_end=1569156130&graph_height=120&graph_width=500

and change the local_graph_id with another value ie 5312 I get a response and inside that response I see the image which is base64. If I decode and create png I can see the other graph that I dont have permission to see.

I tried to update to latest cacti 1.2.6 and it still get that security issue. I also checked the source code carefully and I don't see any permission check regarding graphs. For example I see permission check for tree node creation but not for graphs.

[cigamit]

@george-kar, we need a CVE on this, can you log one?

[bmfmancini]

Wow good find @george-kar !!!!
thanks @cigamit for the fast resolve !

[george-kar]

@cigamit I cant open CVE as the activation email never arrived. Can you please let me know if you already fix it and there is a fix on it?

UPDATE: Requested CVE on https://cveform.mitre.org

[george-kar]

I see the fix but you have wrong post mention (2954). Please fix your code fix with the 2964 instead of 2954.

[cigamit]

Initial commit was here: 7a6a172

[george-kar]

If you see commit mentions 2954 instead of 2964. People cannot track commit with the proper thread.

[netniV]

Without rebasing, the commit history cannot be changed. It will not be changed. So, if necessary, pull the two commit references to highlight the mistake during commit.

[carnil]

CVE-2019-16723 was assigned for this issue.

[hlef]

Quoting Debian bug report #941036:

Graphs are retrieved using rrdtool_function_graph() from lib/rrd.php, this is true for jessie onwards.

rrdtool_function_graph() has a check for permissions, which is in fact very similar to the ones introduced in 7a6a172 and c7cf4a2.

Before cf73ae1 this check in rrdtool_function_graph() was always executed. After this commit the check is only executed when $user > 0.

Note: 0 is the default value for $user:

[lib/rrd.php:1179]

function rrdtool_function_graph($local_graph_id, $rra_id, $graph_data_array,
    $rrdtool_pipe = '', &$xport_meta = array(), $user = 0) {
...

However graph_image.php, graph_json.php and rrdtool_function_xport() call rrdtool_function_graph() without passing $user:

[graph_image.php:132]

$output = rrdtool_function_graph(get_request_var('local_graph_id'), $rra_id, $graph_data_array);

Hence, permissions are never checked after this commit. I don't think this is the intended affect.

Now, let's try something: take 1.2.2+ds1-2+deb10u1, the version in buster which is affected and simply revert cf73ae1. Now try to reproduce: this is sufficient to "fix" the issue and appears to confirm previous analysis.

@cigamit any comment? Can you confirm that this vulnerability was introduced in cf73ae1?

[cigamit]

Yea, that's a good point. I know why I made the original change. Let me see if I can redo the fix.

[cigamit]

Okay, this has been fixed correctly now.

[ddb4github]

Comments for cfb0733

1. Why add 'directories=no' in graph.php?
2. The new solution does not add 'sess_user_id for the 2nd rrdtool_function_graph usage in graph_image.php
3. Suggest remove unused param $xport_meta from function rrdtool_function_graph definition, and update all caller
   Or change all rrdtool_function_graph caller to put 'array()' as 5th param, $_SESSION['sess_user_id'] as 6th param.

More comments for other rrdtool_function_graph caller
Find by grep, and just reproduce aggregate_graphs.php

1. aggregate_graphs.php-->Preview is able to show rrd command and status when graph image is denied due to permission.
2. thold_functions.php and graph_realtime.php also use rrdtool_function_graph without user_id

[hlef]

@ddb4github

1. this is an obsolete argument to window.open (javascript). I don't think it does anything (maybe on old IE browsers)
2. I think you're right, the sess_user_id is missing in the 2nd call. However this should not have any security impact. In fact this second call is only executed in case of an error, and the result is not returned to the user. This might be a consistency issue, though.
3. This is weird, indeed. $xport_meta does not appear to be used. Definitely worth fixing.

@cigamit?

[netniV]

With regards to the older parameters, it may be worth cycling between the release (git checkout release/) and seeing when it became unused. I haven't had time to do that yet, but that's usually what I do to vet why a parameter is suddenly appearing extraneous.

[ddb4github]

As my previous comments, we can take action below:

• change all rrdtool_function_graph caller to put 'array()' as 5th param, $_SESSION['sess_user_id'] as 6th param.

Before go thru rrdtool_function_graph usage from each cacti and plugins delivered release(0.x to 1.x)

[cigamit]

Is there any issue you think in keeping it null though for some of the calls?

[cigamit]

@hlef, $xport_meta used in graph_xport.php for CSV export.

[cigamit]

@ddb4github, since xport_meta is pass by reference, I'm not sure which the best approach would be. It would be really nice if browsers 'could' hide the address bar. However, I know why they are doing it... Phishing attack.

[ddb4github]

Just one comment about $xport_meta:
@cigamit removed $xport_meta usage with $graph_variables by c2bc81f

So I think we can safely remove $xport_meta from function rrdtool_function_xport and rrdtool_function_graph 😆

BTW, the newest rrdtool_function_graph usage in latest commit 9a1d2ec still mismatch function rrdtool_function_graph definition:

• Definition: rrdtool_function_graph($local_graph_id, $rra_id, $graph_data_array, $rrdtool_pipe = false, &$xport_meta = array(), $user = 0<the 6th param>)
• Caller: rrdtool_function_graph(get_request_var('local_graph_id'), '', $graph_data_array, null, $_SESSION['sess_user_id']<the 5th param>);

PR for a temp fix before we remove $xport_meta

[ddb4github]

BTW, plugin_npc/top_graph_header.php:147:

print trim(rrdtool_function_graph($_GET["local_graph_id"], $_GET["rra_id"], $graph_data_array));

[cigamit]

NPC is still kind of waiting for someone to take it over. It's in kind of an archive status.