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

Fix invalid cast from ASN1_TIME to ASN1_GENERALIZEDTIME #612

Merged

Conversation

@moriyoshi
Copy link
Contributor

@moriyoshi moriyoshi commented Apr 5, 2017

X509_get_notBefore() and X509_get_notAfter() return a ASN1_TIME structure which is not compatible with ASN1_GENERALIZEDTIME, but the original code recklessly casts it to ASN1_GENERALIZEDTIME and pass it to ASN1_GENERALIZEDTIME_set_string().

LibreSSL is rigorous enough to reject such.

pyca/cryptography#3491 needs to be applied to cryptography before the application of this patch.

@codecov
Copy link

@codecov codecov bot commented Apr 6, 2017

Codecov Report

Merging #612 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   96.78%   96.81%   +0.03%     
==========================================
  Files          18       18              
  Lines        5626     5621       -5     
  Branches      390      389       -1     
==========================================
- Hits         5445     5442       -3     
+ Misses        121      120       -1     
+ Partials       60       59       -1
Impacted Files Coverage Δ
src/OpenSSL/crypto.py 96.55% <100%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a40898b...b93f1c3. Read the comment docs.

@moriyoshi moriyoshi force-pushed the payjp:moriyoshi/fix-asn1-generalizedtime-casts branch from 610ca51 to fa99b5f Apr 6, 2017
@moriyoshi
Copy link
Contributor Author

@moriyoshi moriyoshi commented Apr 6, 2017

UPDATE: pyca/cryptography#3491 was merged.

@moriyoshi moriyoshi force-pushed the payjp:moriyoshi/fix-asn1-generalizedtime-casts branch from fa99b5f to b93f1c3 Jun 21, 2017
@moriyoshi
Copy link
Contributor Author

@moriyoshi moriyoshi commented Jun 21, 2017

Bumped up cryptography version requirement. Ready to merge.

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented Jun 21, 2017

Those two functions do return an ASN1_TIME, but unfortunately the pyOpenSSL API here for set_notAfter and set_notBefore is documented as taking the form of a byte string generalized time. (e.g. not just YYYYMMDDHHMMSSZ but also YYYYMMDDhhmmss+hhmm or YYYYMMDDhhmmss-hhmm). No one should ever have been passing any form other than the first though, and after checking our primary downstreams it looks like that's not a thing anyone is doing, so let's go ahead and make the change.

@reaperhulk reaperhulk merged commit 80b25ef into pyca:master Jun 21, 2017
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 96.78%)
Details
codecov/project 96.81% (+0.03%) compared to a40898b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
reaperhulk added a commit to reaperhulk/pyopenssl that referenced this pull request Jun 21, 2017
@moriyoshi moriyoshi deleted the payjp:moriyoshi/fix-asn1-generalizedtime-casts branch Jun 21, 2017
alex added a commit that referenced this pull request Jun 21, 2017
* update docs and and changelog for #612

* update changelog

* more detail
@moriyoshi
Copy link
Contributor Author

@moriyoshi moriyoshi commented Jun 22, 2017

the pyOpenSSL API here for set_notAfter and set_notBefore is documented as taking the form of a byte string generalized time. (e.g. not just YYYYMMDDHHMMSSZ but also YYYYMMDDhhmmss+hhmm or YYYYMMDDhhmmss-hhmm).

It appears ASN1_TIME_set_string() can handle the generalized time in addition to the UTC time. (ASN1_TIME is a neutral data structure that can represent either ASN1_UTCTIME or ASN1_GENERALIZEDTIME )

https://github.com/openssl/openssl/blob/master/crypto/asn1/a_time.c#L120-L124

BTW, ASN1_TIME_set_string_X509() is supposed to be better here rather than ASN1_TIME_set_string(), which is a newly introduced, more strict variant of the latter:

openssl/openssl@04e6271

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented Jun 26, 2017

Interesting. Thanks for investigating that. We should revert that backwards incompatibility warning then.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants