Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed HDFFV-10480 (CVE-2018-11206) and HDFFV-11159 (CVE-2018-14033) #405

Merged
merged 22 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
28da8dc
Fixed HDFFV-10480 (CVE-2018-11206) and HDFFV-11159 (CVE-2018-14033)
bmribler Mar 1, 2021
c1bdb0c
Accidentally left in another occurrence of the previous patch from…
bmribler Mar 1, 2021
c24a0c2
Typo
bmribler Mar 1, 2021
1dec6d8
Fixed format issues.
bmribler Mar 2, 2021
a147ae6
Added test.
bmribler Mar 15, 2021
1671982
Changed arguments to ADD_H5_TEST
bmribler Mar 16, 2021
b45de82
Fixing arguments to ADD_H5_TEST again.
bmribler Mar 16, 2021
5832a70
Fixing arguments again.
bmribler Mar 16, 2021
c21324d
Took out the CMake changes until Allen can help.
bmribler Mar 16, 2021
8e71d59
Added files:
bmribler Mar 16, 2021
d0a00d1
Revert "Took out the CMake changes until Allen can help."
bmribler Mar 16, 2021
0c7c02e
Revert "Fixing arguments again."
bmribler Mar 16, 2021
a030e91
Revert "Fixing arguments to ADD_H5_TEST again."
bmribler Mar 16, 2021
9ae8bd2
Revert "Changed arguments to ADD_H5_TEST"
bmribler Mar 16, 2021
b8a0f9a
Added first argument to ADD_H5_TEST for HDFFV-10480 fix.
bmribler Mar 16, 2021
b343d66
Changed argument 0 to 1
bmribler Mar 16, 2021
5060450
Revert "Changed argument 0 to 1"
bmribler Mar 16, 2021
fcfbacb
Revert "Added first argument to ADD_H5_TEST for HDFFV-10480 fix."
bmribler Mar 16, 2021
5213c5a
Added first argument and corrected the second.
bmribler Mar 16, 2021
e5e2404
Updated fixes for HDFFV-10480 and HDFFV-11159/HDFFV-11049
bmribler Mar 16, 2021
0c04d8b
Improved error messages.
bmribler Mar 17, 2021
64490c6
Merge branch 'develop' into bmr_dev_HDFFV-10480_HDFFV-11159
bmribler Mar 17, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions MANIFEST
Original file line number Diff line number Diff line change
Expand Up @@ -2126,6 +2126,8 @@
./tools/testfiles/twithddl.exp
./tools/testfiles/twithddlfile.ddl
./tools/testfiles/twithddlfile.exp
./tools/testfiles/tCVE_2018_11206_fill_old.h5
./tools/testfiles/tCVE_2018_11206_fill_new.h5

# h5dump test error files
./tools/test/h5dump/errfiles/filter_fail.err
Expand Down
27 changes: 17 additions & 10 deletions src/H5Ofill.c
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,9 @@ H5O__fill_new_decode(H5F_t H5_ATTR_UNUSED *f, H5O_t H5_ATTR_UNUSED *open_oh,
unsigned H5_ATTR_UNUSED mesg_flags, unsigned H5_ATTR_UNUSED *ioflags, size_t p_size,
const uint8_t *p)
{
H5O_fill_t *fill = NULL;
void * ret_value = NULL; /* Return value */
H5O_fill_t * fill = NULL;
const uint8_t *p_end = p + p_size - 1; /* End of the p buffer */
void * ret_value = NULL; /* Return value */

FUNC_ENTER_STATIC

Expand Down Expand Up @@ -228,8 +229,11 @@ H5O__fill_new_decode(H5F_t H5_ATTR_UNUSED *f, H5O_t H5_ATTR_UNUSED *open_oh,
INT32DECODE(p, fill->size);
if (fill->size > 0) {
H5_CHECK_OVERFLOW(fill->size, ssize_t, size_t);
if ((size_t)fill->size > p_size)
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "destination buffer too small")

/* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */
if (p + fill->size - 1 > p_end)
HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "fill size exceeds buffer size")

if (NULL == (fill->buf = H5MM_malloc((size_t)fill->size)))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed for fill value")
H5MM_memcpy(fill->buf, p, (size_t)fill->size);
Expand Down Expand Up @@ -311,10 +315,11 @@ static void *
H5O__fill_old_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
unsigned H5_ATTR_UNUSED *ioflags, size_t p_size, const uint8_t *p)
{
H5O_fill_t *fill = NULL; /* Decoded fill value message */
htri_t exists = FALSE;
H5T_t * dt = NULL;
void * ret_value = NULL; /* Return value */
H5O_fill_t * fill = NULL; /* Decoded fill value message */
htri_t exists = FALSE;
H5T_t * dt = NULL;
const uint8_t *p_end = p + p_size - 1; /* End of the p buffer */
void * ret_value = NULL; /* Return value */

FUNC_ENTER_STATIC

Expand All @@ -335,8 +340,10 @@ H5O__fill_old_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flag
/* Only decode the fill value itself if there is one */
if (fill->size > 0) {
H5_CHECK_OVERFLOW(fill->size, ssize_t, size_t);
if ((size_t)fill->size > p_size)
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "destination buffer too small")

/* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */
if (p + fill->size - 1 > p_end)
HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "fill size exceeds buffer size")

