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.

Sign up for GitHub

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

Jump to bottom

ssl_cert_username_cea: Use CertificateExactAssertion as username #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ssl_cert_username_cea: Use CertificateExactAssertion as username #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/example-config/conf.d/10-ssl.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ ssl_key = </etc/ssl/private/dovecot.pem
# auth_ssl_username_from_cert=yes.
#ssl_cert_username_field = commonName

# Use the certificate's CertificateExactAssertion for username, as defined
# in RFC4523 appendix A.1. You'll also need to set
# auth_ssl_username_from_cert=yes.
#ssl_cert_username_cea = no
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably could be configured via ssl_cert_username_field = CertificateExactAssertion? Otherwise there's a conflict with these two settings.

Although I don't know, how common is this use case anyway? Maybe this would be better as a more generic setting using variables, like:

ssl_cert_username = { serialNumber %{cert:serial_number} , issuer rdnSequence:"%{cert:issuer_name}" }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably could be configured via ssl_cert_username_field = CertificateExactAssertion? Otherwise there's a conflict with these two settings.

I considered it originally, however ssl_cert_username_field refers to rdns inside the subject of the certificate, while CertificateExactAssertion refers to a property of the whole certificate. It felt cleaner to have a separate directive.

ssl_cert_username_cea wins.

Maybe this would be better as a more generic setting using variables

The problem with the variable approach is that the RFC definition of CertificateExactAssertion is precise on the format of both the serialNumber and the issuer.

The XN_FLAG_RFC2253 flag gives the correct format, while X509_NAME_oneline uses a legacy format.

I ran into the same problem when I did Apache httpd's support, the definitions of the existing variables weren't tight enough.


