From 0b4e9cf976438f0a6df7719518d1b1bb96c2caca Mon Sep 17 00:00:00 2001
From: Egbert Eich <eich@suse.com>
Date: Wed, 7 Dec 2022 23:14:40 +0100
Subject: Compound datatypes may not have members of size 0 (#2243)
* Compound datatypes may not have members of size 0
A member size of 0 may lead to an FPE later on as reported in
CVE-2021-46244. To avoid this, check for this as soon as the
member is decoded.
This should probably be done in H5O_dtype_decode_helper() already,
however it is not clear whether all sizes are expected to be != 0.
This fixes CVE-2021-46244 / Bug #2242.
Signed-off-by: Egbert Eich <eich@suse.com>
* Rework error recovery code in H5O__dtype_decode_helper() and
H5O__dtype_decode().
* Format changes for src/H5Odtype.c.
Signed-off-by: Egbert Eich <eich@suse.com>
Co-authored-by: Neil Fortner <nfortne2@hdfgroup.org>
Co-authored-by: Larry Knox <lrknox@hdfgroup.org>
---
release_docs/RELEASE.txt | 39 +++++++-----
src/H5Odtype.c | 155 ++++++++++++++++++++++++++++++-----------------
2 files changed, 125 insertions(+), 69 deletions(-)
diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index c0d5c36..b12068c 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -166,24 +166,11 @@ New Features
Support for new platforms, languages and compilers
==================================================
-
-
-
+
Bug Fixes since HDF5-1.13.3 release
===================================
Library
-------
- - Fix CVE-2021-46242 / GHSA-x9pw-hh7v-wjpf
-
- When evicting driver info block, NULL the corresponding entry.
-
- Since H5C_expunge_entry() called (from H5AC_expunge_entry()) sets the flag
- H5C__FLUSH_INVALIDATE_FLAG, the driver info block will be freed. NULLing
- the pointer in f->shared->drvinfo will prevent use-after-free when it is
- used in other functions (like H5F__dest()) - as other places will check
- whether the pointer is initialized before using its value.
-
- (EFE - 2022/09/29 GH-2254)
-
- Fix CVE-2018-13867 / GHSA-j8jr-chrh-qfrf
Validate location (offset) of the accumulated metadata when comparing.
@@ -212,6 +199,17 @@ Bug Fixes since HDF5-1.13.3 release
(EFE - 2022/10/09 GH-2233)
+ - CVE-2021-46244 / GHSA-vrxh-5gxg-rmhm
+
+ Compound datatypes may not have members of size 0
+
+ A member size of 0 may lead to an FPE later on as reported in
+ CVE-2021-46244. To avoid this, check for this as soon as the
+ member is decoded.
+
+ (EFE - 2022/10/05 GEH-2242)
+
+
- Fix CVE-2021-45830 / GHSA-5h2h-fjjr-x9m2
Make H5O__fsinfo_decode() more resilient to out-of-bound reads.
@@ -225,6 +223,18 @@ Bug Fixes since HDF5-1.13.3 release
(EFE - 2022/10/05 GH-2228)
+ - Fix CVE-2021-46242 / GHSA-x9pw-hh7v-wjpf
+
+ When evicting driver info block, NULL the corresponding entry.
+
+ Since H5C_expunge_entry() called (from H5AC_expunge_entry()) sets the flag
+ H5C__FLUSH_INVALIDATE_FLAG, the driver info block will be freed. NULLing
+ the pointer in f->shared->drvinfo will prevent use-after-free when it is
+ used in other functions (like H5F__dest()) - as other places will check
+ whether the pointer is initialized before using its value.
+
+ (EFE - 2022/09/29 GH-2254)
+
- Fix CVE-2021-45833 / GHSA-x57p-jwp6-4v79
Report error if dimensions of chunked storage in data layout < 2
@@ -264,6 +274,7 @@ Bug Fixes since HDF5-1.13.3 release
(EFE - 2022/09/27 HDFFV-10589, GH-2226)
+
Java Library
------------
-
diff --git a/src/H5Odtype.c b/src/H5Odtype.c
index 870aeac..fd211af 100644
--- a/src/H5Odtype.c
+++ b/src/H5Odtype.c
@@ -250,11 +250,11 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
break;
case H5T_COMPOUND: {
+ unsigned nmembs; /* Number of compound members */
unsigned offset_nbytes; /* Size needed to encode member offsets */
size_t max_memb_pos = 0; /* Maximum member covered, so far */
unsigned max_version = 0; /* Maximum member version */
unsigned upgrade_to = 0; /* Version number we can "soft" upgrade to */
- unsigned j;
/* Compute the # of bytes required to store a member offset */
offset_nbytes = H5VM_limit_enc_size((uint64_t)dt->shared->size);
@@ -262,17 +262,18 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
/*
* Compound datatypes...
*/
- dt->shared->u.compnd.nmembs = flags & 0xffff;
- if (dt->shared->u.compnd.nmembs == 0)
- HGOTO_ERROR(H5E_DATATYPE, H5E_BADVALUE, FAIL, "invalid number of members: %u",
- dt->shared->u.compnd.nmembs)
- dt->shared->u.compnd.nalloc = dt->shared->u.compnd.nmembs;
- dt->shared->u.compnd.memb =
- (H5T_cmemb_t *)H5MM_calloc(dt->shared->u.compnd.nalloc * sizeof(H5T_cmemb_t));
- dt->shared->u.compnd.memb_size = 0;
- if (NULL == dt->shared->u.compnd.memb)
+ nmembs = flags & 0xffff;
+ if (nmembs == 0)
+ HGOTO_ERROR(H5E_DATATYPE, H5E_BADVALUE, FAIL, "invalid number of members: %u", nmembs)
+ if (NULL ==
+ (dt->shared->u.compnd.memb = (H5T_cmemb_t *)H5MM_calloc(nmembs * sizeof(H5T_cmemb_t))))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "memory allocation failed")
- for (i = 0; i < dt->shared->u.compnd.nmembs; i++) {
+ dt->shared->u.compnd.nalloc = nmembs;
+
+ HDassert(dt->shared->u.compnd.memb_size == 0);
+
+ for (dt->shared->u.compnd.nmembs = 0; dt->shared->u.compnd.nmembs < nmembs;
+ dt->shared->u.compnd.nmembs++) {
unsigned ndims = 0; /* Number of dimensions of the array field */
htri_t can_upgrade; /* Whether we can upgrade this type's version */
hsize_t dim[H5O_LAYOUT_NDIMS]; /* Dimensions of the array */
@@ -280,7 +281,10 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
H5T_t *temp_type; /* Temporary pointer to the field's datatype */
/* Decode the field name */
- dt->shared->u.compnd.memb[i].name = H5MM_xstrdup((const char *)*pp);
+ if (NULL == (dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xstrdup((const char *)*pp)))
+ HGOTO_ERROR(H5E_RESOURCE, H5E_CANTCOPY, FAIL,
+ "can't duplicate compound member name string")
/* Version 3 of the datatype message eliminated the padding to multiple of 8 bytes */
if (version >= H5O_DTYPE_VERSION_3)
@@ -293,9 +297,10 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
/* Decode the field offset */
/* (starting with version 3 of the datatype message, use the minimum # of bytes required) */
if (version >= H5O_DTYPE_VERSION_3)
- UINT32DECODE_VAR(*pp, dt->shared->u.compnd.memb[i].offset, offset_nbytes)
+ UINT32DECODE_VAR(*pp, dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset,
+ offset_nbytes)
else
- UINT32DECODE(*pp, dt->shared->u.compnd.memb[i].offset)
+ UINT32DECODE(*pp, dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset)
/* Older versions of the library allowed a field to have
* intrinsic 'arrayness'. Newer versions of the library
@@ -305,8 +310,11 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
ndims = *(*pp)++;
/* Check that ndims is valid */
- if (ndims > 4)
+ if (ndims > 4) {
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
HGOTO_ERROR(H5E_DATATYPE, H5E_BADTYPE, FAIL, "invalid number of dimensions for array")
+ }
*pp += 3; /*reserved bytes */
@@ -317,22 +325,35 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
*pp += 4;
/* Decode array dimension sizes */
- for (j = 0; j < 4; j++)
- UINT32DECODE(*pp, dim[j]);
+ for (i = 0; i < 4; i++)
+ UINT32DECODE(*pp, dim[i]);
} /* end if */
/* Allocate space for the field's datatype */
- if (NULL == (temp_type = H5T__alloc()))
+ if (NULL == (temp_type = H5T__alloc())) {
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed")
+ }
/* Decode the field's datatype information */
if ((can_upgrade = H5O__dtype_decode_helper(ioflags, pp, temp_type)) < 0) {
- for (j = 0; j <= i; j++)
- H5MM_xfree(dt->shared->u.compnd.memb[j].name);
- H5MM_xfree(dt->shared->u.compnd.memb);
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
+ if (H5T_close_real(temp_type) < 0)
+ HDONE_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, FAIL, "can't release datatype info")
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTDECODE, FAIL, "unable to decode member type")
} /* end if */
+ /* Check for invalid member size */
+ if (temp_type->shared->size == 0) {
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
+ if (H5T_close_real(temp_type) < 0)
+ HDONE_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, FAIL, "can't release datatype info")
+ HGOTO_ERROR(H5E_OHDR, H5E_BADVALUE, FAIL, "invalid field size in member type")
+ }
+
/* Upgrade the version if we can and it is necessary */
if (can_upgrade && temp_type->shared->version > version) {
upgrade_to = temp_type->shared->version;
@@ -347,15 +368,21 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
if (ndims > 0) {
/* Create the array datatype for the field */
if ((array_dt = H5T__array_create(temp_type, ndims, dim)) == NULL) {
- for (j = 0; j <= i; j++)
- H5MM_xfree(dt->shared->u.compnd.memb[j].name);
- H5MM_xfree(dt->shared->u.compnd.memb);
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
+ if (H5T_close_real(temp_type) < 0)
+ HDONE_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, FAIL,
+ "can't release datatype info")
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL,
"unable to create array datatype")
} /* end if */
/* Close the base type for the array */
- (void)H5T_close_real(temp_type);
+ if (H5T_close_real(temp_type) < 0) {
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name =
+ H5MM_xfree(dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].name);
+ HGOTO_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, FAIL, "can't release datatype info")
+ }
/* Make the array type the type that is set for the field */
temp_type = array_dt;
@@ -387,26 +414,34 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
dt->shared->force_conv = TRUE;
/* Member size */
- dt->shared->u.compnd.memb[i].size = temp_type->shared->size;
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].size = temp_type->shared->size;
dt->shared->u.compnd.memb_size += temp_type->shared->size;
/* Set the field datatype (finally :-) */
- dt->shared->u.compnd.memb[i].type = temp_type;
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].type = temp_type;
/* Check if this field overlaps with a prior field */
/* (probably indicates that the file is corrupt) */
- if (i > 0 && dt->shared->u.compnd.memb[i].offset < max_memb_pos) {
- for (j = 0; j < i; j++)
- if (dt->shared->u.compnd.memb[i].offset >= dt->shared->u.compnd.memb[j].offset &&
- dt->shared->u.compnd.memb[i].offset <
- (dt->shared->u.compnd.memb[j].offset + dt->shared->u.compnd.memb[j].size))
+ if (dt->shared->u.compnd.nmembs > 0 &&
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset < max_memb_pos) {
+ for (i = 0; i < dt->shared->u.compnd.nmembs; i++)
+ if ((dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset >=
+ dt->shared->u.compnd.memb[i].offset &&
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset <
+ (dt->shared->u.compnd.memb[i].offset + dt->shared->u.compnd.memb[i].size)) ||
+ (dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset <
+ dt->shared->u.compnd.memb[i].offset &&
+ (dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset +
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].size) >
+ dt->shared->u.compnd.memb[i].offset))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTDECODE, FAIL,
"member overlaps with previous member")
} /* end if */
/* Update the maximum member position covered */
- max_memb_pos = MAX(max_memb_pos,
- (dt->shared->u.compnd.memb[i].offset + dt->shared->u.compnd.memb[i].size));
+ max_memb_pos =
+ MAX(max_memb_pos, (dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].offset +
+ dt->shared->u.compnd.memb[dt->shared->u.compnd.nmembs].size));
} /* end for */
/* Check if the compound type is packed */
@@ -461,13 +496,15 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "invalid datatype location")
break;
- case H5T_ENUM:
+ case H5T_ENUM: {
+ unsigned nmembs;
+
/*
* Enumeration datatypes...
*/
- dt->shared->u.enumer.nmembs = dt->shared->u.enumer.nalloc = flags & 0xffff;
+ nmembs = flags & 0xffff;
if (NULL == (dt->shared->parent = H5T__alloc()))
- HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed")
+ HGOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, FAIL, "can't allocate parent datatype")
if (H5O__dtype_decode_helper(ioflags, pp, dt->shared->parent) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTDECODE, FAIL, "unable to decode parent datatype")
if (dt->shared->parent->shared->size != dt->shared->size)
@@ -477,15 +514,19 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
* enum itself. */
H5O_DTYPE_CHECK_VERSION(dt, version, dt->shared->parent->shared->version, ioflags, "enum", FAIL)
- if (NULL == (dt->shared->u.enumer.name =
- (char **)H5MM_calloc(dt->shared->u.enumer.nalloc * sizeof(char *))) ||
- NULL == (dt->shared->u.enumer.value = (uint8_t *)H5MM_calloc(
- dt->shared->u.enumer.nalloc * dt->shared->parent->shared->size)))
- HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed")
+ /* Allocate name and value arrays */
+ if (NULL == (dt->shared->u.enumer.name = (char **)H5MM_calloc(nmembs * sizeof(char *))) ||
+ NULL == (dt->shared->u.enumer.value =
+ (uint8_t *)H5MM_calloc(nmembs * dt->shared->parent->shared->size)))
+ HGOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, FAIL, "memory allocation failed")
+ dt->shared->u.enumer.nalloc = nmembs;
/* Names */
- for (i = 0; i < dt->shared->u.enumer.nmembs; i++) {
- dt->shared->u.enumer.name[i] = H5MM_xstrdup((const char *)*pp);
+ for (dt->shared->u.enumer.nmembs = 0; dt->shared->u.enumer.nmembs < nmembs;
+ dt->shared->u.enumer.nmembs++) {
+ if (NULL == (dt->shared->u.enumer.name[dt->shared->u.enumer.nmembs] =
+ H5MM_xstrdup((const char *)*pp)))
+ HGOTO_ERROR(H5E_RESOURCE, H5E_CANTCOPY, FAIL, "can't duplicate enum name string")
/* Version 3 of the datatype message eliminated the padding to multiple of 8 bytes */
if (version >= H5O_DTYPE_VERSION_3)
@@ -495,12 +536,12 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
/* Advance multiple of 8 w/ null terminator */
*pp += ((HDstrlen((const char *)*pp) + 8) / 8) * 8;
} /* end for */
+ HDassert(dt->shared->u.enumer.nmembs == nmembs);
/* Values */
- H5MM_memcpy(dt->shared->u.enumer.value, *pp,
- dt->shared->u.enumer.nmembs * dt->shared->parent->shared->size);
- *pp += dt->shared->u.enumer.nmembs * dt->shared->parent->shared->size;
- break;
+ H5MM_memcpy(dt->shared->u.enumer.value, *pp, nmembs * dt->shared->parent->shared->size);
+ *pp += nmembs * dt->shared->parent->shared->size;
+ } break;
case H5T_VLEN: /* Variable length datatypes... */
/* Set the type of VL information, either sequence or string */
@@ -578,14 +619,12 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
} /* end switch */
done:
+ /* Cleanup on error */
if (ret_value < 0)
- if (dt != NULL) {
- if (dt->shared != NULL) {
- HDassert(!dt->shared->owned_vol_obj);
- dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
- } /* end if */
- dt = H5FL_FREE(H5T_t, dt);
- } /* end if */
+ /* Release (reset) dt but do not free it - leave it as an empty datatype as was the case on
+ * function entry */
+ if (H5T__free(dt) < 0)
+ HDONE_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, FAIL, "can't release datatype info")
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5O__dtype_decode_helper() */
@@ -1142,6 +1181,12 @@ H5O__dtype_decode(H5F_t H5_ATTR_UNUSED *f, H5O_t H5_ATTR_UNUSED *open_oh, unsign
ret_value = dt;
done:
+ /* Cleanup on error */
+ if (!ret_value)
+ /* Free dt */
+ if (H5T_close_real(dt) < 0)
+ HDONE_ERROR(H5E_DATATYPE, H5E_CANTRELEASE, NULL, "can't release datatype info")
+
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5O__dtype_decode() */
--
cgit v0.12