From 82e78bf1dc05a36a9f4e4f15919566f0c4453e2a Mon Sep 17 00:00:00 2001 From: jsing Date: Tue, 1 Feb 2022 17:13:10 +0000 Subject: [PATCH] Revise signer callback interface. The current design of tls_sign_cb provides a pointer to a buffer where the signature needs to be copied, however it fails to provide a length which could result in buffer overwrites. Furthermore, tls_signer_sign() is designed such that it allocates and returns ownership to the caller. Revise tls_sign_cb so that the called function is expected to allocate a buffer, returning ownership of the buffer (along with its length) to the caller of the callback. This makes it far easier (and safer) to implement a tls_sign_cb callback, plus tls_signer_sign can be directly plugged in (with an appropriate cast). While here, rename and reorder some arguments - while we will normally sign a digest, there is no requirement for this to be the case hence use 'input' and 'input_len'. Move padding (an input) before the outputs and add some additional bounds/return value checks. This is technically an API/ABI break that would need a libtls major bump, however since nothing is using the signer interface (outside of regress), we'll ride the original minor bump. With input from tb@ ok inoguchi@ tb@ --- lib/libtls/tls.h | 14 +-- lib/libtls/tls_signer.c | 197 ++++++++++++++++++++++++---------------- 2 files changed, 128 insertions(+), 83 deletions(-) diff --git a/lib/libtls/tls.h b/lib/libtls/tls.h index 22f04f40231..91166bf9a77 100644 --- a/lib/libtls/tls.h +++ b/lib/libtls/tls.h @@ -1,4 +1,4 @@ -/* $OpenBSD: tls.h,v 1.59 2022/01/25 21:51:24 eric Exp $ */ +/* $OpenBSD: tls.h,v 1.60 2022/02/01 17:13:10 jsing Exp $ */ /* * Copyright (c) 2014 Joel Sing * @@ -79,9 +79,9 @@ typedef ssize_t (*tls_read_cb)(struct tls *_ctx, void *_buf, size_t _buflen, void *_cb_arg); typedef ssize_t (*tls_write_cb)(struct tls *_ctx, const void *_buf, size_t _buflen, void *_cb_arg); -typedef int (*tls_sign_cb)(void *_cb_arg, const char *_hash, - const uint8_t *_dgst, size_t _dgstlen, uint8_t *_psig, size_t *_psiglen, - int _padding); +typedef int (*tls_sign_cb)(void *_cb_arg, const char *_pubkey_hash, + const uint8_t *_input, size_t _input_len, int _padding_type, + uint8_t **_out_signature, size_t *_out_signature_len); int tls_init(void); @@ -224,9 +224,9 @@ int tls_signer_add_keypair_file(struct tls_signer *_signer, const char *_cert_file, const char *_key_file); int tls_signer_add_keypair_mem(struct tls_signer *_signer, const uint8_t *_cert, size_t _cert_len, const uint8_t *_key, size_t _key_len); -int tls_signer_sign(struct tls_signer *_signer, const char *_hash, - const uint8_t *_dgst, size_t _dgstlen, uint8_t **_psig, size_t *_psiglen, - int _padding); +int tls_signer_sign(struct tls_signer *_signer, const char *_pubkey_hash, + const uint8_t *_input, size_t _input_len, int _padding_type, + uint8_t **_out_signature, size_t *_out_signature_len); #ifdef __cplusplus } diff --git a/lib/libtls/tls_signer.c b/lib/libtls/tls_signer.c index ca7e72f4ca4..d6429762e95 100644 --- a/lib/libtls/tls_signer.c +++ b/lib/libtls/tls_signer.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls_signer.c,v 1.2 2022/01/29 02:03:19 inoguchi Exp $ */ +/* $OpenBSD: tls_signer.c,v 1.3 2022/02/01 17:13:10 jsing Exp $ */ /* * Copyright (c) 2021 Eric Faurot * @@ -17,7 +17,9 @@ #include +#include #include +#include #include "tls.h" #include "tls_internal.h" @@ -178,83 +180,94 @@ tls_signer_add_keypair_file(struct tls_signer *signer, const char *cert_file, static int tls_sign_rsa(struct tls_signer *signer, struct tls_signer_key *skey, - const uint8_t *dgst, size_t dgstlen, uint8_t **psig, size_t *psiglen, - int padding) + const uint8_t *input, size_t input_len, int padding_type, + uint8_t **out_signature, size_t *out_signature_len) { + int rsa_size, signature_len; + char *signature = NULL; - char *buf; - int siglen, r; + *out_signature = NULL; + *out_signature_len = 0; - *psig = NULL; - *psiglen = 0; - - siglen = RSA_size(skey->rsa); - if (siglen <= 0) { - tls_error_setx(&signer->error, "incorrect RSA_size: %d", - siglen); + if (input_len > INT_MAX) { + tls_error_setx(&signer->error, "input too large"); return (-1); } - - if ((buf = malloc(siglen)) == NULL) { - tls_error_set(&signer->error, "RSA sign"); + if ((rsa_size = RSA_size(skey->rsa)) <= 0) { + tls_error_setx(&signer->error, "invalid RSA size: %d", + rsa_size); + return (-1); + } + if ((signature = calloc(1, rsa_size)) == NULL) { + tls_error_set(&signer->error, "RSA signature"); return (-1); } - r = RSA_private_encrypt((int)dgstlen, dgst, buf, skey->rsa, padding); - if (r == -1) { - tls_error_setx(&signer->error, "RSA_private_encrypt failed"); - free(buf); + if ((signature_len = RSA_private_encrypt((int)input_len, input, + signature, skey->rsa, padding_type)) <= 0) { + /* XXX - include further details from libcrypto. */ + tls_error_setx(&signer->error, "RSA signing failed"); + free(signature); return (-1); } - *psig = buf; - *psiglen = (size_t)r; + *out_signature = signature; + *out_signature_len = (size_t)signature_len; return (0); } static int tls_sign_ecdsa(struct tls_signer *signer, struct tls_signer_key *skey, - const uint8_t *dgst, size_t dgstlen, uint8_t **psig, size_t *psiglen) + const uint8_t *input, size_t input_len, int padding_type, + uint8_t **out_signature, size_t *out_signature_len) { - unsigned char *sig; - unsigned int siglen; + unsigned char *signature; + int signature_len; - *psig = NULL; - *psiglen = 0; + *out_signature = NULL; + *out_signature_len = 0; - siglen = ECDSA_size(skey->ecdsa); - if (siglen == 0) { - tls_error_setx(&signer->error, "incorrect ECDSA_size: %u", - siglen); + if (input_len > INT_MAX) { + tls_error_setx(&signer->error, "digest too large"); return (-1); } - if ((sig = malloc(siglen)) == NULL) { - tls_error_set(&signer->error, "ECDSA sign"); + if ((signature_len = ECDSA_size(skey->ecdsa)) <= 0) { + tls_error_setx(&signer->error, "invalid ECDSA size: %d", + signature_len); + return (-1); + } + if ((signature = calloc(1, signature_len)) == NULL) { + tls_error_set(&signer->error, "ECDSA signature"); return (-1); } - if (!ECDSA_sign(0, dgst, dgstlen, sig, &siglen, skey->ecdsa)) { - tls_error_setx(&signer->error, "ECDSA_sign failed"); - free(sig); + if (!ECDSA_sign(0, input, input_len, signature, &signature_len, + skey->ecdsa)) { + /* XXX - include further details from libcrypto. */ + tls_error_setx(&signer->error, "ECDSA signing failed"); + free(signature); return (-1); } - *psig = sig; - *psiglen = siglen; + *out_signature = signature; + *out_signature_len = signature_len; return (0); } int -tls_signer_sign(struct tls_signer *signer, const char *hash, - const uint8_t *dgst, size_t dgstlen, uint8_t **psig, size_t *psiglen, - int padding) +tls_signer_sign(struct tls_signer *signer, const char *pubkey_hash, + const uint8_t *input, size_t input_len, int padding_type, + uint8_t **out_signature, size_t *out_signature_len) { struct tls_signer_key *skey; + *out_signature = NULL; + *out_signature_len = 0; + for (skey = signer->keys; skey; skey = skey->next) - if (!strcmp(hash, skey->hash)) + if (!strcmp(pubkey_hash, skey->hash)) break; if (skey == NULL) { @@ -263,38 +276,58 @@ tls_signer_sign(struct tls_signer *signer, const char *hash, } if (skey->rsa != NULL) - return tls_sign_rsa(signer, skey, dgst, dgstlen, psig, psiglen, - padding); + return tls_sign_rsa(signer, skey, input, input_len, + padding_type, out_signature, out_signature_len); if (skey->ecdsa != NULL) - return tls_sign_ecdsa(signer, skey, dgst, dgstlen, psig, psiglen); + return tls_sign_ecdsa(signer, skey, input, input_len, + padding_type, out_signature, out_signature_len); tls_error_setx(&signer->error, "unknown key type"); + return (-1); } static int -tls_rsa_priv_enc(int srclen, const unsigned char *src, unsigned char *to, - RSA *rsa, int padding) +tls_rsa_priv_enc(int from_len, const unsigned char *from, unsigned char *to, + RSA *rsa, int rsa_padding) { struct tls_config *config; - const char *hash; - size_t tolen; + uint8_t *signature = NULL; + size_t signature_len = 0; + const char *pubkey_hash; + + /* + * This function is called via RSA_private_encrypt() and has to conform + * to its calling convention/signature. The caller is required to + * provide a 'to' buffer of at least RSA_size() bytes. + */ - hash = RSA_get_ex_data(rsa, 0); + pubkey_hash = RSA_get_ex_data(rsa, 0); config = RSA_get_ex_data(rsa, 1); - if (hash == NULL || config == NULL) - return (-1); + if (pubkey_hash == NULL || config == NULL) + goto err; - if (config->sign_cb(config->sign_cb_arg, hash, (const uint8_t *)src, - srclen, (uint8_t *)to, &tolen, padding) == -1) - return (-1); + if (from_len < 0) + goto err; - if (tolen > INT_MAX) - return (-1); + if (config->sign_cb(config->sign_cb_arg, pubkey_hash, from, from_len, + rsa_padding, &signature, &signature_len) == -1) + goto err; + + if (signature_len > INT_MAX || (int)signature_len > RSA_size(rsa)) + goto err; + + memcpy(to, signature, signature_len); + free(signature); - return ((int)tolen); + return ((int)signature_len); + + err: + free(signature); + + return (-1); } RSA_METHOD * @@ -324,30 +357,42 @@ tls_ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv, const BIGNUM *rp, EC_KEY *eckey) { struct tls_config *config; - ECDSA_SIG *sig = NULL; - const unsigned char *tsigbuf; - const char *hash; - char *sigbuf; - size_t siglen; - - hash = ECDSA_get_ex_data(eckey, 0); + ECDSA_SIG *ecdsa_sig = NULL; + uint8_t *signature = NULL; + size_t signature_len = 0; + const unsigned char *p; + const char *pubkey_hash; + + /* + * This function is called via ECDSA_do_sign_ex() and has to conform + * to its calling convention/signature. + */ + + pubkey_hash = ECDSA_get_ex_data(eckey, 0); config = ECDSA_get_ex_data(eckey, 1); - if (hash == NULL || config == NULL) - return (NULL); + if (pubkey_hash == NULL || config == NULL) + goto err; - siglen = ECDSA_size(eckey); - if ((sigbuf = malloc(siglen)) == NULL) - return (NULL); + if (dgst_len < 0) + goto err; - if (config->sign_cb(config->sign_cb_arg, hash, dgst, dgst_len, sigbuf, - &siglen, 0) != -1) { - tsigbuf = sigbuf; - sig = d2i_ECDSA_SIG(NULL, &tsigbuf, siglen); - } - free(sigbuf); + if (config->sign_cb(config->sign_cb_arg, pubkey_hash, dgst, dgst_len, + 0, &signature, &signature_len) == -1) + goto err; + + p = signature; + if ((ecdsa_sig = d2i_ECDSA_SIG(NULL, &p, signature_len)) == NULL) + goto err; + + free(signature); + + return (ecdsa_sig); + + err: + free(signature); - return (sig); + return (NULL); } ECDSA_METHOD * -- 2.20.1