Code

Opened 3 years ago

Closed 23 months ago

Last modified 23 months ago

#17040 closed Bug (fixed)

In utils.crypto.constant_time_compare only call ord on non ints.

Reported by: adsworth Owned by: adsworth
Component: Python 3 Version: 1.3
Severity: Normal Keywords:
Cc: adsworth Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Iterating over a byte string in Python 3 will yield ints. Make sure that ord is only called on non ints.

Attachments (2)

utils-crypto_constant_time_compare.patch (554 bytes) - added by adsworth 3 years ago.
fix-1740-separate-function-for-py3.patch (1.5 KB) - added by adsworth 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by adsworth

comment:1 Changed 3 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Accepted. But doing an isinstance check for each element seems like a pretty inefficient approach. The function is short enough that we are probably better of conditionally defining a different function for Python 3.

comment:2 Changed 3 years ago by Alex

Should this function take bytes or unicode strings?

comment:3 Changed 3 years ago by adsworth

In a py3 version we could call smart_str on the input then we are sure to have a byte string.
If we want the function to take unicode, then we don't need a new function only a check if the input is a unicode string. Iterating unicode strings in py3 yields chars.

I'll write up a patch with a separate py3 function.

Changed 3 years ago by adsworth

comment:4 Changed 3 years ago by adsworth

  • Patch needs improvement unset

new patch with a separate function for py3. The new function also calls smart_str on the input parameters to make certain they are byte strings.

I'm removing the "Patch needs improvement" flag. I hope that is OK.

comment:5 Changed 3 years ago by Alex

Calling smart_str doesn't seem correct to me. ISTM that this function should either take a bytestring or a unicode string (and I'm not sure which) and any other input should be an error. Also, I'm not sure you can write a constant time comparison of unicode strings in py3k.

comment:6 Changed 3 years ago by adsworth

I am no crypto expert, but since constant_time_compare in django and in the contrib apps is only used to compare hashes or some sort of token. I'd think it save to assume byte strings is the "right thing".

Since a unicode string can use one or two bytes per char depending on the contents I think you are right about a constant time compare not being possible.

also calling ord() on a unicode char that is 2 bytes long results in a TypeError.

>>> ord('ழ்')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ord() expected a character, but string of length 2 found
>>> 
Last edited 3 years ago by adsworth (previous) (diff)

comment:7 Changed 23 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In [62954ba04c86c4cea3b17a872ba6a0837730e17d]:

[py3] Fixed #17040 -- ported django.utils.crypto.constant_time_compare.

This is a private API; adding a type check is acceptable.

comment:8 Changed 23 months ago by Aymeric Augustin <aymeric.augustin@…>

In [e89bc39935afc5096e6a51a49874b2d30cbc2b5e]:

Reverted type check added in 62954ba04c.

Refs #17040.

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.