Skip to content

python: minimal CSRF implementation #8340

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

Merged
merged 21 commits into from
May 10, 2022
Merged

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Mar 4, 2022

Only handles django projects. Criteria are:

  • custom middleware without CSRF protection, but with authentication
  • no local proaction of functions via decorators
  • test code is disregarded

This is expressed via concepts, specifically the CsrfProtectionSetting concept is copied from Ruby to allow sharing down the road. I added the CsrfLocalProtection concept to tighten the query.

- currectly only looks for custom django middleware
@yoff yoff added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 23, 2022
@yoff yoff marked this pull request as ready for review March 23, 2022 13:07
@yoff yoff requested a review from a team as a code owner March 23, 2022 13:07
@yoff
Copy link
Contributor Author

yoff commented Mar 24, 2022

Evaluation looks fine.

@yoff yoff removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 24, 2022
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides the in-line comments, can we move the new concepts to HTTP::Server? (that seems most appropriate)

Comment on lines 117 to 120
* Gets the boolean value corresponding to if CSRF protection is enabled
* (`true`) or disabled (`false`) by this node.
*/
boolean getVerificationSetting() { result = super.getVerificationSetting() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we benefit from this true/false result, or could it be a plain predicate csrfEnabled()? (a bit of a nitpick)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from Ruby, but I think a predicate would work just as well and am happy to change it. We can unify now or once we try to share.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Then it would be good to align with ruby on this, since the 3-state of true/false/none() could be important for them (such as app.setCsrfEnabled(<unknown value>)) -- maybe that IS actually a good thing to be able to model from the get go. I think it is, so ignore this comment ✔️

Comment on lines 149 to 153
/**
* Gets a `Function` representing the protected interaction
* (probably a request handler).
*/
Function getProtected() { result = super.getProtected() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since request handlers can also be made exempt from CSRF with a decorator (see https://docs.djangoproject.com/en/4.0/ref/csrf/#django.views.decorators.csrf.csrf_exempt), I think it would be better if this concept would capture that the CSRF protection was changed, and whether it is now enabled/disabled.

This also means we need to change the predicate that gets the request handler(s) affected.

Suggested change
/**
* Gets a `Function` representing the protected interaction
* (probably a request handler).
*/
Function getProtected() { result = super.getProtected() }
/**
* Gets a request handler whose CSRF protection is changed.
*/
Function getRequestHandler() { result = super.getProtected() }

You initially said "probably a request handler", are there any cases where you would care to change CSRF protection that is NOT a request handler? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, this concept should be broader.
I would expect that we only put such decorators on request handlers, but do we want to require that our request handler concept includes the function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that we only put such decorators on request handlers, but do we want to require that our request handler concept includes the function?

No. I would expect that only a few request handlers have their CSRF settings manually changed, so I think it's fine to have it like this. (with the changes I suggested)

yoff and others added 4 commits March 25, 2022 11:42
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
removing custom middleware stack
will _not_ enable CSRF protection
@yoff yoff requested a review from a team as a code owner March 25, 2022 12:03
@github-actions github-actions bot added the Ruby label Mar 25, 2022
@yoff yoff requested a review from RasmusWL March 25, 2022 12:03
@github-actions
Copy link
Contributor

QHelp previews:

python/ql/src/Security/CWE-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by overriding the default middleware stack and not including the one protecting against CSRF.

MIDDLEWARE = [
    'django.middleware.security.SecurityMiddleware',
    'django.contrib.sessions.middleware.SessionMiddleware',
    'django.middleware.common.CommonMiddleware',
    # 'django.middleware.csrf.CsrfViewMiddleware',
    'django.contrib.auth.middleware.AuthenticationMiddleware',
    'django.contrib.messages.middleware.MessageMiddleware',
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
]

The protecting middleware was probably commented out during a testing phase, when server-side token generation was not set up. Simply commenting it back in will enable CSRF protection.

References

ruby/ql/src/queries/security/cwe-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by skipping token verification.

class UsersController < ApplicationController
  skip_before_action :verify_authenticity_token
end

Verification can be re-enabled by removing the call to skip_before_action.

Care should be taken when using the Rails protect_from_forgery method to prevent CSRF. The default behaviour of this method is to null the session when an invalid CSRF token is provided. This may not be sufficient to avoid a CSRF vulnerability - for example if parts of the session are memoized. Calling protect_from_forgery with: :exception can help to avoid this by raising an exception on an invalid CSRF token instead.

References

@yoff yoff force-pushed the python/simple-csrf branch from 9c37873 to d39410a Compare March 28, 2022 05:36
@github-actions
Copy link
Contributor

QHelp previews:

python/ql/src/Security/CWE-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by overriding the default middleware stack and not including the one protecting against CSRF.

MIDDLEWARE = [
    'django.middleware.security.SecurityMiddleware',
    'django.contrib.sessions.middleware.SessionMiddleware',
    'django.middleware.common.CommonMiddleware',
    # 'django.middleware.csrf.CsrfViewMiddleware',
    'django.contrib.auth.middleware.AuthenticationMiddleware',
    'django.contrib.messages.middleware.MessageMiddleware',
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
]

The protecting middleware was probably commented out during a testing phase, when server-side token generation was not set up. Simply commenting it back in will enable CSRF protection.

References

ruby/ql/src/queries/security/cwe-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by skipping token verification.

class UsersController < ApplicationController
  skip_before_action :verify_authenticity_token
end

Verification can be re-enabled by removing the call to skip_before_action.

Care should be taken when using the Rails protect_from_forgery method to prevent CSRF. The default behaviour of this method is to null the session when an invalid CSRF token is provided. This may not be sufficient to avoid a CSRF vulnerability - for example if parts of the session are memoized. Calling protect_from_forgery with: :exception can help to avoid this by raising an exception on an invalid CSRF token instead.

References

@yoff
Copy link
Contributor Author

yoff commented Mar 28, 2022

force pushed to skip the TestModule, the last two commits will have new shas.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides an extended comment, and discussion about the value to use for @precision, this all looks good to me 👍

Explain why `TestScope` is not used.

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@github-actions
Copy link
Contributor

QHelp previews:

python/ql/src/Security/CWE-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by overriding the default middleware stack and not including the one protecting against CSRF.

MIDDLEWARE = [
    'django.middleware.security.SecurityMiddleware',
    'django.contrib.sessions.middleware.SessionMiddleware',
    'django.middleware.common.CommonMiddleware',
    # 'django.middleware.csrf.CsrfViewMiddleware',
    'django.contrib.auth.middleware.AuthenticationMiddleware',
    'django.contrib.messages.middleware.MessageMiddleware',
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
]

The protecting middleware was probably commented out during a testing phase, when server-side token generation was not set up. Simply commenting it back in will enable CSRF protection.

References

ruby/ql/src/queries/security/cwe-352/CSRFProtectionDisabled.qhelp

CSRF protection weakened or disabled

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

A common countermeasure for CSRF is to generate a unique token to be included in the HTML sent from the server to a user. This token can be used as a hidden field to be sent back with requests to the server, where the server can then check that the token is valid and associated with the relevant user session.

Recommendation

In many web frameworks, CSRF protection is enabled by default. In these cases, using the default configuration is sufficient to guard against most CSRF attacks.

Example

The following example shows a case where CSRF protection is disabled by skipping token verification.

class UsersController < ApplicationController
  skip_before_action :verify_authenticity_token
end

Verification can be re-enabled by removing the call to skip_before_action.

Care should be taken when using the Rails protect_from_forgery method to prevent CSRF. The default behaviour of this method is to null the session when an invalid CSRF token is provided. This may not be sufficient to avoid a CSRF vulnerability - for example if parts of the session are memoized. Calling protect_from_forgery with: :exception can help to avoid this by raising an exception on an invalid CSRF token instead.

References

RasmusWL
RasmusWL previously approved these changes May 10, 2022
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the merge conflict, this LGTM 👍

RasmusWL
RasmusWL previously approved these changes May 10, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 2 vulnerabilities.

Comment on lines 570 to 575
if p.csrfEnabled()
then tag = "CsrfLocalProtectionEnabled"
else tag = "CsrfLocalProtectionDisabled"

class XmlParsingTest extends InlineExpectationsTest {
XmlParsingTest() { this = "XmlParsingTest" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bit is not going to work after the merge 😳 my approval was just a second too fast 😂

caused by an optimistic attempt at solving a
merge conflict in the online GUI.
@RasmusWL RasmusWL merged commit 2b6e0cf into github:main May 10, 2022
@yoff yoff deleted the python/simple-csrf branch May 10, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants