False positive on POST #51

Closed
gnm67 opened this Issue Sep 9, 2016 · 20 comments

Comments

Projects
None yet
2 participants
@gnm67

gnm67 commented Sep 9, 2016

I have an issue in that the token is not being validated on all forms.
The library is configured and I have confirmed it does work.
But on some pages (sample attached) it always gives the 403 error.

test2.html.txt

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 19, 2016

Owner

@gnm67 is this the actual source code or one sent to browser after output is modified by CSRFP?

Owner

mebjas commented Sep 19, 2016

@gnm67 is this the actual source code or one sent to browser after output is modified by CSRFP?

@mebjas mebjas added the repro needed label Sep 19, 2016

@mebjas mebjas self-assigned this Sep 19, 2016

@mebjas mebjas added this to the v0.2.0 milestone Sep 19, 2016

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 19, 2016

The source code attached is the output from the browser after it has been modified by CSRFP

Original code is PHP.
The module is called using a auto-pre-pend file.

PHP 5.6 / Apache 2.4 / Windows 7 & 2012 Server

gnm67 commented Sep 19, 2016

The source code attached is the output from the browser after it has been modified by CSRFP

Original code is PHP.
The module is called using a auto-pre-pend file.

PHP 5.6 / Apache 2.4 / Windows 7 & 2012 Server

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 19, 2016

Owner

wait, are you using the release v0.1.0 ? if so could you test with the clone of this repository if you are facing same issues?

Owner

mebjas commented Sep 19, 2016

wait, are you using the release v0.1.0 ? if so could you test with the clone of this repository if you are facing same issues?

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 19, 2016

I think I downloaded it from 'master'

gnm67 commented Sep 19, 2016

I think I downloaded it from 'master'

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 19, 2016

Owner

From the output it looks like it's you are using older version, try downloading this https://github.com/mebjas/CSRF-Protector-PHP/archive/master.zip and test with this.

Reason: If you look at https://github.com/mebjas/CSRF-Protector-PHP/blob/master/libs/csrf/csrfprotector.php#L409 you'll find that code snippet (from your output)

<script type="text/javascript">
window.onload = function() {
    csrfprotector_init();
};
</script>

is no longer attached to HTML output also in line 399 some extra hidden input is added for the library to work, which isn't available in your output.

Owner

mebjas commented Sep 19, 2016

From the output it looks like it's you are using older version, try downloading this https://github.com/mebjas/CSRF-Protector-PHP/archive/master.zip and test with this.

Reason: If you look at https://github.com/mebjas/CSRF-Protector-PHP/blob/master/libs/csrf/csrfprotector.php#L409 you'll find that code snippet (from your output)

<script type="text/javascript">
window.onload = function() {
    csrfprotector_init();
};
</script>

is no longer attached to HTML output also in line 399 some extra hidden input is added for the library to work, which isn't available in your output.

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 20, 2016

Owner

@gnm67 did you try?

Owner

mebjas commented Sep 20, 2016

@gnm67 did you try?

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 20, 2016

Sorry I'm on holiday this week I will try when I get back in the office next week.

Thanks, Gavin
Sent via mobile

---- minhaz wrote ----

@gnm67https://github.com/gnm67 did you try?

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/mebjas/CSRF-Protector-PHP/issues/51#issuecomment-248332001, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AP5DUdEV2QYut9-QywQ4v3-12YrLT9gxks5qr_fwgaJpZM4J4xkE.

gnm67 commented Sep 20, 2016

Sorry I'm on holiday this week I will try when I get back in the office next week.

Thanks, Gavin
Sent via mobile

---- minhaz wrote ----

@gnm67https://github.com/gnm67 did you try?

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/mebjas/CSRF-Protector-PHP/issues/51#issuecomment-248332001, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AP5DUdEV2QYut9-QywQ4v3-12YrLT9gxks5qr_fwgaJpZM4J4xkE.

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 26, 2016

