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

NewTokenLandingPage not working #55

Closed
LPmaverick opened this issue Feb 2, 2022 · 18 comments
Closed

NewTokenLandingPage not working #55

LPmaverick opened this issue Feb 2, 2022 · 18 comments
Labels
bug Something isn't working question Further information is requested

Comments

@LPmaverick
Copy link

Describe the bug
NewTokenLandingPage still doesn't work in 4.x and did not work in 3.x. In 3.x, I remember it was the httpSession.isNew() check was always false. I haven't tested 4.x yet to see if it is still that or the csrfGuard.isUseNewTokenLandingPage() check.

The result is a redirect to my configured org.owasp.csrfguard.action.Redirect.Page

To Reproduce
Steps to reproduce the behavior:
Providing WebSphere test app source code.
CSRFGuard4 test.zip

Try hitting this URL, http://localhost/CG4xWeb/app?param1=foo at

Expected behavior
The /app endpoint is protected which should cause a redirect to /home which is my configured NewTokenLandingPage; as well as org.owasp.csrfguard.unprotected.Default.

@forgedhallpass
Copy link
Member

Hello @LPmaverick,

Thank you for reporting this. I must say, I haven't tested this functionality, because I've never needed it, nor I am sure why it was introduced. Could you please explain what exactly would you like to achieve with it?

From the JavaDoc of the httpSession.isNew():

Returns true if the client does not yet know about the session or if the client chooses not to join the session. For example, if the server used only cookie-based sessions, and the client had disabled the use of cookies, then a session would be new on each request.

^^^ Based on this, the redirect to the NewTokenLandingPage should only happen, if the user does not have a JSESSIONID cookie set, but if your application requires authentication, your authentication filter should catch the request, hence this part of the code would not be reachable. The only scenario I can imagine this would be potentially useful is if it would be used to prevent anti-automation in case of applications that do not require an authenticated session.

Also note, that starting from 4.x CSRF Guard supports stateless/session-less web applications as well, in which case "emulating" the isNew() method would probably not make sense.

A potential fix to support this would be to add:

final boolean useNewTokenLandingPage = csrfGuard.isUseNewTokenLandingPage();
final LogicalSession logicalSession = useNewTokenLandingPage ? sessionKeyExtractor.extractOrCreate(httpServletRequest)
                                                             : sessionKeyExtractor.extract(httpServletRequest);

in

final LogicalSession logicalSession = sessionKeyExtractor.extract(httpServletRequest);

I've tested a basic use-case to make sure it would work, but it would need to be tested thoroughly.

@forgedhallpass forgedhallpass added bug Something isn't working question Further information is requested labels Feb 7, 2022
@LPmaverick
Copy link
Author

I only use CSRFGuard in my state-full web apps, and I do not use it for authentication. Not sure I understand why a synchronizer token be used for authentication and not an OAuth or similar identity token.

The behavior I expect is based on the description of the configuration

The new token landing page property (org.owasp.csrfguard.NewTokenLandingPage) defines where to send a user if the token is being generated for the first time, and the use new token landing page boolean property (org.owasp.csrfguard.UseNewTokenLandingPage) determines if any redirect happens. UseNewTokenLandingPage defaults to false if NewTokenLandingPage is not specified, and to true if it is specified.. If UseNewTokenLandingPage is set true then this request is generated using auto-posting forms and will only contain the CSRF prevention token parameter, if applicable. All query-string or form parameters sent with the original request will be discarded.

That description of isNew might actually explain the issues others have reported that utilize an SSO filter before the CSRFGuard filter, instead of the other way around. Though, I don't understand why it isn't working in my test app as I am only testing CSRFGuard.

Instead of the fix idea you provided, what if instead some flag is set when the token is generated for the first time? This flag would be be checked in the filter (instead of session.isNew) and then removed if used. However, I do not know how this would work with the TokenPerPage functionality as I do not use it nor recommend it to anyone in my company.

@forgedhallpass
Copy link
Member

Please read my comment again, I haven't said anything about using CSRF Guard for authentication. What I was assuming that your authentication filter would be placed first, and it would catch all requests that do not have a session, hence requests without a session would not be processed by CSRF Guard, ergo isNew() will always be false.

Though, I don't understand why it isn't working in my test app as I am only testing CSRF Guard.

For testing, you can try with the fix I've proposed, and it will work. I've tried it out myself.

The behavior I expect is based on the description of the configuration

Please don't paste the documentation of the property, but explain what exactly you'd like to achieve.

I do not know how this would work with the TokenPerPage functionality as I do not use it nor recommend it to anyone in my company.

Why wouldn't you recommend it? It makes considerably harder to access protected resources in case your application has an XSS vulnerability. In the past I've configured it for both stateless and stateful enterprise fintech applications, and it was working correctly.

@LPmaverick
Copy link
Author

Bah my bad, I conflated the CSRFGuard filter and authentication together from your comments. It is an annoying misconception that I often have to explain to others in my company.

