Opened 5 years ago

Closed 4 years ago

Last modified 4 years 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 5 years ago.
fix-1740-separate-function-for-py3.patch (1.5 KB) - added by adsworth 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by adsworth

comment:1 Changed 5 years ago by Luke Plant

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 5 years ago by Alex Gaynor

Should this function take bytes or unicode strings?

comment:3 Changed 5 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 5 years ago by adsworth

comment:4 Changed 5 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 5 years ago by Alex Gaynor

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 5 years ago by adsworth

I am by no crypto expert, but since constant_time_compare in the django and the contrib apps is only used to compare hashes or some sort of tokens 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
>>> 
Version 0, edited 5 years ago by adsworth (next)

comment:7 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: newclosed

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 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In [e89bc39935afc5096e6a51a49874b2d30cbc2b5e]:

Reverted type check added in 62954ba04c.

Refs #17040.

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