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

Expose allow_truncate option in SigningKey.sign() and VerifyingKey.verify() #205

Merged
merged 5 commits into from
Aug 24, 2020

Conversation

blag
Copy link
Contributor

@blag blag commented Jul 29, 2020

PR #168 added the allow_truncate option to SigningKey.sign_digest() and VerifyingKey.verify_digest(), and then forcibly defaulted their value to True in SigningKey.sign() and VerifyingKey.verify():

class VerifyingKey(object):
    ...
    def verify(self, signature, data, hashfunc=None,
               sigdecode=sigdecode_string):
        ...
        return self.verify_digest(signature, digest, sigdecode, True)
class SigningKey(object):
    def sign(self, data, entropy=None, hashfunc=None,
             sigencode=sigencode_string, k=None):
        return self.sign_digest(h, entropy, sigencode, k, allow_truncate=True)

Unfortunately, this didn't actually satisfy the request in #129 (emphasis mine):

Add an option to verify() and sign() methods to change if they will accept bigger inputs and appropriately truncate them.

Note that while #168 added the allow_truncate option for VerifyingKey.verify_digest() and SigningKey.sign_digest(), the option is still not exposed in the signatures for VerifyingKey.verify() SigningKey.sign().

Furthermore, due to the default behavior changing, ecdsa version 0.15 breaks tests for downstream projects: mpdavis/python-jose#176, which tested for the old behavior (eg: allow_truncate=False).

This PR routes the allow_truncate option up to VerifyingKey.verify_digest() and SigningKey.sign_digest(), and more importantly for you, keeps the default value to True. This is to avoid possibly re-disrupting ecdsa users but also allows python-jose and other users to opt-in to the previous behavior.

I also added tests for the new options.

@tomato42
Copy link
Member

what's the use case (or usage scenario) for not allowing verification of large hashes with small curves?

(I'm not totally opposed to restoring the pre-0.15 behaviour, but I don't think I understand it well enough to decide one way or the other)

src/ecdsa/test_pyecdsa.py Outdated Show resolved Hide resolved
src/ecdsa/test_pyecdsa.py Outdated Show resolved Hide resolved
@blag blag force-pushed the expose-allow-truncate branch from 362f33c to 4ffeed6 Compare July 29, 2020 19:06
@coveralls
Copy link

coveralls commented Jul 29, 2020

Coverage Status

Coverage increased (+0.007%) to 98.008% when pulling 4f7d534 on blag:expose-allow-truncate into 14e673d on warner:master.

@tomato42
Copy link
Member

the pypy failure is caused by #206 so I've rescheduled it, but the codechecks suggests bad formatting for code, please run black on it

@tomato42
Copy link
Member

that being said, my questions from #205 (comment) still stand

@blag
Copy link
Contributor Author

blag commented Jul 29, 2020

Thank you. And yep, will do. I'm juggling a lot of things today, but I'll circle back around to this, fix the formatting issues identified by black, and discuss my usecase.

@tomato42
Copy link
Member

tomato42 commented Aug 5, 2020

ping? I'm planning to release a new version soon, and I think you'd like to have this work released sooner rather than later...

@blag
Copy link
Contributor Author

blag commented Aug 13, 2020

Apologies for not circling back around to this until now. It's sometimes difficult or impossible to predict when I'll get interrupted with higher priority tasks.

When I wrote this PR, I had just read the Security section of the README with the assumption that large hashes with small curves was weird and insecure, but now I'm questioning that assumption. My philosophy on cryptography software is that it should be as easy to use as possible, and part of that does mean intentionally making it difficult for people to use in an insecure manner.

But with common, modern hashes that were designed to be possibly truncated (eg: SHA-2), truncating them only makes it slightly easier to find collisions.

So the remaining rationale for this PR is to preserve backwards compatibility for existing users of VerifyingKey.verify() and SigningKey.sign(), and also to give users a quick and easy way to flip back to the old behavior if truncating is found to be problematic in the future. But that's a decently big if.

If that's not enough to convince you, that's okay. I've also been working on a PR to python-jose that fixes the tests around this issue, so it won't be a problem for me for much longer either way.

Thanks for your work on this project so far!

@tomato42
Copy link
Member

sorry for the late reply, I was on holidays

any cryptosystem is as strong as the weakest link, if you use P-256 curve with SHA-512 hash, the signature will be as strong as P-256 curve, not as the SHA-512 hash
at the same time, on a 64bit machine, SHA-512 is significantly faster than SHA-256 (the "natural" choice for P-256 curve), so when signing large documents, mismatching hashes can very well be quite intentional

so, as long as the application is ok with the given curve and with the given hash used, it can mix and match it

regarding the Security section in README: I consider "having API that's hard to misuse" to fall under "teaching tool", that being said, deciding what combinations of parameters are acceptable is a policy question, not API question, and I think the API provided does allow to build a policy on top of it

@tomato42 tomato42 added this to the v0.16.0 milestone Aug 24, 2020
@tomato42
Copy link
Member

looks good, thanks!

@tomato42 tomato42 merged commit 69e9735 into tlsfuzzer:master Aug 24, 2020
@blag blag deleted the expose-allow-truncate branch October 30, 2020 07:48
@blag
Copy link
Contributor Author

blag commented Oct 30, 2020

Thanks for merging and releasing 0.16! 😃

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

Successfully merging this pull request may close these issues.

3 participants