The functionality I expect to happen is upon accessing an application for the first time where the CSRF token is initially generated, all query-string or form parameters sent with the original request are discarded and the application is redirected to the configured NewTokenLandingPage; which would generally be the app's home screen. At that point, the home screen (and other screens) should be utilizing the JavaScriptServlet or csrfguard-taglib to auto-inject the token into all forms and ajax requests.

I will try the fix you proposed. However, would it still work in instances where an authentication filter comes before the CSRFGuard filter? My company has an SSO filter that has proven quite finicky and is often configured as the first filter in the chain.

CSRFGuard is a great tool for architecting CSRF mitigation into an application. The reason I do not recommend the TokenPerPage functionality to others in my company is because those that utilize CSRFGuard are J2EE/JSP legacy applications and therefore have outdated standards and practices. They often do not utilize HTTP verbs correctly and implement other functionality that make the apps a wet ball of spaghetti. In these instances, we have found the TokenPerPage functionality adds needless complexity to apps that we would rather see modernized. Instead, we generally recommend using the core of CSRFGuard which is a single synchronizer token attached to the session. For XSS related vulnerabilities, ESAPI is the recommendation and again done so in a way that could be architected into the apps. Though many don't follow that practice and just go line-by-line.

@LPmaverick
Copy link
Author

Having compilation issues. I tried the mentioned solution https://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException but did not resolve my error. I am using Maven 3.8.1. Any ideas?

Running mvn clean install I get this

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for OWASP CSRFGuard Parent POM 4.1.1:
[INFO]
[INFO] OWASP CSRFGuard Parent POM ......................... SUCCESS [ 0.166 s]
[INFO] OWASP CSRFGuard .................................... FAILURE [ 0.813 s]
[INFO] csrfguard-extensions ............................... SKIPPED
[INFO] csrfguard-extension-session ........................ SKIPPED
[INFO] csrfguard-jsp-tags ................................. SKIPPED
[INFO] OWASP CSRFGuard Test Parent POM .................... SKIPPED
[INFO] OWASP CSRFGuard JSP Test WebApp .................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project csrfguard: Compilation failure

However, running mvn dependency:list everything resolves successfully

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for OWASP CSRFGuard Parent POM 4.1.1:
[INFO]
[INFO] OWASP CSRFGuard Parent POM ......................... SUCCESS [ 0.393 s]
[INFO] OWASP CSRFGuard .................................... SUCCESS [ 0.103 s]
[INFO] csrfguard-extensions ............................... SUCCESS [ 0.014 s]
[INFO] csrfguard-extension-session ........................ SUCCESS [ 0.020 s]
[INFO] csrfguard-jsp-tags ................................. SUCCESS [ 0.029 s]
[INFO] OWASP CSRFGuard Test Parent POM .................... SUCCESS [ 0.011 s]
[INFO] OWASP CSRFGuard JSP Test WebApp .................... SUCCESS [ 0.045 s]
[INFO] ------------------------------------------------------------------------

@forgedhallpass
Copy link
Member

Having compilation issues.