# SSL DH parameters
# Generate new params with `openssl dhparam -out /etc/dovecot/dh.pem 4096`
# Or migrate from old ssl-parameters.dat file with the command dovecot
Expand Down
1 change: 1 addition & 0 deletions src/lib-ldap/ldap-connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ int ldap_connection_init(struct ldap_client *client,
conn->ssl_set.ca = NULL;
conn->ssl_set.cert.key_password = NULL;
conn->ssl_set.cert_username_field = NULL;
conn->ssl_set.cert_username_cea = FALSE;
conn->ssl_set.crypto_device = NULL;

if (set->ssl_set != NULL) {
Expand Down
3 changes: 3 additions & 0 deletions src/lib-master/master-service-ssl-settings.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ static const struct setting_define master_service_ssl_setting_defines[] = {
DEF(STR, ssl_curve_list),
DEF(STR, ssl_min_protocol),
DEF(STR, ssl_cert_username_field),
DEF(BOOL, ssl_cert_username_cea),
DEF(STR, ssl_crypto_device),
DEF(BOOL, ssl_verify_client_cert),
DEF(BOOL, ssl_client_require_valid_cert),
Expand All @@ -50,6 +51,7 @@ static const struct master_service_ssl_settings master_service_ssl_default_setti
.ssl_curve_list = "",
.ssl_min_protocol = "TLSv1.2",
.ssl_cert_username_field = "commonName",
.ssl_cert_username_cea = FALSE,
.ssl_crypto_device = "",
.ssl_verify_client_cert = FALSE,
.ssl_client_require_valid_cert = TRUE,
Expand Down Expand Up @@ -178,6 +180,7 @@ static void master_service_ssl_common_settings_to_iostream_set(

set_r->crypto_device = p_strdup(pool, ssl_set->ssl_crypto_device);
set_r->cert_username_field = p_strdup(pool, ssl_set->ssl_cert_username_field);
set_r->cert_username_cea = ssl_set->ssl_cert_username_cea;

set_r->verbose = ssl_set->verbose_ssl;
set_r->verbose_invalid_cert = ssl_set->verbose_ssl;
Expand Down
1 change: 1 addition & 0 deletions src/lib-master/master-service-ssl-settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ struct master_service_ssl_settings {
bool ssl_require_crl;
bool verbose_ssl;
bool ssl_prefer_server_ciphers;
bool ssl_cert_username_cea;

/* These are derived from ssl_options, not set directly */
struct {
Expand Down
1 change: 1 addition & 0 deletions src/lib-master/master-service-ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ void master_service_ssl_ctx_init(struct master_service *service)
ssl_set.dh = server_set->ssl_dh;
ssl_set.cert.key_password = server_set->ssl_key_password;
ssl_set.cert_username_field = set->ssl_cert_username_field;
ssl_set.cert_username_cea = set->ssl_cert_username_cea;
if (server_set->ssl_alt_cert != NULL &&
*server_set->ssl_alt_cert != '\0') {
ssl_set.alt_cert.cert = server_set->ssl_alt_cert;
Expand Down
83 changes: 83 additions & 0 deletions src/lib-ssl-iostream/iostream-openssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ openssl_iostream_set(struct ssl_iostream *ssl_io,
set->verbose_invalid_cert ||
event_want_debug(ssl_io->event);
ssl_io->allow_invalid_cert = set->allow_invalid_cert;
ssl_io->username_cea = set->cert_username_cea;
return 0;
}

Expand Down Expand Up @@ -811,6 +812,86 @@ openssl_iostream_get_peer_name(struct ssl_iostream *ssl_io)
#endif
i_assert(x509 != NULL);

if (ssl_io->username_cea) {

ASN1_INTEGER *serialNumber;
X509_NAME *issuer;
BIGNUM *bn;
char *decimal;
BIO *bio;

bio = BIO_new(BIO_s_mem());
if (!bio) {
name = NULL;
goto end;
}

BIO_puts(bio, "{ serialNumber ");

serialNumber = X509_get_serialNumber(x509);
if (!serialNumber) {
BIO_free(bio);
name = NULL;
goto end;
}

bn = ASN1_INTEGER_to_BN(serialNumber, NULL);
if (!bn) {
BIO_free(bio);
name = NULL;
goto end;
}

decimal = BN_bn2dec(bn);
if (!decimal) {
BIO_free(bio);
BN_free(bn);
name = NULL;
goto end;
}

BIO_puts(bio, decimal);

OPENSSL_free(decimal);
BN_free(bn);

BIO_puts(bio, ", issuer rdnSequence:\"");

issuer = X509_get_issuer_name(x509);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be escaping \ and " in case they happen to exist in the issuer name. str_escape() would do that. And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be escaping \ and " in case they happen to exist in the issuer name. str_escape() would do that

Let me check this.

And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Felt a bit weird to mix them up, but can do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should be escaping \ and " in case they happen to exist in the issuer name.

Checked - escaping is unnecessary (and would break the string), as the rfc2253 already escapes the issuer name as described below:

https://datatracker.ietf.org/doc/html/rfc2253#section-2.4

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And seems like this would be a bit simpler if BIO was used only for the X509_NAME_print_ex() and the rest used either t_strdup_printf() or string_t.

Looking at the code, using a BIO is the simplest way.

If I had to try and convert it to t_strdup_printf(), we would still have the same extraction of the fields, then we would need to t_malloc0() the BIO, then assemble the string, then get rid of the temporary t_malloc0.

Letting openssl do all the work is way cleaner.

Copy link
Contributor

@sirainen sirainen Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked - escaping is unnecessary (and would break the string), as the rfc2253 already escapes the issuer name as described below:

I think the certificates could still be evil and have " character without escaping it? So some sanity check for the string would still be useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the certificates could still be evil and have " character without escaping it? So some sanity check for the string would still be useful.

That's not possible in this case.

RFC4523 appendix A.1 describes a string that consists of the serial number (in a safe format), concatenated with the issuer DN encoded as per rfc2253 (in a safe format).

If the issuer wasn't safely escaped already, someone could create a malicious certificate that contained the target desired issuer with a 0x00 character in it, and then fake the identity of another certificate. This is prevented because rfc2253 mandates that the distinguished name be escaped.

Quoting from rfc2253 to show we are escaped already:

If the UTF-8 string does not have any of the following characters
which need escaping, then that string can be used as the string
representation of the value.

o   a space or "#" character occurring at the beginning of the
    string

o   a space character occurring at the end of the string

o   one of the characters ",", "+", """, "\", "<", ">" or ";"

Implementations MAY escape other characters.

If a character to be escaped is one of the list shown above, then it
is prefixed by a backslash ('' ASCII 92).

Otherwise the character to be escaped is replaced by a backslash and
two hex digits, which form a single byte in the code of the
character.

Any attempt to modify the CEA string outside that described by the RFC above means the string will not match with or interoperate with other software.

if (!issuer) {
BIO_free(bio);
name = NULL;
goto end;
}

X509_NAME_print_ex(bio, issuer, 0, XN_FLAG_RFC2253);

BIO_puts(bio, "\" }");

len = BIO_pending(bio);
if (len < 0) {
name = NULL;
}
else {
name = t_malloc0(len + 1);
if (BIO_read(bio, name, len) != len) {
name = NULL;
}
else if (strlen(name) != (size_t)len) {
/* NUL characters in name. Someone's trying to fake
being another user? Don't allow it. */
name = NULL;
}
else {
name[len] = 0;
}
}

BIO_free(bio);

goto end;
}

len = X509_NAME_get_text_by_NID(X509_get_subject_name(x509),
ssl_io->username_nid, NULL, 0);
if (len < 0)
Expand All @@ -827,6 +908,8 @@ openssl_iostream_get_peer_name(struct ssl_iostream *ssl_io)
name = NULL;
}
}

end:
X509_free(x509);

return name;
Expand Down
2 changes: 2 additions & 0 deletions src/lib-ssl-iostream/iostream-openssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ struct ssl_iostream_context {
int username_nid;

bool client_ctx:1;
bool username_cea:1;
};

struct ssl_iostream {
Expand Down Expand Up @@ -68,6 +69,7 @@ struct ssl_iostream {
bool ostream_flush_waiting_input:1;
bool closed:1;
bool destroyed:1;
bool username_cea:1;
};

extern int dovecot_ssl_extdata_index;
Expand Down
1 change: 1 addition & 0 deletions src/lib-ssl-iostream/iostream-ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ struct ssl_iostream_settings {
bool prefer_server_ciphers; /* both */
bool compression; /* context-only */
bool tickets; /* context-only */
bool cert_username_cea; /* both */
};

/* Load SSL module */
Expand Down