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

Incorrect version in self signed certificate #4007

Open
donovanhide opened this issue Nov 29, 2021 · 7 comments
Open

Incorrect version in self signed certificate #4007

donovanhide opened this issue Nov 29, 2021 · 7 comments

Comments

@donovanhide
Copy link
Contributor

https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/basics/impl/make_SSLContext.cpp#L159

X.509 certificates can have a version number between 1 and 3 inclusive. The constant NID_X509 equals 12. The correct constants are one of X509_VERSION_1,X509_VERSION_2 or X509_VERSION_3. Maybe this is an ancient copy paste error? Anyway, it stops Go's crypto\tls library parsing the certificate, reporting an invalid version.

@donovanhide
Copy link
Contributor Author

@nbougalis Another one for you?

@donovanhide
Copy link
Contributor Author

Looks like this might have been the copy paste source :-)

https://stackoverflow.com/questions/2883164/openssl-certificate-lacks-key-identifiers

@donovanhide
Copy link
Contributor Author

Simplest fix for this is to simply delete the erroneous line above and accept the default version (X509_VERSION_1):

https://www.openssl.org/docs/man3.0/man3/X509_set_version.html

Perhaps this is a good lesson in checking return codes from C functions :-) Want a PR?

@MarkusTeufelberger
Copy link
Collaborator

Practically every x509 certificate out there has version 3, please use that if you fix this.

@donovanhide
Copy link
Contributor Author

https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1

This field describes the version of the encoded certificate. When
extensions are used, as expected in this profile, version MUST be 3
(value is 2). If no extensions are present, but a UniqueIdentifier
is present, the version SHOULD be 2 (value is 1); however, the
version MAY be 3. If only basic fields are present, the version
SHOULD be 1 (the value is omitted from the certificate as the default
value); however, the version MAY be 2 or 3.

To me, that reads like it should be X509_VERSION_1.

@donovanhide
Copy link
Contributor Author

Bear in mind that this is only for the self-signed certificate. not the one that can be specified in the configuration file.

https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/basics/impl/make_SSLContext.cpp#L268-L283

It is notable that a lot of return codes are ignored in this code section as well.

@MarkusTeufelberger
Copy link
Collaborator

 x509_ptr cert = x509_new(); 
 x509_set_pubkey(cert.get(), pkey.get()); 
 x509_sign(cert.get(), pkey.get());

Hngggghhhh...

As the co-author of https://docs.ansible.com/ansible/latest/collections/community/crypto/x509_certificate_module.html this looks a bit... let's call it fast and loose.

Yeah, version 1 or 2 might be possible too, but maybe eventually there's some more advanced TLS functionality used to secure P2P connections - then at least this is one place that doesn't need changing or that one needs to worry about. Version 13 (version values are one-off...) is definitely a bug though that should be fixed.

donovanhide added a commit to donovanhide/rippled that referenced this issue Dec 3, 2021
donovanhide added a commit to donovanhide/rippled that referenced this issue Dec 3, 2021
legleux pushed a commit to legleux/rippled that referenced this issue Sep 23, 2022
When starting, the code generates a new ephemeral private key and
a corresponding certificate to go along with it. This process can
take time and, while this is unlikely to matter for normal server
operations, it can have a significant impact for unit testing and
development. Profiling data suggests that ~20% of the time needed
for a unit test run can be attributed to this.

This commit does several things:

1. It restructures the code so that a new self-signed certificate
   and its corresponding private key are only initialized once at
   startup; this has minimal impact on the operation of a regular
   server.
2. It provides new default DH parameters. This doesn't impact the
   security of the connection, but those who compile from scratch
   can generate new parameters if they so choose.
3. It properly sets the version number in the certificate, fixing
   issue XRPLF#4007; thanks to @donovanhide for the report.
4. It uses SHA-256 instead of SHA-1 as the hash algorithm for the
   certificate and adds some X.509 extensions as well as a random
   128-bit serial number.
5. It rounds the certificate's "start of validity" period so that
   the server's precise startup time cannot be easily deduced and
   limits the validity period to two years, down from ten years.
6. It removes some CBC-based ciphers from the default cipher list
   to avoid some potential security issues, such as CVE-2016-2107
   and CVE-2013-0169.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@donovanhide @MarkusTeufelberger and others