Skip to content

Fix #52093: openssl_csr_sign silently truncates $serial #7209

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

Closed
wants to merge 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 30, 2021

We must not silently truncate integer arguments passed to a function.
In this case we could use ASN1_INTEGER_set_int64() on LLP64
architectures, but that might be regarded a BC break; instead we choose
to raise a warning regarding the truncation for now.

We must not silently truncate integer arguments passed to a function.
In this case we could use `ASN1_INTEGER_set_int64()` on LLP64
architectures, but that might be regarded a BC break; instead we choose
to raise a warning regarding the truncation for now.
@cmb69 cmb69 added the Bug label Jun 30, 2021
@cmb69
Copy link
Member Author

cmb69 commented Jun 30, 2021

Hmm, we must skip the tests on architectures where no truncation occurs. Not sure about the best skipif clause – maybe just run on Windows x64?

@nikic
Copy link
Member

nikic commented Jun 30, 2021

Using ASN1_INTEGER_set_int64() seems better here, and I don't think that counts as a BC break. If I understand correctly, then this will already work fine on platforms where long is 64-bit, so effectively this means that currently 64-bit Windows and 64-bit Linux behave differently, which seems like a plain bug.

We actually use `ASN1_INTEGER_set_int64()`[1] on 64bit Windows which is
the only LLP64 platform we support, and where we can be reasonably sure
that nobody uses OpenSSL 1.0 anymore (the official Windows dependencies
ship OppenSSL 1.1 for years).

We also adapt the test to actually verify the serial number.

[1] <https://www.openssl.org/docs/man1.1.0/man3/ASN1_INTEGER_set_int64.html#HISTORY>
@cmb69
Copy link
Member Author

cmb69 commented Jun 30, 2021

Fine! I switched to using ASN1_INTEGER_set_int64() and improved the test to actually verify the serialNumber. The test failures appear to be unrelated.

@cmb69 cmb69 closed this in 334387b Jul 1, 2021
@cmb69 cmb69 deleted the cmb/52093 branch July 1, 2021 13:47
@fichtner
Copy link

@cmb69 we are getting reports that this crashes on LibreSSL 3.3 builds whereas previously we never had issues with LibreSSL compat before... I'm trying to get to the bottom of this now. :)

@fichtner
Copy link

Found it 6724d5d4c2c502b09 sorry for the noise!

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

Successfully merging this pull request may close these issues.

3 participants