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

is CSRF implemented out of the box? #108

Closed
IbrahimOuhamou opened this issue Nov 3, 2024 · 10 comments
Closed

is CSRF implemented out of the box? #108

IbrahimOuhamou opened this issue Nov 3, 2024 · 10 comments

Comments

@IbrahimOuhamou
Copy link
Contributor

I don't know how it should be implemented, but I guess we can do a try session.put("csrf", csrf) on session init, or it can be implemented by the user

@bobf
Copy link
Contributor

bobf commented Nov 3, 2024

It's no provided yet but it on my TODO list to provide automatic anti-CSRF features - we can use this issue to track progess.

@IbrahimOuhamou
Copy link
Contributor Author

@bobf how are you planning to implement it?

  1. how to generate the token?
  2. who should insert the token in the form index.zmpl or in the header
  3. who should check for the token recieved

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html

@IbrahimOuhamou
Copy link
Contributor Author

@bobf I want to make a PR adding an auto generated csrf_token to session on init or reset

bobf added a commit that referenced this issue Nov 21, 2024
Add to middleware in app's `src/main.zig`:

```zig
pub const jetzig_options = struct {
    pub const middleware: []const type = &.{
        jetzig.middleware.AntiCsrfMiddleware,
    };
};
```

CSRF token available in Zmpl templates:

```
{{context.authenticityToken()}}
```
or render a hidden form element:
```
{{context.authenticityFormElement()}}
```

The following HTML requests are rejected (403 Forbidden) if the
submitted query param does not match the value stored in the encrypted
session (added automatically when the token is generated for a template
value):

* POST
* PUT
* PATCH
* DELETE

JSON requests are not impacted - users should either disable JSON
endpoints or implement a different authentication method to protect
them.
@bobf
Copy link
Contributor

bobf commented Nov 21, 2024

@IbrahimOuhamou See this PR: #120

Example usage here: https://github.com/jetzig-framework/jetzig/pull/120/files#diff-f8a5de4287a4ded16bbe3960bb42dce20868f59ee482a7665a914e1608d81da6R2

I need to do a bit of work to figure out how to test this functionality and I need to review it so I won't be merging tonight, but it should be ready for use by the weekend.

@IbrahimOuhamou
Copy link
Contributor Author

I see that you used the session for the token, why didn't you stick to the cookie? wouldn't it be faster than session decryption?

@IbrahimOuhamou
Copy link
Contributor Author

@bobf when you are done tell me so I can close the issue (I got notification that for the pr)

@bobf
Copy link
Contributor

bobf commented Nov 23, 2024

@IbrahimOuhamou No problem. I've decided to go back to using the session for these reasons:

  • We guarantee that the token was generated by the server (only the server knows the encryption key, so if we find a token in the session we know that we generated it unless the encryption key was compromised)
  • By linking the token to the session, the token in the HTML cannot be re-used for another user (i.e. an attacker cannot generate a token in one context and then inject it into another user's context if they have exploited an attack vector on another user's machine)

I am not an expert in this stuff so if you have any feedback on this I would be happy to hear it.

bobf added a commit that referenced this issue Nov 23, 2024
Add to middleware in app's `src/main.zig`:

```zig
pub const jetzig_options = struct {
    pub const middleware: []const type = &.{
        jetzig.middleware.AntiCsrfMiddleware,
    };
};
```

CSRF token available in Zmpl templates:

```
{{context.authenticityToken()}}
```
or render a hidden form element:
```
{{context.authenticityFormElement()}}
```

The following HTML requests are rejected (403 Forbidden) if the
submitted query param does not match the value stored in the encrypted
session (added automatically when the token is generated for a template
value):

* POST
* PUT
* PATCH
* DELETE

JSON requests are not impacted - users should either disable JSON
endpoints or implement a different authentication method to protect
them.
bobf added a commit that referenced this issue Nov 23, 2024
Add to middleware in app's `src/main.zig`:

```zig
pub const jetzig_options = struct {
    pub const middleware: []const type = &.{
        jetzig.middleware.AntiCsrfMiddleware,
    };
};
```

CSRF token available in Zmpl templates:

```
{{context.authenticityToken()}}
```
or render a hidden form element:
```
{{context.authenticityFormElement()}}
```

The following HTML requests are rejected (403 Forbidden) if the
submitted query param does not match the value stored in the encrypted
session (added automatically when the token is generated for a template
value):

* POST
* PUT
* PATCH
* DELETE

JSON requests are not impacted - users should either disable JSON
endpoints or implement a different authentication method to protect
them.
bobf added a commit that referenced this issue Nov 23, 2024
Add to middleware in app's `src/main.zig`:

```zig
pub const jetzig_options = struct {
    pub const middleware: []const type = &.{
        jetzig.middleware.AntiCsrfMiddleware,
    };
};
```

CSRF token available in Zmpl templates:

```
{{context.authenticityToken()}}
```
or render a hidden form element:
```
{{context.authenticityFormElement()}}
```

The following HTML requests are rejected (403 Forbidden) if the
submitted query param does not match the value stored in the encrypted
session (added automatically when the token is generated for a template
value):

* POST
* PUT
* PATCH
* DELETE

JSON requests are not impacted - users should either disable JSON
endpoints or implement a different authentication method to protect
them.
@IbrahimOuhamou
Copy link
Contributor Author

@bobf I think django uses cookies and I that it would be faster

@bobf bobf closed this as completed in 6e6f1be Nov 23, 2024
bobf added a commit that referenced this issue Nov 23, 2024
@bobf
Copy link
Contributor

bobf commented Nov 23, 2024

@IbrahimOuhamou Django uses cookies but it also cryptographically signs the token and verifies its authenticity, so it is not as simple as comparing the bytes in the token.

Ref here:
https://docs.djangoproject.com/en/5.1/ref/csrf/#how-it-works
https://github.com/django/django/blob/main/django/core/checks/security/csrf.py#L60
https://github.com/django/django/blob/main/django/core/signing.py#L199

By using the session we achieve same result. We can look at optimising later by storing a separate, signed cookie and using a faster algorithm than the session encryption, but I think that the performance would be quite negligible. I will do some benchmarking to compare once I find time to do that optimisation.

Also bear in mind that the performance hit will only impact pages that call context.authenticityToken() (or context.authenticityFormField()) in a template - we do not generate the token otherwise, and we only expect the token on POST, PATCH, PUT, DELETE.

@bobf
Copy link
Contributor

bobf commented Nov 23, 2024

I have merged the PR - let me know if you hit any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants