Description

assignee = 'https://github.com/tiran'
closed_at = <Date 2020-11-22.17:34:28.761>
created_at = <Date 2020-05-27.07:41:07.020>
labels = ['type-security', '3.8', '3.9', '3.10', '3.7', 'library']
title = 'hmac.compare_digest could try harder to be constant-time.'
updated_at = <Date 2020-12-14.17:11:27.035>
user = 'https://github.com/ssbr'

activity = <Date 2020-12-14.17:11:27.035>
actor = 'ned.deily'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2020-11-22.17:34:28.761>
closer = 'benjamin.peterson'
components = ['Library (Lib)']
creation = <Date 2020-05-27.07:41:07.020>
creator = 'Devin Jeanpierre'
dependencies = []
files = []
hgrepos = []
issue_num = 40791
keywords = ['patch']
message_count = 15.0
messages = ['370053', '370094', '370108', '370109', '370121', '370122', '370124', '370195', '381530', '381531', '381533', '381624', '382972', '382993', '382995']
nosy_count = 8.0
nosy_names = ['rhettinger', 'gregory.p.smith', 'christian.heimes', 'benjamin.peterson', 'Devin Jeanpierre', 'ned.deily', 'mgorny', 'miss-islington']
pr_nums = ['20444', '20456', '20461', '23436', '23437', '23438', '23767']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue40791'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

Activity

1. ssbr commented on May 27, 2020

   

   ssbrmannequin

   on May 27, 2020

   MannequinAuthor

   More actions

   hmac.compare_digest (via _tscmp) does not mark the accumulator variable result as volatile, which means that the compiler is allowed to short-circuit the comparison loop as long as it still reads from both strings.

   In particular, when result is non-volatile, the compiler is allowed to change the loop from this:

   for (i=0; i < length; i++) {
       result |= *left++ ^ *right++;
   }
   return (result == 0);

   into (the moral equivalent of) this:

   for (i=0; i < length; i++) {
       result |= *left++ ^ *right++;
       if (result) {
           for (; ++i < length;) {
               *left++; *right++;
           }
           return 1;
       }
   }
   return (result == 0);

   (Code not tested.)

   This might not seem like much, but it cuts out almost all of the data dependencies between result, left, and right, which in theory would free the CPU to race ahead using out of order execution -- it could execute code that depends on the result of _tscmp, even while _tscmp is still performing the volatile reads. (I have not actually benchmarked this. :)) In other words, this weird short circuiting could still actually improve performance. That, in turn, means that it would break constant-time guarantees.

   (This is different from saying that it _would_ increase performance, but marking it volatile removes the worry.)

   (Prior art/discussion: tink-crypto/tink@335291c )

   I propose two changes, one trivial, and one that's more invasive:

   1. Make result a volatile unsigned char instead of unsigned char.

   2. When SSL is available, instead use CRYPTO_memcmp from OpenSSL/BoringSSL. We are, in effect, "rolling our own crypto". The SSL libraries are more strictly audited for timing issues, down to actually checking the generated machine code. As tools improve, those libraries will grow to use those tools. If we use their functions, we get the benefit of those audits and improvements.
2. ssbr

   added

   3.10 (EOL)end of lifeend of life

   3.7 (EOL)end of lifeend of life

   3.8 (EOL)end of lifeend of life

   3.9 (EOL)end of lifeend of life

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   on May 27, 2020
3. rhettinger commented on May 27, 2020

   

   rhettinger

   on May 27, 2020

   Contributor

   More actions

   +1 for both of these suggestions
4. rhettinger

   added

   type-securityA security issueA security issue

   on May 27, 2020
5. gpshead commented on May 27, 2020

   

   gpshead

   on May 27, 2020

   Member

   More actions

   Christian - Devin could likely use some help with the build/ifdef plumbing required for (2) to use CRYPTO_memcmp from Modules/_operator.c when OpenSSL is available.
6. 5 remaining items

7. tiran commented on May 27, 2020

   

   tiran

   on May 27, 2020

   Member

   More actions

   New changeset db5aed9 by Christian Heimes in branch 'master':
   bpo-40791: Use CRYPTO_memcmp() for compare_digest (bpo-20456)
   db5aed9
8. miss-islington commented on May 28, 2020

   

   miss-islington

   on May 28, 2020

   Contributor

   More actions

   New changeset 8183e11 by Christian Heimes in branch '3.9':
   [3.9] bpo-40791: Use CRYPTO_memcmp() for compare_digest (GH-20456) (GH-20461)
   8183e11
9. gpshead commented on Nov 21, 2020

   

   gpshead

   on Nov 21, 2020

   Member

   More actions

   New changeset 3172936 by Devin Jeanpierre in branch 'master':
   bpo-40791: Make compare_digest more constant-time. (GH-20444)
   3172936
10. miss-islington commented on Nov 21, 2020

    

    miss-islington

    on Nov 21, 2020

    Contributor

    More actions

    New changeset 97136d7 by Miss Islington (bot) in branch '3.8':
    bpo-40791: Make compare_digest more constant-time. (GH-20444)
    97136d7
11. miss-islington commented on Nov 21, 2020

    

    miss-islington

    on Nov 21, 2020

    Contributor

    More actions

    New changeset c1bbca5 by Miss Islington (bot) in branch '3.9':
    bpo-40791: Make compare_digest more constant-time. (GH-20444)
    c1bbca5
12. benjaminp commented on Nov 22, 2020

    

    benjaminp

    on Nov 22, 2020

    Contributor

    More actions

    New changeset db95802 by Miss Islington (bot) in branch '3.7':
    bpo-40791: Make compare_digest more constant-time. (GH-23438)
    db95802
13. benjaminp

    closed this as completedon Nov 22, 2020
14. benjaminp

    closed this as completedon Nov 22, 2020
15. mgorny commented on Dec 14, 2020

    

    mgornymannequin

    on Dec 14, 2020

    Mannequin

    More actions

    Any reason this wasn't backported to 3.6? FWICS it's supposed to be security supported still.
16. ned-deily commented on Dec 14, 2020

    

    ned-deily

    on Dec 14, 2020

    Member

    More actions

    New changeset 8bef9eb by Miss Islington (bot) in branch '3.6':
    bpo-40791: Make compare_digest more constant-time. (GH-23438) (GH-23767)
    8bef9eb
17. ned-deily commented on Dec 14, 2020

    

    ned-deily

    on Dec 14, 2020

    Member

    More actions

    Any reason this wasn't backported to 3.6?

    Just an oversight. Thanks for pointing it out.
18. ezio-melotti

    transferred this issue fromon Apr 10, 2022
19. samueloph commented on Aug 24, 2023

    

    samueloph

    on Aug 24, 2023

    More actions

    CVE-2022-48566 was assigned to this.

    I wasn't involved in the assignment, posting here for reference only.

    Reacted by Seth Larson, Charlie Zhao and neilpvirtualitics

    Reacted by Gregory P. Smith and neilpvirtualitics
20. neilpvirtualitics commented on Oct 19, 2023

    

    neilpvirtualitics

    on Oct 19, 2023

    More actions

    Also seems to be this: GHSA-cgfh-jp5w-8cmx
    Which really has no info.
21. ssbr

    added a commit that references this issue on Jul 14, 2026

    Make CRYPTO_memcmp less de-constant-time-able via optimization

    6ef4572

Issue actions