/* Get the datatype message */
if ((exists = H5O_msg_exists_oh(open_oh, H5O_DTYPE_ID)) < 0)
Expand Down
29 changes: 19 additions & 10 deletions src/H5Olayout.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* Purpose: Messages related to data layout.
*/

#define H5D_FRIEND /*suppress error about including H5Dpkg */
#define H5D_FRIEND /*suppress error about including H5Dpkg */
#include "H5Omodule.h" /* This source code file is part of the H5O module */

#include "H5private.h" /* Generic Functions */
Expand Down Expand Up @@ -90,12 +90,13 @@ H5FL_DEFINE(H5O_layout_t);
*/
static void *
H5O__layout_decode(H5F_t *f, H5O_t H5_ATTR_UNUSED *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
unsigned H5_ATTR_UNUSED *ioflags, size_t H5_ATTR_UNUSED p_size, const uint8_t *p)
unsigned H5_ATTR_UNUSED *ioflags, size_t p_size, const uint8_t *p)
{
H5O_layout_t *mesg = NULL;
uint8_t * heap_block = NULL;
unsigned u;
void * ret_value = NULL; /* Return value */
H5O_layout_t * mesg = NULL;
uint8_t * heap_block = NULL;
unsigned u;
const uint8_t *p_end = p + p_size - 1; /* End of the p buffer */
void * ret_value = NULL; /* Return value */

FUNC_ENTER_STATIC

Expand Down Expand Up @@ -179,6 +180,10 @@ H5O__layout_decode(H5F_t *f, H5O_t H5_ATTR_UNUSED *open_oh, unsigned H5_ATTR_UNU
if (mesg->type == H5D_COMPACT) {
UINT32DECODE(p, mesg->storage.u.compact.size);
if (mesg->storage.u.compact.size > 0) {
/* Ensure that size doesn't exceed buffer size, due to possible data corruption */
if (p + mesg->storage.u.compact.size - 1 > p_end)
HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "storage size exceeds buffer size")

if (NULL == (mesg->storage.u.compact.buf = H5MM_malloc(mesg->storage.u.compact.size)))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL,
"memory allocation failed for compact data buffer")
Expand All @@ -198,6 +203,10 @@ H5O__layout_decode(H5F_t *f, H5O_t H5_ATTR_UNUSED *open_oh, unsigned H5_ATTR_UNU
UINT16DECODE(p, mesg->storage.u.compact.size);

if (mesg->storage.u.compact.size > 0) {
/* Ensure that size doesn't exceed buffer size, due to possible data corruption */
if (p + mesg->storage.u.compact.size - 1 > p_end)
HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "storage size exceeds buffer size")

/* Allocate space for compact data */
if (NULL == (mesg->storage.u.compact.buf = H5MM_malloc(mesg->storage.u.compact.size)))
HGOTO_ERROR(H5E_OHDR, H5E_CANTALLOC, NULL,
Expand Down Expand Up @@ -887,13 +896,13 @@ H5O__layout_reset(void *_mesg)
} /* end H5O__layout_reset() */

/*-------------------------------------------------------------------------
* Function: H5O__layout_free
* Function: H5O__layout_free
*
* Purpose: Free's the message
* Purpose: Free's the message
*
* Return: Non-negative on success/Negative on failure
* Return: Non-negative on success/Negative on failure
*
* Programmer: Quincey Koziol
* Programmer: Quincey Koziol
* Saturday, March 11, 2000
*
*-------------------------------------------------------------------------
Expand Down
6 changes: 6 additions & 0 deletions tools/test/h5dump/CMakeTests.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@
${HDF5_TOOLS_DIR}/testfiles/tvlstr.h5
${HDF5_TOOLS_DIR}/testfiles/tvms.h5
${HDF5_TOOLS_DIR}/testfiles/t128bit_float.h5
${HDF5_TOOLS_DIR}/testfiles/tCVE_2018_11206_fill_old.h5
${HDF5_TOOLS_DIR}/testfiles/tCVE_2018_11206_fill_new.h5
${HDF5_TOOLS_DIR}/testfiles/zerodim.h5
#STD_REF_OBJ files
${HDF5_TOOLS_DIR}/testfiles/trefer_attr.h5
Expand Down Expand Up @@ -1172,6 +1174,10 @@
# test to verify HDFFV-9407: long double full precision
ADD_H5_GREP_TEST (t128bit_float 1 "1.123456789012345" -m %.35Lf t128bit_float.h5)

# test to verify HDFFV-10480: out of bounds read in H5O_fill_new[old]_decode
ADD_H5_TEST (tCVE_2018_11206_fill_old 1 tCVE_2018_11206_fill_old.h5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if ADD_H5_TEST should be ADD_H5_GREP_TEST and search that the test failure that is expected and doesn't fail for some other reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but TOOLTEST_FAIL took care of that, it would detect the segfault. I wish the author would put some comments to these macros... :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it segfaults - okay then this is fine.

ADD_H5_TEST (tCVE_2018_11206_fill_new 1 tCVE_2018_11206_fill_new.h5)

##############################################################################
### P L U G I N T E S T S
##############################################################################
Expand Down
35 changes: 35 additions & 0 deletions tools/test/h5dump/testh5dump.sh.in
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ $SRC_H5DUMP_TESTFILES/tvlenstr_array.h5
$SRC_H5DUMP_TESTFILES/tvlstr.h5
$SRC_H5DUMP_TESTFILES/tvms.h5
$SRC_H5DUMP_TESTFILES/err_attr_dspace.h5
$SRC_H5DUMP_TESTFILES/tCVE_2018_11206_fill_old.h5
$SRC_H5DUMP_TESTFILES/tCVE_2018_11206_fill_new.h5
"

LIST_OTHER_TEST_FILES="
Expand Down Expand Up @@ -868,6 +870,35 @@ TOOLTEST5() {
fi
}

# same as TOOLTEST1 but expects h5dump to fail
#
TOOLTEST_FAIL() {

infile=$1
expect="$TESTDIR/`basename $1 exp`.ddl"
actual="$TESTDIR/`basename $1 .exp`.out"

# Run test.
TESTING $DUMPER $@
(
cd $TESTDIR
$RUNSERIAL $DUMPER_BIN "$@" $infile
) >&$actual
RET=$?
# Segfault occurred
if [ $RET == 139 ] ; then
nerrors="`expr $nerrors + 1`"
echo " FAILED"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worthwhile to differentiate the two "FAILED" messages to make the log more meaningful and also to make clear in the second case that an expected h5dump failure didn't happen. Also note that all other FAILED messages in the script are surrounded by *s: FAILED. If you follow that convention it makes it easier to distinguish failed tests in the logs from incidental occurrences of "FAILED" that don't indicate test failures.

I suggest:
line 891 "FAILED - [h5dump] test failed with segmentation fault"
line 895 "FAILED - [h5dump] test did not fail as expected"

[ ] - optional - could also be test name if available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! I knew about '' but must have used the existing PASSED line and changed to FAIL and forgot about the ''.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the changes and resolved the conflict.

# Should fail but didn't
elif [ $RET == 0 ] ; then
nerrors="`expr $nerrors + 1`"
echo " FAILED"
else
echo " PASSED"
fi

}

# ADD_HELP_TEST
TOOLTEST_HELP() {

Expand Down Expand Up @@ -1445,6 +1476,10 @@ TOOLTEST err_attr_dspace.ddl err_attr_dspace.h5
# test to verify HDFFV-9407: long double full precision
GREPTEST OUTTXT "1.123456789012345" t128bit_float.ddl -m %.35Lf t128bit_float.h5

# test to verify HDFFV-10480: out of bounds read in H5O_fill_new[old]_decode
TOOLTEST_FAIL tCVE_2018_11206_fill_old.h5
TOOLTEST_FAIL tCVE_2018_11206_fill_new.h5

# Clean up temporary files/directories
CLEAN_TESTFILES_AND_TESTDIR

Expand Down
Binary file added tools/testfiles/tCVE_2018_11206_fill_new.h5
Binary file not shown.
Binary file added tools/testfiles/tCVE_2018_11206_fill_old.h5
Binary file not shown.