I have downloaded the master and done some more testing.

the page that is giving problems is the the login page after a logout.

the logout uRL is for example
\home\logout.php
\index.php?logout
when they have been logged out the URL is as above and the login screen displayed again.
with a POST action to for example
\home\login.php
\index.php

In these instances the POST always fails with the 403 error
if the page is changed to
\home\login.php
\index.php
then the login works OK.

gnm67 commented Sep 26, 2016

I have downloaded the master and done some more testing.

the page that is giving problems is the the login page after a logout.

the logout uRL is for example
\home\logout.php
\index.php?logout
when they have been logged out the URL is as above and the login screen displayed again.
with a POST action to for example
\home\login.php
\index.php

In these instances the POST always fails with the 403 error
if the page is changed to
\home\login.php
\index.php
then the login works OK.

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 26, 2016

Owner

@gnm67 I think I know what going wrong. So when you logout you do something like session_destroy() which destroys the session for user and they are logged out.
What you could do here is

  • rather than destroying session entirely only unset the variable you are validating for session.
  • In case you need to destroy the session, could you just backup the token and feed it back to session?

If it's ok with you could you point me to logout script? just to validate the theory?

Also, thanks for reporting the bug we didn't think of this scenario :D

Owner

mebjas commented Sep 26, 2016

@gnm67 I think I know what going wrong. So when you logout you do something like session_destroy() which destroys the session for user and they are logged out.
What you could do here is

  • rather than destroying session entirely only unset the variable you are validating for session.
  • In case you need to destroy the session, could you just backup the token and feed it back to session?

If it's ok with you could you point me to logout script? just to validate the theory?

Also, thanks for reporting the bug we didn't think of this scenario :D

@mebjas mebjas added the bug label Sep 26, 2016

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 27, 2016

I have checked our code and on the logout we do a

session_unset();
session_destroy();

as a workaround I have done this

    //session_unset();
    //@session_destroy();
    if (isset($_SESSION["CSRFP_TOKEN"])) $temp = $_SESSION["CSRFP_TOKEN"];
    @session_unset();
    @session_destroy();
    @session_start();
    if (isset($temp)) $_SESSION["CSRFP_TOKEN"] = $temp;

gnm67 commented Sep 27, 2016

I have checked our code and on the logout we do a

session_unset();
session_destroy();

as a workaround I have done this

    //session_unset();
    //@session_destroy();
    if (isset($_SESSION["CSRFP_TOKEN"])) $temp = $_SESSION["CSRFP_TOKEN"];
    @session_unset();
    @session_destroy();
    @session_start();
    if (isset($temp)) $_SESSION["CSRFP_TOKEN"] = $temp;
@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 27, 2016

Owner

and it worked?

Owner

mebjas commented Sep 27, 2016

and it worked?

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Sep 27, 2016

this worked, but is a bit clumsy.
Also now having problems with phpmyadmin.
I had hoped that doing it in a auto-pre-pend would make it easier and would then cover all sites on the server, but it is causing too many problems to handle at the moment.
I will have to forget it for now and try doing it again either server wide or per site when I have more time.

gnm67 commented Sep 27, 2016

this worked, but is a bit clumsy.
Also now having problems with phpmyadmin.
I had hoped that doing it in a auto-pre-pend would make it easier and would then cover all sites on the server, but it is causing too many problems to handle at the moment.
I will have to forget it for now and try doing it again either server wide or per site when I have more time.

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Sep 27, 2016

Owner

I don't get it. One thing is session_destroy() is creating issues with PMA.
Ideally what you should try is change logout logic to unset($_SESSION['id_or_something'])

though this means you have to look for this as a authentication thingy for logged in user. But ideally that's how it's done.

How do you check if a user is logged in or not using session?

Owner

mebjas commented Sep 27, 2016

