Skip to content

Commit

Permalink
Updating erroneous documentation for BIO_get_mem_data and subsequent …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@smittals2
smittals2 authored Aug 23, 2024
1 parent 398ae9c commit d77ca7b
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 24 deletions.
10 changes: 7 additions & 3 deletions crypto/bio/bio_mem.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
50 changes: 35 additions & 15 deletions crypto/ocsp/ocsp_http.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,22 +147,24 @@ 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;
}
return 0;
}

// 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) {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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++;
}
Expand All @@ -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;
Expand All @@ -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));
Expand Down
14 changes: 8 additions & 6 deletions include/openssl/bio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down

0 comments on commit d77ca7b

Please sign in to comment.