Hi, please find attached two small patches related to the conversion of the public key in a certificate to a public ssh-key.
The first fixes an issue which should only happen in master. The second might be useful for already released versions as well, but I would wait until we get a report that the old scheme fails with some certificates. bye, Sumit
From 30dc3f904918a4a5b8e1245222881d97f1737fdf Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Fri, 17 Jun 2016 13:50:55 +0200 Subject: [PATCH 1/2] SSH-CERT: always initialize cert_verify_opts Currently cert_verify_opts is only initialized when there is an option in the config file. This might cause issues later when the struct is accessed. Since parse_cert_verify_opts() can already handle an empty option the additional check is not needed at all. --- src/responder/ssh/sshsrv_cmd.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c index ba3b694d97821696ca0218c279c1f9d9859ab13e..89a173d47a2a1817ac6c2993e9b122e8083a92b3 100644 --- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -819,14 +819,12 @@ static errno_t get_valid_certs_keys(TALLOC_CTX *mem_ctx, goto done; } - if (cert_verification_opts != NULL) { - ret = parse_cert_verify_opts(tmp_ctx, cert_verification_opts, - &cert_verify_opts); - if (ret != EOK) { - DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to parse verifiy option.\n"); - goto done; - } + ret = parse_cert_verify_opts(tmp_ctx, cert_verification_opts, + &cert_verify_opts); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to parse verifiy option.\n"); + goto done; } el_res = talloc_zero(tmp_ctx, struct ldb_message_element); -- 2.1.0
From 73c2bb7a12d124619f664890b6108d836c2dc2f5 Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Wed, 15 Jun 2016 21:49:02 +0200 Subject: [PATCH 2/2] cert_to_ssh_key: properly add leading 0 to bignums In the ssh keys a leading 0 is added to the bignums of the RSA modulus and exponent if the leading bit is set to avoid the interpretation as a negative number. --- src/util/cert/nss/cert.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/util/cert/nss/cert.c b/src/util/cert/nss/cert.c index 851378379f4e508824368347aa7a7e079d6cf8e1..2e3fe45743b4175bbc0b2950aca24954041930bd 100644 --- a/src/util/cert/nss/cert.c +++ b/src/util/cert/nss/cert.c @@ -267,6 +267,8 @@ errno_t cert_to_ssh_key(TALLOC_CTX *mem_ctx, const char *ca_db, parameters.length = sizeof (parameters); SECStatus rv; SECStatus rv_verify; + size_t exponent_prefix_len; + size_t modulus_prefix_len; if (der_blob == NULL || der_size == 0) { return EINVAL; @@ -369,10 +371,21 @@ errno_t cert_to_ssh_key(TALLOC_CTX *mem_ctx, const char *ca_db, goto done; } + /* Looks like nss drops the leading 00 which afaik is added to make sure + * the bigint is handled as positive number if the leading bit is set. */ + exponent_prefix_len = 0; + if (cert_pub_key->u.rsa.publicExponent.data[0] & 0x80) { + exponent_prefix_len = 1; + } + + modulus_prefix_len = 0; + if (cert_pub_key->u.rsa.modulus.data[0] & 0x80) { + modulus_prefix_len = 1; + } size = SSH_RSA_HEADER_LEN + 3 * sizeof(uint32_t) + cert_pub_key->u.rsa.modulus.len + cert_pub_key->u.rsa.publicExponent.len - + 1; /* see comment about missing 00 below */ + + exponent_prefix_len + modulus_prefix_len; buf = talloc_size(mem_ctx, size); if (buf == NULL) { @@ -386,17 +399,20 @@ errno_t cert_to_ssh_key(TALLOC_CTX *mem_ctx, const char *ca_db, SAFEALIGN_SET_UINT32(buf, htobe32(SSH_RSA_HEADER_LEN), &c); safealign_memcpy(&buf[c], SSH_RSA_HEADER, SSH_RSA_HEADER_LEN, &c); SAFEALIGN_SET_UINT32(&buf[c], - htobe32(cert_pub_key->u.rsa.publicExponent.len), &c); + htobe32(cert_pub_key->u.rsa.publicExponent.len + + exponent_prefix_len), &c); + if (exponent_prefix_len == 1) { + SAFEALIGN_SETMEM_VALUE(&buf[c], '\0', unsigned char, &c); + } safealign_memcpy(&buf[c], cert_pub_key->u.rsa.publicExponent.data, cert_pub_key->u.rsa.publicExponent.len, &c); - /* Looks like nss drops the leading 00 which afaik is added to make sure - * the bigint is handled as positive number */ - /* TODO: make a better check if 00 must be added or not, e.g. ... & 0x80) - */ SAFEALIGN_SET_UINT32(&buf[c], - htobe32(cert_pub_key->u.rsa.modulus.len + 1 ), &c); - SAFEALIGN_SETMEM_VALUE(&buf[c], '\0', unsigned char, &c); + htobe32(cert_pub_key->u.rsa.modulus.len + + modulus_prefix_len ), &c); + if (modulus_prefix_len == 1) { + SAFEALIGN_SETMEM_VALUE(&buf[c], '\0', unsigned char, &c); + } safealign_memcpy(&buf[c], cert_pub_key->u.rsa.modulus.data, cert_pub_key->u.rsa.modulus.len, &c); -- 2.1.0
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
