On Thu, Feb 28, 2019 at 01:39:50PM -0600, jared.bents@xxxxxxxxxxxxxxxxxxx wrote:
> This patch added 'check_cert_subject' support to match the value of
> every field against the DN of the subject in the client certificate. If
> the values do not match, the certificate verification will fail and will reject
> the user.
>
> This option allows hostapd to match every individual field in the right order,
> also allow '*' character as a wildcard (e.g OU=Development*).
>
> Note: Hostapd will match string up to 'wildcard' against the DN of the subject
> in the client certificate for every individual field.
Thanks. I applied this quite a bit of cleanup and some design changes to
make this more consistent with how the existing client side subject name
validation checks are used. This also allowed the same implementation to
be enabled in wpa_supplicant configuration.
> diff --git a/src/crypto/tls.h b/src/crypto/tls.h
> +struct tls_dn_field_order_cnt {
> + uint8_t cn;
> + uint8_t c;
> + uint8_t l;
> + uint8_t st;
> + uint8_t o;
> + uint8_t ou;
> + uint8_t email;
> +};
This was used only as an internal data structure within tls_openssl.c
(i.e., this has nothing to do with the API used to configure the TLS
library parameters from upper layers), so I moved it to tls_openssl.c.
> +enum digest_alg{
> + DIGEST_HASH_ALG_MD5,
> + DIGEST_HASH_ALG_SHA,
> + DIGEST_HASH_ALG_SHA1,
> + DIGEST_HASH_ALG_SHA224,
> + DIGEST_HASH_ALG_SHA256,
> + DIGEST_HASH_ALG_SHA384,
> + DIGEST_HASH_ALG_SHA512
> +};
Same here, but I actually deleted this completely.
> @@ -329,10 +350,12 @@ int __must_check tls_global_set_params(
> * @check_crl: 0 = do not verify CRLs, 1 = verify CRL for the user certificate,
> * 2 = verify CRL for all certificates
> * @strict: 0 = allow CRL time errors, 1 = do not allow CRL time errors
> + * @check_cert_subject : Hostapd configuration 'check_cert_subject' string pointer
> * Returns: 0 on success, -1 on failure
> */
> int __must_check tls_global_set_verify(void *tls_ctx, int check_crl,
> - int strict);
> + int strict,
> + const char *check_cert_subject);
I moved check_cert_subject into struct tls_connection_params so that it
can be set in both server and client cases with tls_global_set_params()
and tls_connection_set_params().
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> @@ -206,6 +206,7 @@ struct tls_context {
> void *cb_ctx;
> int cert_in_cb;
> char *ocsp_stapling_response;
> + char *check_cert_subject;
> };
That makes no sense to me. This belongs in struct tls_data and/or struct
tls_connection, so I removed this unnecessary copying into the global
tls_context.
> static struct tls_context *tls_global = NULL;
> @@ -219,6 +220,8 @@ struct tls_data {
> char *ca_cert;
> unsigned int crl_reload_interval;
> struct os_reltime crl_last_reload;
> + char *client_cert_subject;
> + struct tls_dn_field_order_cnt *dn_cnt;
> };
I saw no reason for maintaining struct tls_dn_field_order_cnt here over
longer period of time since it was cleared for every single use
separately. In other words, I moved it to be a local stack variable in
tls_match_dn_field().
> +/**
> + * client_cert_fingerprint - print fingerprint of certificate
> + * @cert: Certificate
> + * @alg: hash algorithm
> + * Returns: Return 1 on success and 0 on Failure
> +*/
> +static int client_cert_fingerprint(X509* cert, enum digest_alg alg)
...
This does not seem to have anything to do with the change described in
the commit message. Furthermore, this was very complex way of debug
printing a fingerprint with large number of different hash functions
while then hardcoding only one of them to used. I dropped this
completely since there was no justification for adding such information
into debug log (and if there is a use case, this could be done in a much
simpler manner).
> +static int match_dn_field(X509 *cert, int nid, char *value, struct tls_dn_field_order_cnt *dn_cnt)
> + if( '*' == *(value + len - 1)) {
> + /* Compare actual certificate dn field value with configuration file dn field value upto specified length */
> + if (!os_strncasecmp(cn->data, value, len - 1)) {
This is a security vulnerability since comparison would be stopped at an
embedded '\0' character.
> + /* Compare actual certificate dn field value with configuration file dn field value */
> + if (!os_strcmp(cn->data, value)) {
Same issue here with embedded '\0'. And why was this full string match
case sensitive while the wildcard match was case insensitive? Such
differences sound quite undesired. I replaced both of these with memcmp.
> +static int tls_match_dn_field(X509 *cert, const char *match, struct tls_dn_field_order_cnt *dn_cnt)
> + while ( match[match_loop] != '\0') {
> + /* Stop at '/' character */
> + if (match[match_loop] == '/') {
> + field[field_loop] = '\0';
> + if (strlen(field) > 0) {
> + if(!get_value_from_field(cert, field, dn_cnt)) {
> + ret = 0;
> + break;
> + }
> + }
> + os_memset(field,0,256);
> + field_loop = 0;
> + last_index = match_loop;
> + last_index++;
> + } else {
> + field[field_loop++] = match[match_loop];
This could result in writing beyond the buffer..
> + }
> + match_loop++;
> + }
> + if (strlen(field) > 0 && ret != 0) {
> + strncpy(field, match+last_index, len - last_index);
As could this.
> + if(!get_value_from_field(cert, field, dn_cnt))
> + ret = 0;
> + }
This is unnecessarily complex. Replaced this with a cleaner loop using
cstr_token().
> + X509_NAME_oneline(X509_get_subject_name(cert), buf, sizeof(buf));
> + wpa_printf(MSG_INFO,"TLS: Certificate subject= %s\n", buf);
> + os_memset(buf,0,2048);
> + X509_NAME_oneline(X509_get_issuer_name(cert), buf, sizeof(buf));
> + wpa_printf(MSG_INFO,"TLS: Certificate issuer= %s\n", buf);
Those debug prints have nothing to do with this function, so I dropped
them.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
