From 7e3cacac943d298348d97c8f7f980ca0916378c5 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 29 Aug 2022 14:58:57 -0400 Subject: Update COMP_METHOD size_t-ify the COMP_METHOD structure and functions. Get rid of the non-functional COMP_METHODS and return NULL instead. Reviewed-by: Matt Caswell Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/18186) --- crypto/comp/c_brotli.c | 104 +++++++++++++++++++++++++++++----------------- crypto/comp/c_zlib.c | 43 +++++++++---------- crypto/comp/c_zstd.c | 72 +++++++++++++++++++------------- crypto/comp/comp_lib.c | 7 ++++ crypto/comp/comp_local.h | 12 +++--- doc/man3/COMP_CTX_new.pod | 3 +- ssl/statem/statem_clnt.c | 6 +-- ssl/statem/statem_lib.c | 4 +- test/ssl_old_test.c | 16 ++++--- 9 files changed, 155 insertions(+), 112 deletions(-) diff --git a/crypto/comp/c_brotli.c b/crypto/comp/c_brotli.c index 377ea2b8d0..2d98b63a89 100644 --- a/crypto/comp/c_brotli.c +++ b/crypto/comp/c_brotli.c @@ -22,15 +22,6 @@ COMP_METHOD *COMP_brotli(void); -static COMP_METHOD brotli_method_nobrotli = { - NID_undef, - "(undef)", - NULL, - NULL, - NULL, - NULL, -}; - #ifdef OPENSSL_NO_BROTLI # undef BROTLI_SHARED #else @@ -155,16 +146,16 @@ static void brotli_stateful_finish(COMP_CTX *ctx) } } -static int brotli_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t brotli_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { BROTLI_BOOL done; struct brotli_state *state = ctx->data; size_t in_avail = ilen; size_t out_avail = olen; - if (state == NULL) + if (state == NULL || olen > OSSL_SSIZE_MAX) return -1; if (ilen == 0) @@ -178,35 +169,44 @@ static int brotli_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, * output buffer space */ done = BrotliEncoderCompressStream(state->encoder, BROTLI_OPERATION_FLUSH, - &in_avail, (const uint8_t**)&in, &out_avail, &out, NULL); - if (done == BROTLI_FALSE || in_avail != 0 - || BrotliEncoderHasMoreOutput(state->encoder)) + &in_avail, (const uint8_t**)&in, + &out_avail, &out, NULL); + if (done == BROTLI_FALSE + || in_avail != 0 + || BrotliEncoderHasMoreOutput(state->encoder)) return -1; - return (int)(olen - out_avail); + if (out_avail > olen) + return -1; + return (ossl_ssize_t)(olen - out_avail); } -static int brotli_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t brotli_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { BrotliDecoderResult result; struct brotli_state *state = ctx->data; size_t in_avail = ilen; size_t out_avail = olen; - if (state == NULL) + if (state == NULL || olen > OSSL_SSIZE_MAX) return -1; if (ilen == 0) return 0; - result = BrotliDecoderDecompressStream(state->decoder, &in_avail, (const uint8_t**)&in, &out_avail, &out, NULL); - if (result == BROTLI_DECODER_RESULT_ERROR || in_avail != 0 - || BrotliDecoderHasMoreOutput(state->decoder)) + result = BrotliDecoderDecompressStream(state->decoder, &in_avail, + (const uint8_t**)&in, &out_avail, + &out, NULL); + if (result == BROTLI_DECODER_RESULT_ERROR + || in_avail != 0 + || BrotliDecoderHasMoreOutput(state->decoder)) return -1; - return (int)(olen - out_avail); + if (out_avail > olen) + return -1; + return (ossl_ssize_t)(olen - out_avail); } static COMP_METHOD brotli_stateful_method = { @@ -227,11 +227,12 @@ static void brotli_oneshot_finish(COMP_CTX *ctx) { } -static int brotli_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t brotli_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { size_t out_size = olen; + ossl_ssize_t ret; if (ilen == 0) return 0; @@ -241,14 +242,20 @@ static int brotli_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, &out_size, out) == BROTLI_FALSE) return -1; - return (int)out_size; + if (out_size > OSSL_SSIZE_MAX) + return -1; + ret = (ossl_ssize_t)out_size; + if (ret < 0) + return -1; + return ret; } -static int brotli_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t brotli_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { size_t out_size = olen; + ossl_ssize_t ret; if (ilen == 0) return 0; @@ -256,7 +263,12 @@ static int brotli_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, if (BrotliDecoderDecompress(ilen, in, &out_size, out) != BROTLI_DECODER_RESULT_SUCCESS) return -1; - return (int)out_size; + if (out_size > OSSL_SSIZE_MAX) + return -1; + ret = (ossl_ssize_t)out_size; + if (ret < 0) + return -1; + return ret; } static COMP_METHOD brotli_oneshot_method = { @@ -316,7 +328,7 @@ DEFINE_RUN_ONCE_STATIC(ossl_comp_brotli_init) COMP_METHOD *COMP_brotli(void) { - COMP_METHOD *meth = &brotli_method_nobrotli; + COMP_METHOD *meth = NULL; #ifndef OPENSSL_NO_BROTLI if (RUN_ONCE(&brotli_once, ossl_comp_brotli_init)) @@ -327,7 +339,7 @@ COMP_METHOD *COMP_brotli(void) COMP_METHOD *COMP_brotli_oneshot(void) { - COMP_METHOD *meth = &brotli_method_nobrotli; + COMP_METHOD *meth = NULL; #ifndef OPENSSL_NO_BROTLI if (RUN_ONCE(&brotli_once, ossl_comp_brotli_init)) @@ -492,8 +504,16 @@ static int bio_brotli_read(BIO *b, char *out, int outl) int ret; BIO *next = BIO_next(b); - if (out == NULL || outl <= 0) + if (out == NULL || outl <= 0) { + ERR_raise(ERR_LIB_COMP, ERR_R_PASSED_INVALID_ARGUMENT); return 0; + } +#if INT_MAX > SIZE_MAX + if ((unsigned int)outl > SIZE_MAX) { + ERR_raise(ERR_LIB_COMP, ERR_R_PASSED_INVALID_ARGUMENT); + return 0; + } +#endif ctx = BIO_get_data(b); BIO_clear_retry_flags(b); @@ -555,8 +575,16 @@ static int bio_brotli_write(BIO *b, const char *in, int inl) int ret; BIO *next = BIO_next(b); - if (in == NULL || inl <= 0) + if (in == NULL || inl <= 0) { + ERR_raise(ERR_LIB_COMP, ERR_R_PASSED_INVALID_ARGUMENT); + return 0; + } +#if INT_MAX > SIZE_MAX + if ((unsigned int)inl > SIZE_MAX) { + ERR_raise(ERR_LIB_COMP, ERR_R_PASSED_INVALID_ARGUMENT); return 0; + } +#endif ctx = BIO_get_data(b); if (ctx->encode.done) @@ -576,7 +604,7 @@ static int bio_brotli_write(BIO *b, const char *in, int inl) } /* Obtain input data directly from supplied buffer */ ctx->encode.next_in = (unsigned char *)in; - ctx->encode.avail_in = inl; + ctx->encode.avail_in = (size_t)inl; for (;;) { /* If data in output buffer write it first */ while (ctx->encode.count > 0) { diff --git a/crypto/comp/c_zlib.c b/crypto/comp/c_zlib.c index 426b8b0b74..ba5a8d9ce4 100644 --- a/crypto/comp/c_zlib.c +++ b/crypto/comp/c_zlib.c @@ -20,15 +20,6 @@ COMP_METHOD *COMP_zlib(void); -static COMP_METHOD zlib_method_nozlib = { - NID_undef, - "(undef)", - NULL, - NULL, - NULL, - NULL, -}; - #ifdef OPENSSL_NO_ZLIB # undef ZLIB_SHARED #else @@ -37,12 +28,12 @@ static COMP_METHOD zlib_method_nozlib = { static int zlib_stateful_init(COMP_CTX *ctx); static void zlib_stateful_finish(COMP_CTX *ctx); -static int zlib_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen); -static int zlib_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen); +static ossl_ssize_t zlib_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen); +static ossl_ssize_t zlib_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen); /* memory allocations functions for zlib initialisation */ static void *zlib_zalloc(void *opaque, unsigned int no, unsigned int size) @@ -162,9 +153,9 @@ static void zlib_stateful_finish(COMP_CTX *ctx) OPENSSL_free(state); } -static int zlib_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zlib_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { int err = Z_OK; struct zlib_state *state = ctx->data; @@ -180,12 +171,14 @@ static int zlib_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, err = deflate(&state->ostream, Z_SYNC_FLUSH); if (err != Z_OK) return -1; - return olen - state->ostream.avail_out; + if (state->ostream.avail_out > olen) + return -1; + return (ossl_ssize_t)(olen - state->ostream.avail_out); } -static int zlib_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zlib_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { int err = Z_OK; struct zlib_state *state = ctx->data; @@ -201,7 +194,9 @@ static int zlib_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, err = inflate(&state->istream, Z_SYNC_FLUSH); if (err != Z_OK) return -1; - return olen - state->istream.avail_out; + if (state->istream.avail_out > olen) + return -1; + return (ossl_ssize_t)(olen - state->istream.avail_out); } static CRYPTO_ONCE zlib_once = CRYPTO_ONCE_STATIC_INIT; @@ -245,7 +240,7 @@ DEFINE_RUN_ONCE_STATIC(ossl_comp_zlib_init) COMP_METHOD *COMP_zlib(void) { - COMP_METHOD *meth = &zlib_method_nozlib; + COMP_METHOD *meth = NULL; #ifndef OPENSSL_NO_ZLIB if (RUN_ONCE(&zlib_once, ossl_comp_zlib_init)) diff --git a/crypto/comp/c_zstd.c b/crypto/comp/c_zstd.c index 15b826c589..17dc0f64a4 100644 --- a/crypto/comp/c_zstd.c +++ b/crypto/comp/c_zstd.c @@ -25,15 +25,6 @@ COMP_METHOD *COMP_zstd(void); -static COMP_METHOD zstd_method_nozstd = { - NID_undef, - "(undef)", - NULL, - NULL, - NULL, - NULL, -}; - #ifdef OPENSSL_NO_ZSTD # undef ZSTD_SHARED #else @@ -191,13 +182,14 @@ static void zstd_stateful_finish(COMP_CTX *ctx) } } -static int zstd_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zstd_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { ZSTD_inBuffer inbuf; ZSTD_outBuffer outbuf; size_t ret; + ossl_ssize_t fret; struct zstd_state *state = ctx->data; inbuf.src = in; @@ -215,7 +207,7 @@ static int zstd_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, ret = ZSTD_endStream(state->compressor, &outbuf); if (ZSTD_isError(ret)) return -1; - return outbuf.pos; + goto end; } /* @@ -240,16 +232,23 @@ static int zstd_stateful_compress_block(COMP_CTX *ctx, unsigned char *out, if (ZSTD_isError(ret)) return -1; - return outbuf.pos; + end: + if (outbuf.pos > OSSL_SSIZE_MAX) + return -1; + fret = (ossl_ssize_t)outbuf.pos; + if (fret < 0) + return -1; + return fret; } -static int zstd_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zstd_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { ZSTD_inBuffer inbuf; ZSTD_outBuffer outbuf; size_t ret; + ossl_ssize_t fret; struct zstd_state *state = ctx->data; inbuf.src = in; @@ -276,7 +275,12 @@ static int zstd_stateful_expand_block(COMP_CTX *ctx, unsigned char *out, if (inbuf.pos < inbuf.size) return -1; - return outbuf.pos; + if (outbuf.pos > OSSL_SSIZE_MAX) + return -1; + fret = (ossl_ssize_t)outbuf.pos; + if (fret < 0) + return -1; + return fret; } @@ -298,11 +302,12 @@ static void zstd_oneshot_finish(COMP_CTX *ctx) { } -static int zstd_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zstd_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { size_t out_size; + ossl_ssize_t ret; if (ilen == 0) return 0; @@ -312,14 +317,20 @@ static int zstd_oneshot_compress_block(COMP_CTX *ctx, unsigned char *out, if (ZSTD_isError(out_size)) return -1; - return out_size; + if (out_size > OSSL_SSIZE_MAX) + return -1; + ret = (ossl_ssize_t)out_size; + if (ret < 0) + return -1; + return ret; } -static int zstd_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, - unsigned int olen, unsigned char *in, - unsigned int ilen) +static ossl_ssize_t zstd_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, + size_t olen, unsigned char *in, + size_t ilen) { size_t out_size; + ossl_ssize_t ret; if (ilen == 0) return 0; @@ -329,7 +340,12 @@ static int zstd_oneshot_expand_block(COMP_CTX *ctx, unsigned char *out, if (ZSTD_isError(out_size)) return -1; - return out_size; + if (out_size > OSSL_SSIZE_MAX) + return -1; + ret = (ossl_ssize_t)out_size; + if (ret < 0) + return -1; + return ret; } static COMP_METHOD zstd_oneshot_method = { @@ -387,7 +403,7 @@ DEFINE_RUN_ONCE_STATIC(ossl_comp_zstd_init) COMP_METHOD *COMP_zstd(void) { - COMP_METHOD *meth = &zstd_method_nozstd; + COMP_METHOD *meth = NULL; #ifndef OPENSSL_NO_ZSTD if (RUN_ONCE(&zstd_once, ossl_comp_zstd_init)) @@ -398,7 +414,7 @@ COMP_METHOD *COMP_zstd(void) COMP_METHOD *COMP_zstd_oneshot(void) { - COMP_METHOD *meth = &zstd_method_nozstd; + COMP_METHOD *meth = NULL; #ifndef OPENSSL_NO_ZSTD if (RUN_ONCE(&zstd_once, ossl_comp_zstd_init)) diff --git a/crypto/comp/comp_lib.c b/crypto/comp/comp_lib.c index c5946fecdb..56ca17a7a5 100644 --- a/crypto/comp/comp_lib.c +++ b/crypto/comp/comp_lib.c @@ -19,6 +19,9 @@ COMP_CTX *COMP_CTX_new(COMP_METHOD *meth) { COMP_CTX *ret; + if (meth == NULL) + return NULL; + if ((ret = OPENSSL_zalloc(sizeof(*ret))) == NULL) return NULL; ret->meth = meth; @@ -36,11 +39,15 @@ const COMP_METHOD *COMP_CTX_get_method(const COMP_CTX *ctx) int COMP_get_type(const COMP_METHOD *meth) { + if (meth == NULL) + return NID_undef; return meth->type; } const char *COMP_get_name(const COMP_METHOD *meth) { + if (meth == NULL) + return NULL; return meth->name; } diff --git a/crypto/comp/comp_local.h b/crypto/comp/comp_local.h index acf113e31c..d8be9271a0 100644 --- a/crypto/comp/comp_local.h +++ b/crypto/comp/comp_local.h @@ -12,12 +12,12 @@ struct comp_method_st { const char *name; /* A text string to identify the library */ int (*init) (COMP_CTX *ctx); void (*finish) (COMP_CTX *ctx); - int (*compress) (COMP_CTX *ctx, - unsigned char *out, unsigned int olen, - unsigned char *in, unsigned int ilen); - int (*expand) (COMP_CTX *ctx, - unsigned char *out, unsigned int olen, - unsigned char *in, unsigned int ilen); + ossl_ssize_t (*compress) (COMP_CTX *ctx, + unsigned char *out, size_t olen, + unsigned char *in, size_t ilen); + ossl_ssize_t (*expand) (COMP_CTX *ctx, + unsigned char *out, size_t olen, + unsigned char *in, size_t ilen); }; struct comp_ctx_st { diff --git a/doc/man3/COMP_CTX_new.pod b/doc/man3/COMP_CTX_new.pod index 7e1c8c4a83..d4c0e326bf 100644 --- a/doc/man3/COMP_CTX_new.pod +++ b/doc/man3/COMP_CTX_new.pod @@ -68,8 +68,7 @@ buffer I of size I using the lgorithm specified by I. Methods (B) may be specified by one of these functions. These functions will be available even if their corresponding compression algorithm is not configured -into the OpenSSL library. In such a case, a non-operative method will be returned. -Any compression operations using a non-operative method will fail. +into the OpenSSL library. In such a case, NULL will be returned. =over 4 diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 31f0ebe8e5..303e877282 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -3702,9 +3702,9 @@ CON_FUNC_RETURN tls_construct_client_compressed_certificate(SSL_CONNECTION *sc, } max_length = ossl_calculate_comp_expansion(alg, length); - if (!WPACKET_start_sub_packet_u24(pkt) - || !WPACKET_reserve_bytes(pkt, max_length, NULL) - || (comp = COMP_CTX_new(method)) == NULL) + if ((comp = COMP_CTX_new(method)) == NULL + || !WPACKET_start_sub_packet_u24(pkt) + || !WPACKET_reserve_bytes(pkt, max_length, NULL)) goto err; comp_len = COMP_compress_block(comp, WPACKET_get_curr(pkt), max_length, diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 69e900f90e..9712fb3735 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -2522,12 +2522,12 @@ MSG_PROCESS_RETURN tls13_process_compressed_certificate(SSL_CONNECTION *sc, goto err; } - if (!PACKET_get_net_3_len(pkt, &expected_length) + if ((comp = COMP_CTX_new(method)) == NULL + || !PACKET_get_net_3_len(pkt, &expected_length) || !PACKET_get_net_3_len(pkt, &comp_length) || PACKET_remaining(pkt) != comp_length || !BUF_MEM_grow(buf, expected_length) || !PACKET_buf_init(tmppkt, (unsigned char *)buf->data, expected_length) - || (comp = COMP_CTX_new(method)) == NULL || COMP_expand_block(comp, (unsigned char *)buf->data, expected_length, (unsigned char*)PACKET_data(pkt), comp_length) != (int)expected_length) { SSLfatal(sc, SSL_AD_BAD_CERTIFICATE, SSL_R_BAD_DECOMPRESSION); diff --git a/test/ssl_old_test.c b/test/ssl_old_test.c index beabb7205e..439d0ed8d0 100644 --- a/test/ssl_old_test.c +++ b/test/ssl_old_test.c @@ -1314,17 +1314,15 @@ int main(int argc, char *argv[]) if (comp == COMP_ZLIB) cm = COMP_zlib(); if (cm != NULL) { - if (COMP_get_type(cm) != NID_undef) { - if (SSL_COMP_add_compression_method(comp, cm) != 0) { - fprintf(stderr, "Failed to add compression method\n"); - ERR_print_errors_fp(stderr); - } - } else { - fprintf(stderr, - "Warning: %s compression not supported\n", - comp == COMP_ZLIB ? "zlib" : "unknown"); + if (SSL_COMP_add_compression_method(comp, cm) != 0) { + fprintf(stderr, "Failed to add compression method\n"); ERR_print_errors_fp(stderr); } + } else { + fprintf(stderr, + "Warning: %s compression not supported\n", + comp == COMP_ZLIB ? "zlib" : "unknown"); + ERR_print_errors_fp(stderr); } ssl_comp_methods = SSL_COMP_get_compression_methods(); n = sk_SSL_COMP_num(ssl_comp_methods); -- cgit v1.2.3