diff --git a/release_docs/CHANGELOG.md b/release_docs/CHANGELOG.md index 323a6f79af8..a08fb1ae3e4 100644 --- a/release_docs/CHANGELOG.md +++ b/release_docs/CHANGELOG.md @@ -203,6 +203,10 @@ The `h5repack` tool now obtains its default low and high library version bounds Fixed a bug where a flag in H5Cimage.c wasn't getting set correctly for release builds of HDF5, leading to incorrect error checking when reconstructing metadata cache entries. +### Hardened decoding of serialized dataspace selections against malformed buffers + + `H5S_select_deserialize()` and the per-selection-type deserialize callbacks (all, hyperslab, none, and point) previously computed the pointer to the last valid buffer byte as `buffer + size - 1` without first checking the buffer size. A buffer shorter than the 4-byte selection-type header, or a zero-length selection-info buffer, would underflow this computation and produce an out-of-bounds end pointer, defeating subsequent overflow checks. The deserialize routines now reject a buffer that is too small to hold the selection type, and they reject an empty selection-info buffer before deriving the end pointer. Hyperslab decoding additionally now rejects a serialized rank of 0 or greater than `H5S_MAX_RANK`. As a companion fix, `H5S__hyper_serialize()` now returns an error when asked to serialize a hyperslab selection on a rank-0 (scalar or null) dataspace, a state that can arise when a dataspace extent is collapsed to a scalar after a hyperslab selection has already been made. + ## Java Library ## Configuration diff --git a/src/H5Sall.c b/src/H5Sall.c index 4dc7465a8dc..c9ae8d09a04 100644 --- a/src/H5Sall.c +++ b/src/H5Sall.c @@ -636,6 +636,8 @@ H5S__all_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size, bool if (skip) p_end = *p; + else if (p_size == 0) + HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "empty all selection buffer"); else p_end = *p + p_size - 1; /* Pointer to last valid byte in buffer */ diff --git a/src/H5Shyper.c b/src/H5Shyper.c index 8af4b93f175..3ffbe10cd46 100644 --- a/src/H5Shyper.c +++ b/src/H5Shyper.c @@ -3989,6 +3989,14 @@ H5S__hyper_serialize(H5S_t *space, uint8_t **p) pp = (*p); assert(pp); + /* A hyperslab selection is meaningless on a scalar/null dataspace. This + * state can arise if the extent is collapsed to a scalar after the + * selection was made, since changing the extent does not reset an + * existing hyperslab selection. */ + if (0 == space->extent.rank) + HGOTO_ERROR(H5E_DATASPACE, H5E_BADVALUE, FAIL, + "cannot serialize hyperslab selection with a rank of 0"); + /* Set some convenience values */ ndims = space->extent.rank; diminfo = space->select.sel_info.hslab->diminfo.opt; @@ -4263,6 +4271,8 @@ H5S__hyper_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size, bo if (skip) p_end = *p; + else if (p_size == 0) + HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "empty hyperslab selection buffer"); else p_end = *p + p_size - 1; /* Pointer to last valid byte in buffer */ @@ -4328,6 +4338,9 @@ H5S__hyper_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size, bo if (H5_IS_KNOWN_BUFFER_OVERFLOW(skip, pp, sizeof(uint32_t), p_end)) HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "buffer overflow while decoding selection rank"); UINT32DECODE(pp, rank); + if (0 == rank || rank > H5S_MAX_RANK) + HGOTO_ERROR(H5E_DATASPACE, H5E_BADVALUE, FAIL, "invalid rank (%u) for serialized hyperslab selection", + rank); if (!*space) { /* Patch the rank of the allocated dataspace */ diff --git a/src/H5Snone.c b/src/H5Snone.c index ac3ec9beb4a..80dfeb51da6 100644 --- a/src/H5Snone.c +++ b/src/H5Snone.c @@ -579,17 +579,24 @@ H5S__none_serialize(H5S_t *space, uint8_t **p) static herr_t H5S__none_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size, bool skip) { - H5S_t *tmp_space = NULL; /* Pointer to actual dataspace to use, - either *space or a newly allocated one */ - uint32_t version; /* Version number */ - herr_t ret_value = SUCCEED; /* return value */ - const uint8_t *p_end = *p + p_size - 1; /* Pointer to last valid byte in buffer */ + H5S_t *tmp_space = NULL; /* Pointer to actual dataspace to use, + either *space or a newly allocated one */ + uint32_t version; /* Version number */ + herr_t ret_value = SUCCEED; /* return value */ + const uint8_t *p_end; /* Pointer to last valid byte in buffer */ FUNC_ENTER_PACKAGE assert(p); assert(*p); + if (skip) + p_end = *p; + else if (p_size == 0) + HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "empty none selection buffer"); + else + p_end = *p + p_size - 1; + /* As part of the efforts to push all selection-type specific coding to the callbacks, the coding for the allocation of a null dataspace is moved from H5S_select_deserialize() in H5Sselect.c to here. diff --git a/src/H5Spoint.c b/src/H5Spoint.c index 22e73ed84a0..3bfc03372ef 100644 --- a/src/H5Spoint.c +++ b/src/H5Spoint.c @@ -1375,6 +1375,8 @@ H5S__point_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size, bo if (skip) p_end = *p; + else if (p_size == 0) + HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "empty point selection buffer"); else p_end = *p + p_size - 1; /* Pointer to last valid byte in buffer */ diff --git a/src/H5Sselect.c b/src/H5Sselect.c index e8f3ae4a206..fdf9d212327 100644 --- a/src/H5Sselect.c +++ b/src/H5Sselect.c @@ -526,7 +526,8 @@ H5S_select_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size) uint32_t sel_type; /* Pointer to the selection type */ herr_t ret_value = FAIL; /* Return value */ const uint8_t *p_end = NULL; /* Pointer to last valid byte in buffer */ - bool skip = false; + size_t sel_info_size; /* Size of selection-type-specific information */ + bool skip = false; FUNC_ENTER_NOAPI(FAIL) @@ -536,6 +537,8 @@ H5S_select_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size) skip = (p_size == SIZE_MAX ? true : false); if (skip) p_end = *p; + else if (p_size < sizeof(uint32_t)) + HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "selection buffer too small"); else p_end = *p + p_size - 1; @@ -545,23 +548,24 @@ H5S_select_deserialize(H5S_t **space, const uint8_t **p, const size_t p_size) if (H5_IS_KNOWN_BUFFER_OVERFLOW(skip, *p, sizeof(uint32_t), p_end)) HGOTO_ERROR(H5E_DATASPACE, H5E_OVERFLOW, FAIL, "buffer overflow while decoding selection type"); UINT32DECODE(*p, sel_type); + sel_info_size = (skip ? SIZE_MAX : p_size - sizeof(uint32_t)); /* Make routine for selection type */ switch (sel_type) { case H5S_SEL_POINTS: /* Sequence of points selected */ - ret_value = (*H5S_sel_point->deserialize)(space, p, p_size - sizeof(uint32_t), skip); + ret_value = (*H5S_sel_point->deserialize)(space, p, sel_info_size, skip); break; case H5S_SEL_HYPERSLABS: /* Hyperslab selection defined */ - ret_value = (*H5S_sel_hyper->deserialize)(space, p, p_size - sizeof(uint32_t), skip); + ret_value = (*H5S_sel_hyper->deserialize)(space, p, sel_info_size, skip); break; case H5S_SEL_ALL: /* Entire extent selected */ - ret_value = (*H5S_sel_all->deserialize)(space, p, p_size - sizeof(uint32_t), skip); + ret_value = (*H5S_sel_all->deserialize)(space, p, sel_info_size, skip); break; case H5S_SEL_NONE: /* Nothing selected */ - ret_value = (*H5S_sel_none->deserialize)(space, p, p_size - sizeof(uint32_t), skip); + ret_value = (*H5S_sel_none->deserialize)(space, p, sel_info_size, skip); break; default: diff --git a/test/tselect.c b/test/tselect.c index 03477d16919..dccb88990d7 100644 --- a/test/tselect.c +++ b/test/tselect.c @@ -16054,6 +16054,142 @@ test_h5s_set_extent_none(void) } /* test_h5s_set_extent_none() */ +/**************************************************************** +** +** test_select_deserialize_invalid: +** Test malformed serialized selections that should fail cleanly. +** +****************************************************************/ +static void +test_select_deserialize_invalid(void) +{ + uint8_t hyper_bad_rank[14]; + uint8_t point_bad_rank[13]; + uint8_t short_selection_type[1] = {0}; + uint8_t type_only[4]; + uint8_t none_selection[16]; + uint8_t *w; + const uint8_t *r; + H5S_t *space = NULL; + herr_t ret; + + TESTING("deserialization of malformed serialized selections"); + + w = hyper_bad_rank; + UINT32ENCODE(w, (uint32_t)H5S_SEL_HYPERSLABS); + UINT32ENCODE(w, (uint32_t)H5S_HYPER_VERSION_3); + *w++ = 0; + *w++ = H5S_SELECT_INFO_ENC_SIZE_4; + UINT32ENCODE(w, (uint32_t)H5S_MAX_RANK + 1); + + r = hyper_bad_rank; + H5E_BEGIN_TRY + { + ret = H5S_SELECT_DESERIALIZE(&space, &r, sizeof(hyper_bad_rank)); + } + H5E_END_TRY + VERIFY(ret, FAIL, "H5S_SELECT_DESERIALIZE"); + VERIFY(space, NULL, "H5S_SELECT_DESERIALIZE"); + + /* A point selection with a rank larger than H5S_MAX_RANK is rejected */ + w = point_bad_rank; + UINT32ENCODE(w, (uint32_t)H5S_SEL_POINTS); + UINT32ENCODE(w, (uint32_t)H5S_POINT_VERSION_2); + *w++ = H5S_SELECT_INFO_ENC_SIZE_4; + UINT32ENCODE(w, (uint32_t)H5S_MAX_RANK + 1); + + r = point_bad_rank; + H5E_BEGIN_TRY + { + ret = H5S_SELECT_DESERIALIZE(&space, &r, sizeof(point_bad_rank)); + } + H5E_END_TRY + VERIFY(ret, FAIL, "H5S_SELECT_DESERIALIZE"); + VERIFY(space, NULL, "H5S_SELECT_DESERIALIZE"); + + r = short_selection_type; + H5E_BEGIN_TRY + { + ret = H5S_SELECT_DESERIALIZE(&space, &r, 0); + } + H5E_END_TRY + VERIFY(ret, FAIL, "H5S_SELECT_DESERIALIZE"); + VERIFY(space, NULL, "H5S_SELECT_DESERIALIZE"); + + /* A buffer holding only a valid selection type with no type-specific + payload passes the outer size check but is rejected by the concrete + deserializer's empty-payload guard */ + w = type_only; + UINT32ENCODE(w, (uint32_t)H5S_SEL_HYPERSLABS); + + r = type_only; + H5E_BEGIN_TRY + { + ret = H5S_SELECT_DESERIALIZE(&space, &r, sizeof(type_only)); + } + H5E_END_TRY + VERIFY(ret, FAIL, "H5S_SELECT_DESERIALIZE"); + VERIFY(space, NULL, "H5S_SELECT_DESERIALIZE"); + + w = none_selection; + UINT32ENCODE(w, (uint32_t)H5S_SEL_NONE); + UINT32ENCODE(w, (uint32_t)H5S_NONE_VERSION_1); + memset(w, 0, 8); + + r = none_selection; + ret = H5S_SELECT_DESERIALIZE(&space, &r, SIZE_MAX); + CHECK(ret, FAIL, "H5S_SELECT_DESERIALIZE"); + CHECK_PTR(space, "H5S_SELECT_DESERIALIZE"); + ret = H5S_close(space); + CHECK(ret, FAIL, "H5S_close"); + + PASSED(); +} /* test_select_deserialize_invalid() */ + +/**************************************************************** +** +** test_select_hyper_serialize_scalar: +** Attempting to serialize a hyperslab selection whose dataspace has been +** collapsed to a scalar extent (rank 0) must fail cleanly. +** +****************************************************************/ +static void +test_select_hyper_serialize_scalar(void) +{ + hid_t sid = H5I_INVALID_HID; + hsize_t dims[1] = {10}; + hsize_t start[1] = {0}, stride[1] = {2}, count[1] = {5}, block[1] = {1}; + uint8_t buf[2048]; + size_t buf_size = sizeof(buf); + herr_t ret; + + TESTING("serialization of hyperslab selection on a scalar extent"); + + sid = H5Screate_simple(1, dims, NULL); + CHECK(sid, H5I_INVALID_HID, "H5Screate_simple"); + + ret = H5Sselect_hyperslab(sid, H5S_SELECT_SET, start, stride, count, block); + CHECK(ret, FAIL, "H5Sselect_hyperslab"); + + /* Collapse the extent to scalar, leaving the rank-mismatched + hyperslab selection in place */ + ret = H5Sset_extent_simple(sid, 0, NULL, NULL); + CHECK(ret, FAIL, "H5Sset_extent_simple"); + + /* Encoding the mismatched selection must fail rather than crash */ + H5E_BEGIN_TRY + { + ret = H5Sencode2(sid, buf, &buf_size, H5P_DEFAULT); + } + H5E_END_TRY + VERIFY(ret, FAIL, "H5Sencode2"); + + ret = H5Sclose(sid); + CHECK(ret, FAIL, "H5Sclose"); + + PASSED(); +} /* test_select_hyper_serialize_scalar() */ + /**************************************************************** ** ** test_select(): Main H5S selection testing routine. @@ -16260,6 +16396,12 @@ test_select(void H5_ATTR_UNUSED *params) */ test_h5s_set_extent_none(); + /* Test malformed serialized selections */ + test_select_deserialize_invalid(); + + /* Test serialization of a hyperslab selection on a scalar extent */ + test_select_hyper_serialize_scalar(); + } /* test_select() */ /*-------------------------------------------------------------------------