Code

Opened 3 years ago

Closed 2 years ago

#16827 closed Bug (fixed)

validate CSRF token (Check length)

Reported by: jedie Owned by: raulcd
Component: contrib.csrf Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I wonder that the CSRF token send from the client didn't be validated.

Don't know if a DOS attack is possible by sending many request with very long CSRF tokens?

IMHO it's a good idea to check the length before do anything with it.

See also: https://groups.google.com/group/django-developers/browse_thread/thread/9fc008d2a3735bc2

Attachments (2)

patch_16827.diff (1.9 KB) - added by zsiciarz 2 years ago.
Provided test for the token length.
patch_16827_2.diff (2.7 KB) - added by zsiciarz 2 years ago.
Replaced the magic number with a constant.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by jedie

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by tinodb

  • Component changed from Uncategorized to contrib.csrf
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by raulcd

  • Owner changed from nobody to raulcd
  • Status changed from new to assigned

I will provide the tests to the pull request.

Changed 2 years ago by zsiciarz

Provided test for the token length.

Changed 2 years ago by zsiciarz

Replaced the magic number with a constant.

comment:4 Changed 2 years ago by oinopion

  • Triage Stage changed from Accepted to Ready for checkin

Looks good.

comment:6 Changed 2 years ago by aashu_dwivedi

from the source code i noticed that the method

django.utils.crypto.constant_time_compare

is being used for comparing the request token from the one already present in the cookie, since the comparison is already constant time is the length checking really required ?

comment:7 Changed 2 years ago by PaulM

  • Resolution set to fixed
  • Status changed from assigned to closed

aashu_dwivedi: The length check is worth fixing. It is true that the constant_time_compare will fail early if the lengths do not match, but the performance concern is in the regex applied to sanitize the string before it gets to constant_time_compare.

In [17500]:

Fixes #16827. Adds a length check to CSRF tokens before applying the santizing regex. Thanks to jedie for the report and zsiciarz for the initial patch.

Last edited 2 years ago by PaulM (previous) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.