From bc3d878add940845a2ec5b8873f2d45a00926ce8 Mon Sep 17 00:00:00 2001 From: Binh-Minh Ribler Date: Tue, 15 Jan 2019 11:48:31 -0600 Subject: Fixed HDFFV-10586 and HDFFV-10588 Description: HDFFV-10586 CVE-2018-17434 Divide by zero inh5repack_filters Added a check for zero value HDFFV-10588 CVE-2018-17437 Memory leak in H5O_dtype_decode_helper This is actually an Invalid read issue. It was found that the attribute name length in an attribute message was corrupted, which caused the buffer pointer to be advanced too far and later caused an invalid read. Added a check to detect attribute name and its length mismatch. The fix is not perfect, but it'll reduce the chance of this issue when a name length is corrupted or the attribute name is corrupted. Platforms tested: Linux/64 (jelly) Linux/64 (platypus) Darwin (osx1010test) --- src/H5Oattr.c | 7 ++++++- tools/src/h5repack/h5repack_filters.c | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/H5Oattr.c b/src/H5Oattr.c index c93bf32..c420046 100644 --- a/src/H5Oattr.c +++ b/src/H5Oattr.c @@ -176,7 +176,12 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags, /* Decode and store the name */ if(NULL == (attr->shared->name = H5MM_strdup((const char *)p))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") + + /* Make an attempt to detect corrupted name or name length - HDFFV-10588 */ + if(name_len != (HDstrlen(attr->shared->name) + 1)) + HGOTO_ERROR(H5E_ATTR, H5E_CANTDECODE, NULL, "attribute name has different length than stored length") + if(attr->shared->version < H5O_ATTR_VERSION_2) p += H5O_ALIGN_OLD(name_len); /* advance the memory pointer */ else diff --git a/tools/src/h5repack/h5repack_filters.c b/tools/src/h5repack/h5repack_filters.c index 0092abc..123263c 100644 --- a/tools/src/h5repack/h5repack_filters.c +++ b/tools/src/h5repack/h5repack_filters.c @@ -338,12 +338,13 @@ int apply_filters(const char* name, /* object name from traverse list */ sm_nbytes = msize; for (i = rank; i > 0; --i) { + if(sm_nbytes == 0) + HGOTO_ERROR(FAIL, H5E_tools_min_id_g, "number of bytes per stripmine must be > 0"); hsize_t size = H5TOOLS_BUFSIZE / sm_nbytes; if (size == 0) /* datum size > H5TOOLS_BUFSIZE */ size = 1; sm_size[i - 1] = MIN(dims[i - 1], size); sm_nbytes *= sm_size[i - 1]; - HDassert(sm_nbytes > 0); } for (i = 0; i < rank; i++) { -- cgit v0.12 From d584b233a571fcc9854a716590fd09bd9e9320c4 Mon Sep 17 00:00:00 2001 From: Binh-Minh Ribler Date: Sun, 20 Jan 2019 16:49:46 -0600 Subject: Fixed HDFFV-10684 Description: The test for HDFFV-10588 has revealed a bug in H5Ewalk. H5Ewalk did not stop midway even when the call back function returns H5_ITER_STOP. This is because a condition is missing from the for loops in H5E__walk causing the callback functions unable to stop until all the errors in the stack are iterated. Quincey advised on the final fix. In this fix, "status" is switched to "ret_value" and HGOTO_ERROR to HERROR, and the for loops won't continue when "ret_value" is not 0. Platforms tested: Linux/64 (jelly) Linux/64 (platypus) Darwin (osx1010test) --- src/H5E.c | 4 ++-- src/H5Eint.c | 36 +++++++++++++++++------------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/H5E.c b/src/H5E.c index 170133e..3f83355 100644 --- a/src/H5E.c +++ b/src/H5E.c @@ -1484,7 +1484,7 @@ done: * * Purpose: Prints the error stack in some default way. This is just a * convenience function for H5Ewalk() with a function that - * prints error messages. Users are encouraged to write there + * prints error messages. Users are encouraged to write their * own more specific error handlers. * * Return: SUCCEED/FAIL @@ -1567,7 +1567,7 @@ H5Ewalk2(hid_t err_stack, H5E_direction_t direction, H5E_walk2_t stack_func, voi op.vers = 2; op.u.func2 = stack_func; if(H5E__walk(estack, direction, &op, client_data) < 0) - HGOTO_ERROR(H5E_ERROR, H5E_CANTLIST, FAIL, "can't walk error stack") + HERROR(H5E_ERROR, H5E_CANTLIST, "can't walk error stack"); done: FUNC_LEAVE_API(ret_value) diff --git a/src/H5Eint.c b/src/H5Eint.c index 540c9b1..66653ca 100644 --- a/src/H5Eint.c +++ b/src/H5Eint.c @@ -422,7 +422,7 @@ done: * Purpose: Private function to print the error stack in some default * way. This is just a convenience function for H5Ewalk() and * H5Ewalk2() with a function that prints error messages. - * Users are encouraged to write there own more specific error + * Users are encouraged to write their own more specific error * handlers. * * Return: SUCCEED/FAIL @@ -511,10 +511,9 @@ H5E__walk(const H5E_t *estack, H5E_direction_t direction, const H5E_walk_op_t *o void *client_data) { int i; /* Local index variable */ - herr_t status; /* Status from callback function */ - herr_t ret_value = SUCCEED; /* Return value */ + herr_t ret_value = H5_ITER_CONT; /* Return value */ - FUNC_ENTER_PACKAGE + FUNC_ENTER_PACKAGE_NOERR /* Sanity check */ HDassert(estack); @@ -530,9 +529,9 @@ H5E__walk(const H5E_t *estack, H5E_direction_t direction, const H5E_walk_op_t *o if(op->u.func1) { H5E_error1_t old_err; - status = SUCCEED; + ret_value = SUCCEED; if(H5E_WALK_UPWARD == direction) { - for(i = 0; i < (int)estack->nused && status >= 0; i++) { + for(i = 0; i < (int)estack->nused && ret_value == H5_ITER_CONT; i++) { /* Point to each error record on the stack and pass it to callback function.*/ old_err.maj_num = estack->slot[i].maj_num; old_err.min_num = estack->slot[i].min_num; @@ -541,12 +540,12 @@ H5E__walk(const H5E_t *estack, H5E_direction_t direction, const H5E_walk_op_t *o old_err.desc = estack->slot[i].desc; old_err.line = estack->slot[i].line; - status = (op->u.func1)(i, &old_err, client_data); + ret_value = (op->u.func1)(i, &old_err, client_data); } /* end for */ } /* end if */ else { H5_CHECK_OVERFLOW(estack->nused - 1, size_t, int); - for(i = (int)(estack->nused - 1); i >= 0 && status >= 0; i--) { + for(i = (int)(estack->nused - 1); i >= 0 && ret_value == H5_ITER_CONT; i--) { /* Point to each error record on the stack and pass it to callback function.*/ old_err.maj_num = estack->slot[i].maj_num; old_err.min_num = estack->slot[i].min_num; @@ -555,12 +554,12 @@ H5E__walk(const H5E_t *estack, H5E_direction_t direction, const H5E_walk_op_t *o old_err.desc = estack->slot[i].desc; old_err.line = estack->slot[i].line; - status = (op->u.func1)((int)(estack->nused - (size_t)(i + 1)), &old_err, client_data); + ret_value = (op->u.func1)((int)(estack->nused - (size_t)(i + 1)), &old_err, client_data); } /* end for */ } /* end else */ - if(status < 0) - HGOTO_ERROR(H5E_ERROR, H5E_CANTLIST, FAIL, "can't walk error stack") + if(ret_value < 0) + HERROR(H5E_ERROR, H5E_CANTLIST, "can't walk error stack"); } /* end if */ #else /* H5_NO_DEPRECATED_SYMBOLS */ HDassert(0 && "version 1 error stack walk without deprecated symbols!"); @@ -569,23 +568,22 @@ H5E__walk(const H5E_t *estack, H5E_direction_t direction, const H5E_walk_op_t *o else { HDassert(op->vers == 2); if(op->u.func2) { - status = SUCCEED; + ret_value = SUCCEED; if(H5E_WALK_UPWARD == direction) { - for(i = 0; i < (int)estack->nused && status >= 0; i++) - status = (op->u.func2)((unsigned)i, estack->slot + i, client_data); + for(i = 0; i < (int)estack->nused && ret_value == H5_ITER_CONT; i++) + ret_value = (op->u.func2)((unsigned)i, estack->slot + i, client_data); } /* end if */ else { H5_CHECK_OVERFLOW(estack->nused - 1, size_t, int); - for(i = (int)(estack->nused - 1); i >= 0 && status >= 0; i--) - status = (op->u.func2)((unsigned)(estack->nused - (size_t)(i + 1)), estack->slot + i, client_data); + for(i = (int)(estack->nused - 1); i >= 0 && ret_value == H5_ITER_CONT; i--) + ret_value = (op->u.func2)((unsigned)(estack->nused - (size_t)(i + 1)), estack->slot + i, client_data); } /* end else */ - if(status < 0) - HGOTO_ERROR(H5E_ERROR, H5E_CANTLIST, FAIL, "can't walk error stack") + if(ret_value < 0) + HERROR(H5E_ERROR, H5E_CANTLIST, "can't walk error stack"); } /* end if */ } /* end else */ -done: FUNC_LEAVE_NOAPI(ret_value) } /* end H5E__walk() */ -- cgit v0.12 From 25cd1ab02b9ddaf58a4f5422f4ab4fde411e050a Mon Sep 17 00:00:00 2001 From: Binh-Minh Ribler Date: Thu, 24 Jan 2019 09:42:19 -0600 Subject: Added test for HDFFV-10588 Platforms tested: Linux/64 (jelly) Linux/64 (platypus) Darwin (osx1010test) --- src/H5E.c | 2 +- test/titerate.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/H5E.c b/src/H5E.c index 3f83355..06ae806 100644 --- a/src/H5E.c +++ b/src/H5E.c @@ -1566,7 +1566,7 @@ H5Ewalk2(hid_t err_stack, H5E_direction_t direction, H5E_walk2_t stack_func, voi /* Walk the error stack */ op.vers = 2; op.u.func2 = stack_func; - if(H5E__walk(estack, direction, &op, client_data) < 0) + if((ret_value = H5E__walk(estack, direction, &op, client_data)) < 0) HERROR(H5E_ERROR, H5E_CANTLIST, "can't walk error stack"); done: diff --git a/test/titerate.c b/test/titerate.c index de652a7..87ddfb8 100644 --- a/test/titerate.c +++ b/test/titerate.c @@ -20,6 +20,7 @@ *************************************************************/ #include "testhdf5.h" +#include "H5srcdir.h" #define DATAFILE "titerate.h5" @@ -53,6 +54,17 @@ typedef struct { iter_enum command; /* The type of return value */ } iter_info; +/* Definition for test_corrupted_attnamelen */ +#define CORRUPTED_ATNAMELEN_FILE "memleak_H5O_dtype_decode_helper_H5Odtype.h5" +#define DSET_NAME "image" +typedef struct searched_err_t { + char message[256]; + bool found; +} searched_err_t; + +/* Call back function for test_corrupted_attnamelen */ +static int find_err_msg_cb(unsigned n, const H5E_error2_t *err_desc, void *_client_data); + /* Local functions */ int iter_strcmp(const void *s1, const void *s2); int iter_strcmp2(const void *s1, const void *s2); @@ -915,6 +927,92 @@ static void test_links(hid_t fapl) CHECK(ret, FAIL, "H5Fclose"); } /* test_links() */ +/*------------------------------------------------------------------------- + * Function: find_err_msg_cb + * + * Purpose: Callback function to find the given error message. + * Helper function for test_corrupted_attnamelen(). + * + * Return: H5_ITER_STOP when the message is found + * H5_ITER_CONT, otherwise + * + *------------------------------------------------------------------------- + */ +static int +find_err_msg_cb(unsigned n, const H5E_error2_t *err_desc, void *_client_data) +{ + int status = H5_ITER_CONT; + searched_err_t *searched_err = (searched_err_t *)_client_data; + + if (searched_err == NULL) + return -1; + + /* If the searched error message is found, stop the iteration */ + if (err_desc->desc != NULL && strcmp(err_desc->desc, searched_err->message) == 0) + { + searched_err->found = true; + status = H5_ITER_STOP; + } + return status; +} /* end find_err_msg_cb() */ + +/************************************************************************** +** +** test_corrupted_attnamelen(): Test the fix for the JIRA issue HDFFV-10588, +** where corrupted attribute's name length can be +** detected and invalid read can be avoided. +** +**************************************************************************/ +static void test_corrupted_attnamelen(void) +{ + hid_t fid = -1; /* File ID */ + hid_t did = -1; /* Dataset ID */ + searched_err_t err_caught; /* Data to be passed to callback func */ + int err_status; /* Status returned by H5Aiterate2 */ + herr_t ret; /* Return value */ + const char *testfile = H5_get_srcdir_filename(CORRUPTED_ATNAMELEN_FILE); /* Corrected test file name */ + + const char *err_message = "attribute name has different length than stored length"; + /* the error message produced when the failure occurs */ + + /* Output message about test being performed */ + MESSAGE(5, ("Testing the Handling of Corrupted Attribute's Name Length\n")); + + fid = H5Fopen(testfile, H5F_ACC_RDONLY, H5P_DEFAULT); + CHECK(fid, FAIL, "H5Fopen"); + + /* Open the dataset */ + did = H5Dopen2(fid, DSET_NAME, H5P_DEFAULT); + CHECK(did, FAIL, "H5Dopen2"); + + /* Call H5Aiterate2 to trigger the failure in HDFFV-10588. Failure should + occur in the decoding stage, so some arguments are not needed. */ + err_status = H5Aiterate2(did, H5_INDEX_NAME, H5_ITER_INC, NULL, NULL, NULL); + + /* Make sure the intended error was caught */ + if(err_status == -1) + { + /* Initialize client data */ + HDstrcpy(err_caught.message, err_message); + err_caught.found = false; + + /* Look for the correct error message */ + ret = H5Ewalk2(H5E_DEFAULT, H5E_WALK_UPWARD, find_err_msg_cb, &err_caught); + CHECK(ret, FAIL, "H5Ewalk2"); + + /* Fail if the indicated message is not found */ + CHECK(err_caught.found, false, "test_corrupted_attnamelen: Expected error not found"); + } + + /* Close the dataset and file */ + ret = H5Dclose(did); + CHECK(ret, FAIL, "H5Dclose"); + + ret = H5Fclose(fid); + CHECK(ret, FAIL, "H5Fclose"); + +} /* test_corrupted_attnamelen() */ + /**************************************************************** ** ** test_iterate(): Main iteration testing routine. @@ -951,6 +1049,9 @@ test_iterate(void) test_links(new_format ? fapl2 : fapl); /* Test soft and hard link iteration */ } /* end for */ + /* Test the fix for issue HDFFV-10588 */ + test_corrupted_attnamelen(); + /* Close FAPLs */ ret = H5Pclose(fapl); CHECK(ret, FAIL, "H5Pclose"); -- cgit v0.12