I don't get it. One thing is session_destroy() is creating issues with PMA.
Ideally what you should try is change logout logic to unset($_SESSION['id_or_something'])

though this means you have to look for this as a authentication thingy for logged in user. But ideally that's how it's done.

How do you check if a user is logged in or not using session?

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Oct 7, 2016

The session_destroy() is working fine for the logout.

the problem I have with PhpMyAdmin is just by applying the module then PMA fails with 403 errors.

gnm67 commented Oct 7, 2016

The session_destroy() is working fine for the logout.

the problem I have with PhpMyAdmin is just by applying the module then PMA fails with 403 errors.

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Oct 11, 2016

Owner

What I don't get is why applying this to PMA on the first place, although it should play along but I haven't tested. It could be PMA's internal CSRF Protection doesn't plays with this one.

And if this is about PMA logging out with session_destroy(), I think that makes sense if both of them are working in same domain.

Owner

mebjas commented Oct 11, 2016

What I don't get is why applying this to PMA on the first place, although it should play along but I haven't tested. It could be PMA's internal CSRF Protection doesn't plays with this one.

And if this is about PMA logging out with session_destroy(), I think that makes sense if both of them are working in same domain.

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Oct 12, 2016

We have many sites on each server, I was applying the module to the server as a auto-pre-pend file so that it is applied to everything rather than having to edit each individual site which could lead to it being forgotten.
The logging out session_destroy() doesn't affect PMA.

gnm67 commented Oct 12, 2016

We have many sites on each server, I was applying the module to the server as a auto-pre-pend file so that it is applied to everything rather than having to edit each individual site which could lead to it being forgotten.
The logging out session_destroy() doesn't affect PMA.

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Oct 12, 2016

Owner

Oh that way. Put a filter on path of the script, something like this pseudocode

if not ( __DIR__ contain PMA PATH ) csrfp::init()

as a workaround for now

Owner

mebjas commented Oct 12, 2016

Oh that way. Put a filter on path of the script, something like this pseudocode

if not ( __DIR__ contain PMA PATH ) csrfp::init()

as a workaround for now

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Nov 3, 2016

Owner

@gnm67 so where are we on this? workaround works?

Owner

mebjas commented Nov 3, 2016

@gnm67 so where are we on this? workaround works?

@gnm67

This comment has been minimized.

Show comment
Hide comment
@gnm67

gnm67 Nov 4, 2016

yes the workaround worked.


From: minhaz notifications@github.com
Sent: 03 November 2016 21:24
To: mebjas/CSRF-Protector-PHP
Cc: gnm67; Mention
Subject: Re: [mebjas/CSRF-Protector-PHP] False positive on POST (#51)

@gnm67https://github.com/gnm67 so where are we on this? workaround works?

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/mebjas/CSRF-Protector-PHP/issues/51#issuecomment-258278541, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AP5DUdXrr25nu5iYqM36efRrx2pXEtp5ks5q6lEVgaJpZM4J4xkE.

gnm67 commented Nov 4, 2016

yes the workaround worked.


From: minhaz notifications@github.com
Sent: 03 November 2016 21:24
To: mebjas/CSRF-Protector-PHP
Cc: gnm67; Mention
Subject: Re: [mebjas/CSRF-Protector-PHP] False positive on POST (#51)

@gnm67https://github.com/gnm67 so where are we on this? workaround works?

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/mebjas/CSRF-Protector-PHP/issues/51#issuecomment-258278541, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AP5DUdXrr25nu5iYqM36efRrx2pXEtp5ks5q6lEVgaJpZM4J4xkE.

@mebjas mebjas added wontfix and removed repro needed labels Nov 4, 2016

@mebjas

This comment has been minimized.

Show comment
Hide comment
@mebjas

mebjas Nov 4, 2016

Owner

Closing with the blog - article

Owner

mebjas commented Nov 4, 2016

Closing with the blog - article

@mebjas mebjas closed this Nov 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment