Opened 10 years ago

Closed 10 years ago

#16827 closed Bug (fixed)

validate CSRF token (Check length)

Reported by: jedie Owned by: Raúl Cumplido
Component: 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 Zbigniew Siciarz 10 years ago.
Provided test for the token length.
patch_16827_2.diff (2.7 KB) - added by Zbigniew Siciarz 10 years ago.
Replaced the magic number with a constant.

Download all attachments as: .zip

Change History (8)

comment:2 Changed 10 years ago by tinodb

Component: Uncategorizedcontrib.csrf
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 10 years ago by Raúl Cumplido

Owner: changed from nobody to Raúl Cumplido
Status: newassigned

I will provide the tests to the pull request.

Changed 10 years ago by Zbigniew Siciarz

Attachment: patch_16827.diff added

Provided test for the token length.

Changed 10 years ago by Zbigniew Siciarz

Attachment: patch_16827_2.diff added

Replaced the magic number with a constant.

comment:4 Changed 10 years ago by Tomek Paczkowski

Triage Stage: AcceptedReady for checkin

Looks good.

comment:6 Changed 10 years ago by Ashutosh 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 10 years ago by Paul McMillan

Resolution: fixed
Status: assignedclosed

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 10 years ago by Paul McMillan (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top