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

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 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 moriyoshi/fix-asn1-generalizedtime-casts branch from 610ca51 to fa99b5f Compare April 6, 2017 07:40
@moriyoshi
Copy link
Contributor Author

UPDATE: pyca/cryptography#3491 was merged.

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

Bumped up cryptography version requirement. Ready to merge.

@reaperhulk
Copy link
Member

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
reaperhulk added a commit to reaperhulk/pyopenssl that referenced this pull request Jun 21, 2017
@moriyoshi moriyoshi deleted the moriyoshi/fix-asn1-generalizedtime-casts branch June 21, 2017 16:19
alex pushed 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 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

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.
Development

Successfully merging this pull request may close these issues.

3 participants