Opened 8 years ago

Closed 8 years ago

#26628 closed Cleanup/optimization (fixed)

Log CSRF failures to django.security instead of django.request

Reported by: Jacob Kaplan-Moss Owned by: Holly Becker
Component: CSRF Version: 1.9
Severity: Normal Keywords: csrf security
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

It's currently possible to log CSRF exceptions with the 403 handler. But a more sensible default would be to log them by default. This would be an easy simple step that would make Django play more nicely with a SIEM

[One of a series of bugs from a discussion I had with @mallyvai about improving the security of Django's admin - see https://gist.github.com/mallyvai/bcb0bb827d6d53212879dff23cf15d03 for the full list.]

Change History (14)

comment:1 by Tim Graham, 8 years ago

The CsrfViewMiddleware does have logging for rejected requests. Is this insufficient? I couldn't find this mentioned in the documentation so that could be added, at least.

comment:2 by Tim Graham, 8 years ago

Resolution: worksforme
Status: newclosed

comment:3 by Vaibhav Mallya, 8 years ago

Hi Tim,

Definitely think it's a good idea to add to the docs.

My original thinking was that it would be a more sensible default to log this as a security exception and use the django.security loggers (i'm not 100% sure about how that's wired up), since CSRFs are meant to be an explicit security measure. It seems to be logged as a generic logger.warning right now.

Version 0, edited 8 years ago by Vaibhav Mallya (next)

comment:4 by Tim Graham, 8 years ago

Component: CSRFDocumentation
Resolution: worksforme
Status: closednew
Summary: CSRF violations should be loggedDocument that CSRF violations are logged to the django.request logger
Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

comment:5 by Holly Becker, 8 years ago

Easy pickings: set
Needs documentation: set
Owner: changed from nobody to Holly Becker
Status: newassigned

comment:6 by Carl Meyer, 8 years ago

Summary: Document that CSRF violations are logged to the django.request loggerLog CSRF failures to django.security by default.

Sergio Campos (seocam) and I were just discussing this same issue at the PyCon sprint (in the context of looking at the broader context in #26688). I'm inclined to agree with mallyvai that this isn't just a doc bug -- we should be logging CSRF failures to django.security by default. Updating the ticket title accordingly -- please let me know if you don't agree, or think this should be discussed on the mailing list.

comment:7 by Tim Graham, 8 years ago

Are you concerned at all about backwards-compatibility in the case of some custom logging configuration?

Last edited 8 years ago by Tim Graham (previous) (diff)

comment:8 by Carl Meyer, 8 years ago

I guess I'm not too concerned about back-compat, because it seems it would only be an issue in a case where someone has configured logging to watch django.request but ignore django.security, which seems like a pretty strange choice. But I also don't feel strongly. I'd be inclined to merge the doc fix that Hwesta has already submitted (https://github.com/django/django/pull/6694/files) but as a Refs, and leave this open for further discussion of the actual logger change.

comment:9 by Tim Graham <timograham@…>, 8 years ago

In ff9198e:

Refs #26628 -- Documented CSRF failure logging.

comment:10 by Tim Graham <timograham@…>, 8 years ago

In cbc8ef61:

[1.9.x] Refs #26628 -- Documented CSRF failure logging.

Backport of ff9198ee0f1de24a5b2861d28849344e7a5714c4 from master

comment:11 by Tim Graham <timograham@…>, 8 years ago

In 697ed75:

[1.10.x] Refs #26628 -- Documented CSRF failure logging.

Backport of ff9198ee0f1de24a5b2861d28849344e7a5714c4 from master

comment:12 by Tim Graham, 8 years ago

Component: DocumentationCSRF
Has patch: set
Needs documentation: unset
Patch needs improvement: set
Summary: Log CSRF failures to django.security by default.Log CSRF failures to django.security instead of django.request

PR with comments for improvement.

comment:13 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:14 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 55fec16a:

Fixed #26628 -- Changed CSRF logger to django.security.csrf.

Note: See TracTickets for help on using tickets.
Back to Top