For questions please use the Discussions Q&A category. The project compiles just fine for me, and on the CI as well (see https://github.com/OWASP/www-project-csrfguard/actions). Please provide the full log (use the -X flag if required).

@forgedhallpass
Copy link
Member

The functionality I expect to happen is upon accessing an application for the first time where the CSRF token is initially generated, all query-string or form parameters sent with the original request are discarded and the application is redirected to the configured NewTokenLandingPage; which would generally be the app's home screen. At that point, the home screen (and other screens) should be utilizing the JavaScriptServlet or csrfguard-taglib to auto-inject the token into all forms and ajax requests.

I will try the fix you proposed. However, would it still work in instances where an authentication filter comes before the CSRFGuard filter? My company has an SSO filter that has proven quite finicky and is often configured as the first filter in the chain.

If your application requires authentication, in most of the use-cases your authentication filter needs to be the first in the chain. This is what I've assumed and written above. In this case, as mentioned previously, all the request that do not have a valid session associated will be redirected towards the login screen probably (meaning, no requests will even get to CSRF Guard, hence the httpSession.isNew() will always be false, and the NewTokenLandingPage logic will NOT be executed. That being said, you DON'T even need it! Your can reference the CSRF Guard JavaScript endpoint in the page where you redirect after login, and that will do the initialization etc.

There are a lot of configuration options you can use, based on your requirements.

CSRFGuard are J2EE/JSP legacy applications and therefore have outdated standards and practices. They often do not utilize HTTP verbs correctly and implement other functionality that make the apps a wet ball of spaghetti. In these instances, we have found the TokenPerPage functionality adds needless complexity to apps that we would rather see modernized.

A lot of legacy applications use CSRF Guard. The complexity of the TokenPerPage functionality is hidden from the user. Normally you shouldn't need to modify anything in your system for this to work, besides doing the configuration in the CSRF Guard property file.

@LPmaverick
Copy link
Author

How does the token per page work when an application has a single endpoint and the delivered JSP is determined by a query parameter? Is it based on the endpoint or the JSP page?

@forgedhallpass
Copy link
Member

forgedhallpass commented Feb 9, 2022

The solution protects endpoints/resources. You could probably make separate "page" protection rules (e.g. regex or wildcards), but if you only have one endpoint, it might not worth doing so.

The related logic:

public ProtectionResult isProtectedPage(final String normalizedResourceUri) {

The documentation:

If you use JSPs without AJAX, you could probably ditch the whole JavaScriptServlet and use the JSP tag library to inject the tokens on the server side. This would be more efficient, but would require some adjustment in your JSPs.

The project contains a test application that covers multiple scenarios. You could use that to understand/debug/try out different configurations.

See https://github.com/OWASP/www-project-csrfguard/tree/master/csrfguard-test/csrfguard-test-jsp

@LPmaverick
Copy link
Author

Bummer okay thanks. It would appear that this functionality isn't usable for many of the apps I come across at my company.

If you do look into changing this functionality, I would request that some request attribute be set upon token creation indicating as such. If that request attribute is present and ValidateWhenNoSessionExists is true, then the redirect to the configured NewTokenLandingPage would trigger. This would handle applications that have authentication filters before the CSRFGuard filter.

@forgedhallpass
Copy link
Member

I really wonder if am I not making myself clear, because it seems that you keep misunderstanding what I am saying.

If you want to, DM me on the ProjectDiscovery Discord server, and we can talk more directly...

@LPmaverick
Copy link
Author

I am not sure where the misunderstanding is.

It does not seem that NewTokenLandingPage will work due to the current conditional if checks since many/most of the apps at my company have an SSO/authentication filter configured first. This means that session.isNew will always evaluate to false, which is something you called out.

In addition, the JavaScriptServlet provides a convenient LOE for auto-injecting the token into forms and AJAX requests. I am excited for this functionality with respect to dynamic content as this was something not possible in 3.x; which I would recommend using the taglib in those instances.

@forgedhallpass
Copy link
Member

The most common use-case is to use CSRF Guard after an authentication filter! What I am trying to tell you is that, you might not even need the token landing page functionality and you can probably live without it. I've done different enterpise level integrations and I haven't needed it at all. Just reference the JavaScript logic in your pages and you should be good to go!

@LPmaverick
Copy link
Author

The use case I have seen is when people/teams have apps that will redirect to the configured error page upon an initial hit of their application of a protected resource. The NewTokenLandingPage provides a much more graceful, initial redirect into the application.

Yes, I understand that the configured error page could be the home page (where an error message could be displayed regarding a failed CSRF check) or provide a link to said homepage on the error page. However, I have found that to be a foreign concept at my company.

I am often not the one doing the code work, so I have to manage the LOE of my recommendations being asked of AppDev teams so that they are more apt to implement proper security controls like CSRFGuard and ESAPI instead of their own custom solutions that are more often than not flawed.

@forgedhallpass
Copy link
Member

Normally GET requests should not need to be protected, hence they should not trigger CSRF alarms. Now if your applications are doing state changing operations through GET requests, that's another problem (which you should sort out ASAP). The problem is in this case, how do you differentiate these use-cases from real attacks? An option would be for you to extend the main filter with some logic of your own, that would decide whether the request is state changing or not, and let CSRF Guard act on problematic ones only. As an alternative, there is support for Actions that are executed upon potential CSRF attacks. You can also use a redirect action to point wherever you need it to. There are multiple ways to achieve the same thing, depending on your exact requirements.

@LPmaverick
Copy link
Author

Interesting. I need to look more into these actions.

So I think that is the misunderstanding between us. Yes, assume I am talking about apps that are not using HTTP verbs correctly and have state-changing functionality via GET endpoints. I agree that this is the problem that should be corrected, and while I do suggest that be done, ultimately there is no teeth in making that a hard requirement. So, the alternative is to use the configurations of CSRFGuard to protect the app from itself.

@LPmaverick
Copy link
Author

Thinking more on the NewTokenLandingPage, it would be far more complex a change than I originally thought. It would depend on ValidateWhenNoSessionExists=true and for GET methods to be protected. Also, ideally this functionality I would only want to trigger on a protected GET call (since that will have state-changing functionality in this case), whereas a POST call I would prefer to throw the redirect error page.

forgedhallpass added a commit that referenced this issue Feb 22, 2022
* If the user has no session (e.g. missing JSESSIONID cookie) and the "org.owasp.csrfguard.UseNewTokenLandingPage" property is configured to true, the user will be redirected to the new token landing page (defined by the "org.owasp.csrfguard.NewTokenLandingPage" property). If the "NewTokenLandingPage" is protected, a new CSRF token is sent along in the auto-submit form. The request body and query parameters are discarded.
* Removed null check for the "getNewTokenLandingPage()" method in the "CsrfGuard.java#writeLandingPage" method, because the method is only being invoked if "org.owasp.csrfguard.UseNewTokenLandingPage" is true, which can only happen if the "org.owasp.csrfguard.NewTokenLandingPage" property is not null.

This is a very rare/special use-case that can only happen if an application without authentication is protected against CSRF attacks (probably anti-automation), or if for some reason the CSRF filter is before the integrator application's authentication filter (which is not recommended!), otherwise the authentication filter should redirect to the login page.
@forgedhallpass
Copy link
Member

Closed by 74008a2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants