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

Allow packagers to be able to specify an alternate location of csrf-secret.php file #3039

Closed
ddb4github opened this issue Oct 18, 2019 · 26 comments
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@ddb4github
Copy link
Contributor

Describe the bug
csrf_get_secret function want to generate a secret file if not exist.
Installer should check cacti/include/vendor/csrf permission:
For install procedure only if installer create a secret file
Or always writable

@netniV
Copy link
Member

netniV commented Oct 18, 2019

Hmm, that sounds like an interesting problem. The issue being that the secret file is created for any forms. I think we would have to go back a step and put it into global.php as a fatal error given that from 1.2.x, you have to log in BEFORE the installer even starts.

@paulgevers do you do anything fancy with the CSRF package inclusion ? Do I need to check a different folder for the secret file and if its directory is writable?

@netniV
Copy link
Member

netniV commented Oct 18, 2019

@mortenstevens not one to leave you out, but do you strip out the vendor folder too?

@netniV netniV changed the title Installer should check cacti/include/vendor/csrf permission When the CSRF secret file is missing, folder permissions should be checked Oct 18, 2019
@netniV netniV changed the title When the CSRF secret file is missing, folder permissions should be checked When the CSRF secret file is missing, folder permissions for secret file should be checked Oct 18, 2019
@paulgevers
Copy link
Contributor

paulgevers commented Oct 18, 2019 via email

@cigamit cigamit added the installer Installation issue label Oct 19, 2019
@cigamit
Copy link
Member

cigamit commented Oct 19, 2019

When install, I've never seen this problem since I set the ownership to apache:apache and therefore the csrf call always is able to generate the file. I wonder if we can relocate the file to /etc/cacti for debian and /var/www/html/cacti/include for base cacti?

@netniV
Copy link
Member

netniV commented Oct 20, 2019

As I said above, this is not an installer issue it's pre-installer, much like a database connection. If CSRF isn't available, you won't be able to log in. I'll try and review tonight what we can do about it.

@netniV netniV added bug Undesired behaviour and removed installer Installation issue labels Oct 20, 2019
@netniV
Copy link
Member

netniV commented Oct 21, 2019

Looking at the code in include/vendor/csrf/csrf-magic.php, it does already check that the directory is writable and only generates a secret if it is.

function csrf_get_secret() {
    if ($GLOBALS['csrf']['secret']) return $GLOBALS['csrf']['secret'];
    $dir = dirname(__FILE__);
    $file = $dir . '/csrf-secret.php';
    $secret = '';
    if (file_exists($file)) {
        include $file;
        return $secret;
    }
    if (is_writable($dir)) {
        $secret = csrf_generate_secret();
        $fh = fopen($file, 'w');
        fwrite($fh, '<?php $secret = "'.$secret.'";' . PHP_EOL);
        fclose($fh);
        return $secret;
    }
    return '';
}

So, I am now going to revise my previous answer and say that we can add this as a requirement check since the secret will not be generated and should still allow logging in.

I think it also needs adding to the tech support page to make sure it shows if the CSRF has actually been generated.

@cigamit cigamit added the installer Installation issue label Nov 9, 2019
@cigamit
Copy link
Member

cigamit commented Nov 9, 2019

Maybe we need to make this a config item in config.php. That way the package maintainers can locate in places like /etc/cacti for example.

@cigamit cigamit changed the title When the CSRF secret file is missing, folder permissions for secret file should be checked Allow packagers to be able to specify an alternate location of csrf-secret.php file Nov 9, 2019
cigamit added a commit that referenced this issue Nov 9, 2019
Allow packagers to be able to specify an alternate location of csrf-secret.php file
@cigamit
Copy link
Member

cigamit commented Nov 9, 2019

@paulgevers, you should take note to the solution here.

@cigamit cigamit added resolved A fixed issue and removed installer Installation issue labels Nov 9, 2019
@cigamit cigamit closed this as completed Nov 9, 2019
@paulgevers
Copy link
Contributor

Hi,

I wonder how this csrf secrets file is supposed to work (my php knowledge is not extremely big). Is the user vulnerable to CSS without the secrets file?

