[Django] #36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------
Reporter: Sage Abdullah | Type: Bug
Status: new | Component: Utilities
Version: dev | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
The deprecation of `constant_time_compare` in #36546
(0246f478882c26bc1fe293224653074cd46a90d0) removed the `force_bytes`
conversion of the arguments passed to the function. The function now
raises an error if passed arguments of different types, e.g. `bytes` and
`str`. Test:


{{{#!diff
diff --git a/tests/utils_tests/test_crypto.py
b/tests/utils_tests/test_crypto.py
index bbedb3080d..4ed8167150 100644
--- a/tests/utils_tests/test_crypto.py
+++ b/tests/utils_tests/test_crypto.py
@@ -21,6 +21,8 @@ class TestUtilsCryptoMisc(SimpleTestCase):
self.assertFalse(constant_time_compare(b"spam", b"eggs"))
self.assertTrue(constant_time_compare("spam", "spam"))
self.assertFalse(constant_time_compare("spam", "eggs"))
+ self.assertTrue(constant_time_compare(b"spam", "spam"))
+ self.assertFalse(constant_time_compare("spam", b"eggs"))

def test_constant_time_compare_deprecated(self):
msg = (
}}}

The fix on my side is trivial (ensure both arguments are the same type),
but I'm not sure if this was intentional for the deprecation process. If
it were intentional, I'm happy to close this as a wontfix. Otherwise, I'm
also happy to send a PR that adds the `force_bytes` back in.
--
Ticket URL: <https://code.djangoproject.com/ticket/36572>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------

Reporter: Sage Abdullah | Owner: (none)

Type: Bug | Status: new
Component: Utilities | Version: dev

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Sarah Boyce):

This was mentioned in the ticket description #36546

> `constant_time_compare()` does call `force_bytes()` on its arguments but
this was a workaround for Python 2.7
(7e3cf3cfd27e53ced0a1fc65a02849f78a292d3d) and no tests in Django's test
suite fail with those calls removed.

Is this something you spotted and made you concerned or has this broken
something on a project?
--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:1>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------
Reporter: Sage Abdullah | Owner: (none)
Type: Bug | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Changes (by Sarah Boyce):

* cc: Tim Graham (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:2>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------
Reporter: Sage Abdullah | Owner: (none)
Type: Bug | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Sage Abdullah):

> Is this something you spotted and made you concerned or has this broken
something on a project?

I spotted this in Wagtail (ref:
https://github.com/wagtail/wagtail/pull/13363) because apparently our use
of the function involves passing mixed-type arguments to the function,
which likely was unintentional.

However, I can see another issue with leaving out `force_bytes`: the `str`
vs. `str` use of `hmac.compare_digest()`
[https://docs.python.org/3/library/hmac.html#hmac.compare_digest only
works with ASCII characters]. This means the following test now also
errors:

{{{#!diff
diff --git a/tests/utils_tests/test_crypto.py
b/tests/utils_tests/test_crypto.py

index bbedb3080d..e1795c766e 100644
--- a/tests/utils_tests/test_crypto.py
+++ b/tests/utils_tests/test_crypto.py
@@ -21,6 +21,10 @@ class TestUtilsCryptoMisc(SimpleTestCase):

self.assertFalse(constant_time_compare(b"spam", b"eggs"))
self.assertTrue(constant_time_compare("spam", "spam"))
self.assertFalse(constant_time_compare("spam", "eggs"))
+ self.assertTrue(constant_time_compare(b"spam", "spam"))
+ self.assertFalse(constant_time_compare("spam", b"eggs"))

+ self.assertTrue(constant_time_compare("ありがとう", "ありがとう
"))
+ self.assertFalse(constant_time_compare("ありがとう", "おはよう"))

def test_constant_time_compare_deprecated(self):
msg = (
}}}

If developers use the utility function to e.g. compare passwords, and they
just pass plain strings to the function, it no longer works if the string
contains non-ASCII characters.
--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:3>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------
Reporter: Sage Abdullah | Owner: (none)
Type: Bug | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Tim Graham):

I hadn't noticed that the original patch changed the implementation of
`constant_time_compare()` (removing `force_bytes()`). With this new
information, that's certainly inappropriate.

I suppose there's a larger question of whether we keep the
`constant_time_compare()` API since we now learned it provides genuinely
different functionality that third-party code is relying on (comparing
strings with non-ASCII characters and arguments of different type) than
`hmac.compare_digest()`. In argument against keeping it, it forces third-
parties to identify these areas and add their own casting rather than
imposing a casting penalty where it's unneeded.

As far as I see, Django itself doesn't need to use
`constant_time_compare()` internally since in always compares digests.
Well, `hmac.compare_digest(request.session.get(HASH_SESSION_KEY, ""),
session_auth_hash)` looks like the empty string needs to become a
bytestring.

At this point in the release cycle, probably it's best to revert the
deprecation to consider this more carefully. A later first step could be
to replace internal usage of `constant_time_compare()` with
`compare_digest()` (part of the original patch, but checked more carefully
to make sure a str/bytes comparison can't happen).
--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:4>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------+--------------------------------------
Reporter: Sage Abdullah | Owner: (none)
Type: Bug | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Changes (by Jake Howard):

* cc: Jake Howard (added)

Comment:

I agree with reverting #36546 and reviewing each case more closely. An
alternative would be to explicitly state using `force_bytes` in the
deprecation message, but that ends up quite verbose)

Given the complexities around using `compare_digest` with the correct
types, I'd be in favour of keeping `constant_time_compare` around to hide
those from users, even if we slowly port Django's internal uses to
`compare_digest` separately.
--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:5>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.

---------------------------------+---------------------------------------
Reporter: Sage Abdullah | Owner: Sarah Boyce
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

---------------------------------+---------------------------------------
Changes (by Sarah Boyce):

* has_patch: 0 => 1
* owner: (none) => Sarah Boyce
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:6>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
---------------------------------+---------------------------------------
Reporter: Sage Abdullah | Owner: Sarah Boyce
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------

Changes (by JaeHyuckSa):

* cc: JaeHyuckSa (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:7>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.

-------------------------------------+-------------------------------------

Reporter: Sage Abdullah | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: Utilities | Version: dev
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:8>

[Django]

#36572: Deprecation of constant_time_compare broke usage with mixed-type arguments.
-------------------------------------+-------------------------------------
Reporter: Sage Abdullah | Owner: Sarah
| Boyce

Type: Bug | Status: closed
Component: Utilities | Version: dev
Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"d0e4dd5cdd743a5c43c4ccc2c8fa29d3982eaa71" d0e4dd5c]:
{{{#!CommitTicketReference repository=""
revision="d0e4dd5cdd743a5c43c4ccc2c8fa29d3982eaa71"
Fixed #36572 -- Revert "Fixed #36546 -- Deprecated
django.utils.crypto.constant_time_compare() in favor of
hmac.compare_digest()."

This reverts commit 0246f478882c26bc1fe293224653074cd46a90d0.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36572#comment:9>