From d77ca7b7996e9b6b9184b212297b9f69d59fa96e Mon Sep 17 00:00:00 2001 From: Shubham Mittal <107728331+smittals2@users.noreply.github.com> Date: Fri, 23 Aug 2024 15:21:38 -0700 Subject: [PATCH] Updating erroneous documentation for BIO_get_mem_data and subsequent usage (#1752) ### Description of changes: Coverity scan flagged the usage of BIO_get_mem_data. The documentation for this function was incorrect. The documentation stated it would return the length of the data or 0 for failure, but the return value could in fact be negative (this function is a macro to BIO_ctrl which may return -2). BIO_get_mem_data is subsequently used in ocsp_http.c without a check for the case of -2 which may lead to unexpected behavior. This PR updates the documentation for BIO_get_mem_data and replaces BIO_get_mem_data usage in ocsp_http.c with BIO_mem_contents which has less edge cases. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- crypto/bio/bio_mem.c | 10 ++++++--- crypto/ocsp/ocsp_http.c | 50 ++++++++++++++++++++++++++++------------- include/openssl/bio.h | 14 +++++++----- 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/crypto/bio/bio_mem.c b/crypto/bio/bio_mem.c index f4c01d1dff..54243f9df4 100644 --- a/crypto/bio/bio_mem.c +++ b/crypto/bio/bio_mem.c @@ -282,13 +282,17 @@ const BIO_METHOD *BIO_s_mem(void) { return &mem_method; } int BIO_mem_contents(const BIO *bio, const uint8_t **out_contents, size_t *out_len) { const BUF_MEM *b; - if (bio->method != &mem_method) { + if (!bio || bio->method != &mem_method) { return 0; } b = (BUF_MEM *)bio->ptr; - *out_contents = (uint8_t *)b->data; - *out_len = b->length; + if (out_contents != NULL) { + *out_contents = (uint8_t *)b->data; + } + if(out_len) { + *out_len = b->length; + } return 1; } diff --git a/crypto/ocsp/ocsp_http.c b/crypto/ocsp/ocsp_http.c index 284b28cc01..5c43c53b1b 100644 --- a/crypto/ocsp/ocsp_http.c +++ b/crypto/ocsp/ocsp_http.c @@ -147,12 +147,13 @@ static int parse_http_line(char *line) { // read, |OCSP_REQ_CTX_nbio| will finish in the state of OHS_DONE. // |OCSP_REQ_CTX_nbio| will not return 1 until we reach OHS_DONE. int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { - int ret, data_len; + int ret, tmp_data_len; + size_t data_len; const unsigned char *data; next_io: if (!(rctx->state & OHS_NOREAD)) { - data_len = BIO_read(rctx->io, rctx->iobuf, rctx->iobuflen); - if (data_len <= 0) { + tmp_data_len = BIO_read(rctx->io, rctx->iobuf, rctx->iobuflen); + if (tmp_data_len <= 0) { if (BIO_should_retry(rctx->io)) { return -1; } @@ -160,9 +161,10 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { } // Write data to memory BIO. - if (BIO_write(rctx->mem, rctx->iobuf, data_len) != data_len) { + if (BIO_write(rctx->mem, rctx->iobuf, tmp_data_len) != tmp_data_len) { return 0; } + data_len = (size_t)tmp_data_len; } switch (rctx->state) { @@ -176,12 +178,19 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { OPENSSL_FALLTHROUGH; case OHS_ASN1_WRITE_INIT: - rctx->asn1_len = BIO_get_mem_data(rctx->mem, NULL); + if(!BIO_mem_contents(rctx->mem, NULL, &data_len)) { + rctx->state = OHS_ERROR; + return 0; + } + rctx->asn1_len = data_len; rctx->state = OHS_ASN1_WRITE; OPENSSL_FALLTHROUGH; case OHS_ASN1_WRITE: - data_len = BIO_get_mem_data(rctx->mem, &data); + if(!BIO_mem_contents(rctx->mem, &data, &data_len)) { + rctx->state = OHS_ERROR; + return 0; + } int write_len = BIO_write(rctx->io, data + (data_len - rctx->asn1_len), (int)rctx->asn1_len); @@ -227,17 +236,20 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { // Due to strange memory BIO behaviour with BIO_gets we have to // check there's a complete line in there before calling BIO_gets // or we'll just get a partial read. - data_len = BIO_get_mem_data(rctx->mem, &data); + if(!BIO_mem_contents(rctx->mem, &data, &data_len)) { + rctx->state = OHS_ERROR; + return 0; + } if ((data_len <= 0) || !memchr(data, '\n', data_len)) { - if (data_len >= rctx->iobuflen) { + if (data_len >= (size_t)rctx->iobuflen) { rctx->state = OHS_ERROR; return 0; } goto next_io; } - data_len = BIO_gets(rctx->mem, (char *)rctx->iobuf, rctx->iobuflen); + tmp_data_len = BIO_gets(rctx->mem, (char *)rctx->iobuf, rctx->iobuflen); - if (data_len <= 0) { + if (tmp_data_len <= 0) { if (BIO_should_retry(rctx->mem)) { goto next_io; } @@ -246,10 +258,11 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { } // Don't allow excessive lines. - if (data_len >= rctx->iobuflen) { + if (tmp_data_len >= rctx->iobuflen) { rctx->state = OHS_ERROR; return 0; } + data_len = (size_t)tmp_data_len; // First line. if (rctx->state == OHS_FIRSTLINE) { @@ -279,7 +292,10 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { // Now reading ASN1 header: can read at least 2 bytes which is // enough for ASN1 SEQUENCE header and either length field or at // least the length of the length field. - data_len = BIO_get_mem_data(rctx->mem, &data); + if(!BIO_mem_contents(rctx->mem, &data, &data_len)) { + rctx->state = OHS_ERROR; + return 0; + } if (data_len < 2) { goto next_io; } @@ -308,7 +324,7 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { } data++; rctx->asn1_len = 0; - for (int i = 0; i < data_len; i++) { + for (size_t i = 0; i < data_len; i++) { rctx->asn1_len <<= 8; rctx->asn1_len |= *data++; } @@ -324,8 +340,11 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { OPENSSL_FALLTHROUGH; case OHS_ASN1_CONTENT: - data_len = BIO_get_mem_data(rctx->mem, NULL); - if (data_len < (int)rctx->asn1_len) { + if(!BIO_mem_contents(rctx->mem, NULL, &data_len)) { + rctx->state = OHS_ERROR; + return 0; + } + if (data_len < rctx->asn1_len) { goto next_io; } rctx->state = OHS_DONE; @@ -338,6 +357,7 @@ int OCSP_REQ_CTX_nbio(OCSP_REQ_CTX *rctx) { return 0; } + int OCSP_sendreq_nbio(OCSP_RESPONSE **presp, OCSP_REQ_CTX *rctx) { return OCSP_REQ_CTX_nbio_d2i(rctx, (ASN1_VALUE **)presp, ASN1_ITEM_rptr(OCSP_RESPONSE)); diff --git a/include/openssl/bio.h b/include/openssl/bio.h index ceddf66865..730601467a 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h @@ -438,9 +438,11 @@ OPENSSL_EXPORT const BIO_METHOD *BIO_s_mem(void); // don't depend on this in new code. OPENSSL_EXPORT BIO *BIO_new_mem_buf(const void *buf, ossl_ssize_t len); -// BIO_mem_contents sets |*out_contents| to point to the current contents of -// |bio| and |*out_len| to contain the length of that data. It returns one on -// success and zero otherwise. +// BIO_mem_contents sets |*out_contents|, if not null, to point to the current +// contents of|bio| and |*out_len|, if not null, to contain the length of +// that data. +// +// It returns one on success and zero otherwise. OPENSSL_EXPORT int BIO_mem_contents(const BIO *bio, const uint8_t **out_contents, size_t *out_len); @@ -449,9 +451,9 @@ OPENSSL_EXPORT int BIO_mem_contents(const BIO *bio, // and returns the length of the data. Despite being a macro, this function // should always take |char *| as a value and nothing else. // -// WARNING: don't use this, use |BIO_mem_contents|. A return value of zero from -// this function can mean either that it failed or that the memory buffer is -// empty. +// WARNING: don't use this, use |BIO_mem_contents|. A negative return value +// or zero from this function can mean either that it failed or that the +// memory buffer is empty. #define BIO_get_mem_data(bio, contents) BIO_ctrl(bio, BIO_CTRL_INFO, 0, \ (char *)(contents)) // BIO_get_mem_ptr sets |*out| to a BUF_MEM containing the current contents of