I think that as a maintainer, I don't want to create the file, but just direct Cacti to a location where it is allowed to write. In my case, I decided on /var/lib/cacti/csrf/secret.php. However, although that file doesn't yet exist and the directory is writable by the www-data user (the one that runs apache), I don't see the secret file appearing when I log in as admin. What am I missing?

paul@testavoira ~ $ sudo grep '$path_csrf_secret' /etc/cacti/debian.php 
$path_csrf_secret = '/var/lib/cacti/csrf/secret.php';
paul@testavoira ~ $ sudo ls -al /var/lib/cacti/csrf/
total 8
drwxrwx--- 2 root www-data 4096 dec 12 20:52 .
drwxr-xr-x 5 root root     4096 dec 12 21:11 ..
paul@testavoira ~ $

@netniV
Copy link
Member

netniV commented Dec 13, 2019

Overview

The following is the code that was modified as mentioned above.

   if ($GLOBALS['csrf']['secret']) return $GLOBALS['csrf']['secret'];

        if (isset($config['path_csrf_secret'])) {
                $dir  = dirname($config['path_csrf_secret']);
                $file = $config['path_csrf_secret'];

                if (file_exists($file)) {
                        include($file);
                        return $secret;
                }
        } else {
            $dir    = dirname(__FILE__);
                $file   = $dir . '/csrf-secret.php';
                $secret = '';
                if (file_exists($file)) {
                        include($file);
                        return $secret;
                }
        }

        if (is_writable($dir)) {
                $secret = csrf_generate_secret();
                $fh = fopen($file, 'w');
                fwrite($fh, '<?php $secret = "'.$secret.'";' . PHP_EOL);
                fclose($fh);

Step 1 - Check Global

   if ($GLOBALS['csrf']['secret']) return $GLOBALS['csrf']['secret'];

If the secret is already part of the globals, that will be used. This can happen if the session happens to load it, if I remember rightly.

Step 2 - Check predefined packager path

        if (isset($config['path_csrf_secret'])) {
                $dir  = dirname($config['path_csrf_secret']);
                $file = $config['path_csrf_secret'];

                if (file_exists($file)) {
                        include($file);
                        return $secret;
                }

If we have a path defined for the CSRF secret, lets use the directory of that file and set that as the base ($dir). If the file specified itself doesn't exist, drop throw the part 4.

Step 3 - Otherwise, fall back to standard behaviour

        } else {
            $dir    = dirname(__FILE__);
                $file   = $dir . '/csrf-secret.php';
                $secret = '';
                if (file_exists($file)) {
                        include($file);
                        return $secret;
                }
        }

Assume that the secret path should be in the same directory as the reset of the CSRF data. If it exists, use it, otherwise, fall through to step 4.

Step 4 - See if we can write the secret

        if (is_writable($dir)) {
                $secret = csrf_generate_secret();
                $fh = fopen($file, 'w');
                fwrite($fh, '<?php $secret = "'.$secret.'";' . PHP_EOL);
                fclose($fh);

See if the directory to hold the secret is writable, and if so, store the generated secret into it.

@paulgevers
Copy link
Contributor

@netniV thanks for the detailed explanation. However, I was more referring to the situation where csrf_get_secret returns ''. What happens with CSS protection by csrf in that case?

Also, any idea why the file isn't created on my system?

@netniV
Copy link
Member

netniV commented Dec 16, 2019

If the secret is blank, the csrf_get_secret() function will do as I described above which is why I described it :)

php > if ('') echo 'test blank';
php > if (!'') echo 'test not blank';
test not blank

A blank string is effectively false to PHP. Also, if somehow a blank string was returned as the secret, it is used by csrf_check_token() and that will also return false if there is no secret.

Ultimately, the real use of the secret is within the csrf_hash() function:

/**
 * Generates a hash/expiry double. If time isn't set it will be calculated
 * from the current time.
 */
function csrf_hash($value, $time = null) {
    if (!$time) $time = time();
    if (function_exists("hash_hmac")) {
        return hash_hmac('sha1', $time . ':' . $value, csrf_get_secret()) . ',' . $time;
    } else {
        $secret = csrf_get_secret();
        return sha1($secret . sha1($secret . $time . ':' . $value)) . ',' . $time;
    }
}

This hashing function is then used by csrf_get_tokens():

...
                $ip = ';ip:' . csrf_hash($_SERVER['REMOTE_ADDR']);
...
    if (session_id()) return 'sid:' . csrf_hash(session_id()) . $ip;
...
    if ($GLOBALS['csrf']['key']) return 'key:' . csrf_hash($GLOBALS['csrf']['key']) . $ip;
...
        return 'user:' . csrf_hash($GLOBALS['csrf']['user']);

If there is no secret or mode of operation (user, cookie, key, etc), the result from csrf_get_tokens() will be 'Invalid'. In essence, it's my understanding that the 'secret' is more of a 'salt' in proper encryption terms but does have to exist or it'll supposed invalid any POST requests?

@netniV
Copy link
Member

netniV commented Dec 16, 2019

Have you checked what cookie value you are getting as you should have a cookie?

@paulgevers
Copy link
Contributor

@netniV I do have 1 cookie, its value is gl3v7ds6c9ve3fa0rp6tigil4h, does that look like a valid csrf value?

I still don't have a file, so it seems so far I'm either not touching that part of the code flow, or getting '' isn't any problem for the code flow.

@netniV
Copy link
Member

netniV commented Dec 17, 2019

Hnm, without doing a debug session I couldn't say off the top of my head. As you have a cookie, is that updated every time or was it last updated a while ago?

@paulgevers
Copy link
Contributor

@netniV I should probably store it right now (I cleared it yesterday), but the current cookie appears to have the same content as the one I saw yesterday on my second run. The cookie stays the same when I log out and in again.

@paulgevers
Copy link
Contributor

@netniV I released 1.2.8 just now to Debian and my PPA, however, without setting the csrf-secret path. If we can understand what's going on, that would be great. I probably need more hints how to debug this.

@netniV
Copy link
Member

netniV commented Dec 30, 2019

Sorry, I've been working over the Christmas period so not had that much free time. I'll try and look into what is going on with this over the next week before the 1.2.9 release next weekend.

@netniV netniV added this to the v1.2.9 milestone Dec 30, 2019
@TheWitness
Copy link
Member

I'm thinking we add the creation to the installer script @paulgevers, are you running that in you dpk?

@paulgevers
Copy link
Contributor

@TheWitness no, we don't. In the Debian package I (try to) do everything needed to have Cacti up and running. So far that never involved the installer script (as that script (used to be?) is interactive). Installing the Cacti package and have it running should not require user interaction. I'll look into what the script does nowadays.

@netniV
Copy link
Member

netniV commented Jan 2, 2020

There is a newer cli installer than can accept the JSON settings for everything and install automatically installed of prompting the user.

@paulgevers
Copy link
Contributor

@netniV I guess I should probably rework the Debian installation logic to avoid further issues. What will happen if I don't provide all the JSON settings (future extensions of the installer)?

@netniV
Copy link
Member

netniV commented Jan 3, 2020

They use the defaults. I also forgot that a customer is doing a power down this weekend, so I won't be able to have a good look until Sunday which is also when I planned to do the 1.2.9 release.

@cigamit
Copy link
Member

cigamit commented Jan 4, 2020

Guys, I've added a step to the installer. So, @paulgevers, you should simply set the path to /etc/cacti/csrf-secret.php, in the config.php file, and then run the installer cli program. The installer will now initialize that path to the path indicated in the $path_csrf_secret variable in config.php.

@netniV
Copy link
Member

netniV commented Jan 17, 2020

Hi Paul,

One thing, if you are using a debian package for CSRF, that may not have our modifications to support Cacti within it. Can you confirm either way so we know?

We also just made a slight tweak as there was an issue with the installer trying to kick CSRF in during the background installation.

Thanks,

Mark.

@paulgevers
Copy link
Contributor

@netniV Debian has CSRF from Cacti. I.e. I don't strip that one out.

@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

5 participants