Opened 13 years ago

Closed 12 years ago

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

Download all attachments as: .zip

Change History (10)

comment:1 by Luke Plant, 12 years ago

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

Should this function take bytes or unicode strings?

comment:3 by adsworth, 12 years ago

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.

comment:4 by adsworth, 12 years ago

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

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

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 12 years ago by adsworth (next)

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

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

In [e89bc39935afc5096e6a51a49874b2d30cbc2b5e]:

Reverted type check added in 62954ba04c.

Refs #17040.

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