Navigation Menu

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

Warnings can appear from CSRF Magic library due to multiple token values being found #3317

Closed
ddb4github opened this issue Mar 4, 2020 · 6 comments
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@ddb4github
Copy link
Contributor

Describe the bug
Sometimes, CSRF report error as below.

2020/03/04 04:04:12 - CMDPHP PHP ERROR NOTICE Backtrace: (/plugins.php[25]:include(), /include/auth.php[28]:unknown(), /include/global.php[496]:include_once(), /include/csrf.php[31]:include_once(), /include/vendor/csrf/csrf-magic.php[580]:csrf_check(), /include/vendor/csrf/csrf-magic.php[88]:CactiErrorHandler()) 
2020/03/04 04:04:12 - ERROR PHP NOTICE: Array to string conversion in file: /var/www/html/cacti/include/vendor/csrf/csrf-magic.php on line: 88 

To Reproduce
Not sure how to reproduce, before/after error i access several page, and uninstall/install syslog plugin

Possible Diff

--- csrf-magic.php    2020-03-04 04:13:32.712019835 +0000
+++ csrf-magic.php    2020-03-04 04:27:08.960811931 +0000
@@ -85,9 +85,6 @@
                        // schemes are volatile.
                        $tokens = $_POST[$name];
                        $result = csrf_check_tokens($tokens);
+                       if (is_array($tokens)){
+                               $tokens = implode(';', $tokens);
+                       }
                        csrf_log(__FUNCTION__,"check_tokens($name, $tokens) returned $result");
                }
@netniV
Copy link
Member

netniV commented Mar 4, 2020

Well, tokens comes from the $_POST, which I would have expected to be a string, unless there are multiple token values? Could that make it an array, and if so, I suspect the csrf_check_tokens() call would be wrong too.

@ddb4github
Copy link
Contributor Author

Well, tokens comes from the $_POST, which I would have expected to be a string, unless there are multiple token values? Could that make it an array, and if so, I suspect the csrf_check_tokens() call would be wrong too.

csrf_check_tokens work well. Previous diff idea come from csrf_check_tokens, as below:

function csrf_check_tokens($tokens) {
	if (is_string($tokens)) {
		$tokens = explode(';', $tokens);
	}

	$valid_token = false;
	foreach ($tokens as $token) {

@netniV
Copy link
Member

netniV commented Mar 6, 2020

Tokens should be a string made up like 'sid:xxxxx;ip:xxxxx' (key/value pairs separated by semi colons). So, imploding as you've mentioned could merge multiple values in my mind into a single string (eg 'sid:xxxxx;ip:xxxxx;sid:yyyyy;ip:yyyyy' etc)

@cigamit
Copy link
Member

cigamit commented Mar 8, 2020

This is due to there being two instances of the CSRF token in the post. So, I'm okay with this, but there is a question as to which one is the right token if they are different. That is yet to be determined.

cigamit added a commit that referenced this issue Mar 8, 2020
CSRF report PHP Notice: Array to string conversion
@cigamit cigamit added this to the 1.2.11 milestone Mar 8, 2020
@cigamit cigamit added bug Undesired behaviour resolved A fixed issue labels Mar 8, 2020
@cigamit
Copy link
Member

cigamit commented Mar 8, 2020

This is resolved now.

@cigamit cigamit closed this as completed Mar 8, 2020
@netniV
Copy link
Member

netniV commented Mar 8, 2020

The dual token was something I had wondered in the past. It may be that a form is creating a field with the token, but then our AJAX is also doing it.

@netniV netniV changed the title CSRF report PHP Notice: Array to string conversion Warnings can appear from CSRF Magic library due to multiple token values being found Apr 5, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 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