plugins: irc: support server certificate public key fingerprints #1507
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(If anyone needs to test this, you can obtain a
|
|
👍 |
sim642
reviewed
May 14, 2020
src/plugins/irc/irc-server.c
Outdated
Comment on lines
4315
to
4409
| /* attempt to extract and fingerprint the certificate's public key */ | ||
| if (gnutls_pubkey_init (&pk) != GNUTLS_E_SUCCESS) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%sgnutls: failed to initialize public key object"), | ||
| weechat_prefix ("error")); | ||
| } | ||
| else | ||
| { | ||
| if (gnutls_pubkey_import_x509 (pk, certificate, 0) | ||
| != GNUTLS_E_SUCCESS) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%sgnutls: failed to import certificate public key"), | ||
| weechat_prefix ("error")); | ||
| } | ||
| else if (gnutls_pubkey_export (pk, GNUTLS_X509_FMT_DER, NULL, | ||
| &pkbuf_len) != GNUTLS_E_SHORT_MEMORY_BUFFER) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%sgnutls: failed to export certificate public key"), | ||
| weechat_prefix ("error")); | ||
| } | ||
| else if (!pkbuf_len) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%sgnutls: failed to export certificate public key"), | ||
| weechat_prefix ("error")); | ||
| } | ||
| else if (!(pkbuf = malloc (pkbuf_len))) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%s%s: not enough memory (%s)"), | ||
| weechat_prefix ("error"), IRC_PLUGIN_NAME, | ||
| "fingerprint"); | ||
| } | ||
| else if (gnutls_pubkey_export (pk, GNUTLS_X509_FMT_DER, pkbuf, | ||
| &pkbuf_len) != GNUTLS_E_SUCCESS) | ||
| { | ||
| weechat_printf ( | ||
| server->buffer, | ||
| _("%sgnutls: failed to export certificate public key"), | ||
| weechat_prefix ("error")); | ||
| } |
There was a problem hiding this comment.
This whole public key extraction might deserve to be a separate function, if one doesn't already exist in GnuTLS.
There was a problem hiding this comment.
It does exist in GnuTLS, in that gnutls_pubkey_export is all you get. This scaffolding around it is necessary to prepare for that export.
There is no gnutls_x509_crt_get_* function that will hand you the SubjectPublicKeyInfo structure; the closest is gnutls_x509_crt_get_pk_rsa_raw which hands you the modulus and exponent separately (and would hardcode us to RSA).
I can move it to a separate function on request, but they would still be heavily tied together...
aaronmdjones
added a commit
to AlphaChat/weechat
that referenced
this pull request
May 14, 2020
Codecov Report
@@ Coverage Diff @@
## master #1507 +/- ##
==========================================
- Coverage 35.26% 35.25% -0.01%
==========================================
Files 203 203
Lines 84164 84189 +25
==========================================
+ Hits 29678 29681 +3
- Misses 54486 54508 +22
Continue to review full report at Codecov.
|
aaronmdjones
added a commit
to AlphaChat/weechat
that referenced
this pull request
May 14, 2020
This allows users to pin known public key fingerprints instead of certificate fingerprints, which means that the fingerprints will not change when the server's certificate is renewed (unless that is accompanied by a change of server private key). With the advent of short-lifetime certificates from the likes of Let's Encrypt, this is a very useful feature. This is not any more insecure than a certificate fingerprint; server certificates only exist to securely bind a server name to a cryptographic identity (its public key), so that you have some degree of confidence that you are talking to the correct server, and not an impostor (MITM). If you're establishing its crypto- graphic identity up-front (by pinning a known-good fingerprint of it with a cryptographically-secure digest algorithm), then the certificate is, by definition, not needed. In fact, this is slightly more secure than a certificate fingerprint, because certificates are a lot more malleable; the ability to pad and inject random data to aid in e.g. a second preimage attack is much more easily achieved with an X.509 certificate than it is with a public key (because you can generate a valid private key, compute its corresponding public key, and then mess with other fields in the certificate to affect the digest instead). Also, any attack against Hash(publickey) implies an attack against Hash(certificate), which would break the CA PKI model anyway. Specifically, this fingerprints the entire SubjectPublicKeyInfo structure from the certificate, which includes not just the key itself, but also other meta-information about it (like its algorithm identifier and any associated parameters). This is the same public key fingerprinting approach taken by e.g. the HSTS and DANE TLSA standards, and also some IRC servers (such as Charybdis IRCd). While working on this commit, I noticed a few oddities and/or deficiencies in the existing fingerprinting code, which this commit also corrects and/or improves: - Several for() loops used an unrelated variable to iterate over arrays of fingerprint variables (for setting their initial values or freeing them after). - When displaying that the server's fingerprint has matched the client's configuration, it did not specify which fingerprint was received from the server for matching. This is relevant in the case of a comma-separated list of fingerprints. - The code checked if a variable was non-NULL before calling free(3) on it, but free(3) is well-defined to do nothing if passed NULL. cf. weechat#879 Signed-off-by: Aaron Jones <aaron@alphachat.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows users to pin known public key fingerprints instead of (or in addition to) certificate fingerprints, which means that the fingerprints will not change when the server's certificate is renewed (unless that is accompanied by a change of server private key). With the advent of short-lifetime certificates from the likes of Let's Encrypt, this is a very useful feature.
This is not any more insecure than a certificate fingerprint; server certificates only exist to securely bind a server name to a cryptographic identity (its public key), so that you have some degree of confidence that you are talking to the correct server, and not an impostor (MITM). If you're establishing its cryptographic identity up-front (by pinning a known-good fingerprint of it with a cryptographically-secure digest algorithm), then the certificate is, by definition, not needed. In fact, this is slightly more secure than a certificate fingerprint, because certificates are a lot more malleable; the ability to pad and inject random data to aid in e.g. a second preimage attack is much more easily achieved with an X.509 certificate than it is with a public key (because you can generate a valid private key, compute its corresponding public key, and then mess with other fields in the certificate to affect the digest instead). Also, any attack against
Hash(publickey)implies an attack againstHash(certificate), which would break the CA PKI model anyway.Specifically, this fingerprints the entire
SubjectPublicKeyInfostructure from the certificate, which includes not just the key itself, but also other meta-information about it (like its algorithm identifier and any associated parameters). This is the same public key fingerprinting approach taken by e.g. the HSTS and DANE TLSA standards, and also some IRC servers (such as Charybdis IRCd).While working on this commit, I noticed a few oddities and/or deficiencies in the existing fingerprinting code, which this commit also corrects and/or improves:
Fingerprints were scanned 1 byte (2 hexadecimal characters) at a time and then compared 1 byte at a time, instead of using efficient C standard library string comparison functions.
Several
for()loops used an unrelated variable to iterate over arrays of fingerprint variables (for setting their initial values or freeing them after).When displaying that the server's fingerprint has matched the client's configuration, it did not specify which fingerprint was received from the server for matching. This is relevant in the case of a comma-separated list of fingerprints.
The code checked if a variable was non-
NULLbefore callingfree(3)on it, butfree(3)is well-defined to do nothing if passedNULL.cf. #879