Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conch MAC verification should use cryptography #8199

Closed
twisted-trac opened this issue Feb 6, 2016 · 8 comments
Closed

Conch MAC verification should use cryptography #8199

twisted-trac opened this issue Feb 6, 2016 · 8 comments

Comments

@twisted-trac
Copy link

twisted-trac commented Feb 6, 2016

mithrandi's avatar @mithrandi reported
Trac ID trac#8199
Type defect
Created 2016-02-06 17:30:20Z

Currently verification is done using ==; this is bad because it is not a constant-time comparison. Cryptography's HMAC objects have a verify() method that does the right thing, as well as a general-purpose constant-time comparison, so now that we use Cryptography for the rest of our crypto, we should just use these.

Searchable metadata
trac-id__8199 8199
type__defect defect
reporter__mithrandi mithrandi
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__conch conch
keywords__None None
time__1454779820235485 1454779820235485
changetime__1626936906376695 1626936906376695
version__None None
owner__glyph glyph
cc__z3p
@twisted-trac
Copy link
Author

twisted-trac commented Mar 21, 2019

wiml's avatar @wiml commented

This seems like a good idea; on Python3.6+, we could also use secrets.compare_digest().

@twisted-trac
Copy link
Author

twisted-trac commented Apr 5, 2021

wiml's avatar @wiml commented

PR here, using hmac.compare_digest() since we're using the hmac digest for digesting anyway: #1580

@twisted-trac
Copy link
Author

twisted-trac commented Apr 5, 2021

wiml's avatar @wiml set owner to @wiml
@wiml set status to assigned

Taking this out of review until I write up a unit test that covers the untested part of the change

@twisted-trac
Copy link
Author

twisted-trac commented Apr 6, 2021

adiroiban's avatar @adiroiban commented

Maybe related #4536

@twisted-trac
Copy link
Author

twisted-trac commented Apr 6, 2021

wiml's avatar @wiml removed owner
@wiml set status to new

@twisted-trac
Copy link
Author

twisted-trac commented Apr 11, 2021

wiml's avatar @wiml commented

Replying to Adi Roiban:

Yes I think this is a subset of [#4536](#4536) (as is another open PR, for ticket #10086, which is disjoint from this one). I think a lot of the concerns in #4536 disappear with the availability of compare_digest() in python's stdlib.

The PR I submitted for this ticket just covers the couple cases in conch, which are I think more straightforward to resolve than a few of the other sensitive comparisons in Twisted. Hopefully this can chip away at the variety of comparisons mentioned by #4536 until that ticket is easier to resolve.

@twisted-trac
Copy link
Author

twisted-trac commented Jul 22, 2021

glyph's avatar @glyph set owner to @glyph

Thanks for making such a nice bite-sized change. I'll approve and land it!

@twisted-trac
Copy link
Author

twisted-trac commented Jul 22, 2021

glyph's avatar @glyph set status to closed

In changeset 65f382f

#!CommitTicketReference repository="" revision="65f382f43dfe9bec9d25d99ce0cb05db115f5fac"
Merge pull request #1580 from wiml/8199-conch-constant-time-compare

Author: wiml
Reviewer: glyph
Fixes: ticket:8199
Refs: ticket:4536

Use constant-time compare_digest() for MACs and similar secret data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants