From 0fd60acdbe7b6d86eced078565bef1bc833ec066 Mon Sep 17 00:00:00 2001 From: Binh-Minh Ribler Date: Mon, 30 Nov 2020 09:32:31 -0600 Subject: [PATCH 1/2] Fixed HDFFV-10480 and HDFFV-11159 Description Checked against buffer size to prevent segfault, in case of data corruption. + HDFFV-11159 CVE-2018-14033 Buffer over-read in H5O_layout_decode + HDFFV-10480 CVE-2018-11206 Buffer over-read in H5O_fill_new[/old]_decode and A user's patch was applied to this previously, but it is redone for a more correct fix, that is the check now accounted for the previous advance of the buffer pointer. Platforms tested: Linux/64 (jelly) --- src/H5Ofill.c | 15 +++++++++++++-- src/H5Olayout.c | 11 ++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/H5Ofill.c b/src/H5Ofill.c index 1095d1b4824..ef4f50b0a9d 100644 --- a/src/H5Ofill.c +++ b/src/H5Ofill.c @@ -184,9 +184,10 @@ H5FL_BLK_EXTERN(type_conv); static void * H5O_fill_new_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, 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) + size_t p_size, const uint8_t *p) { 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_NOAPI_NOINIT @@ -218,6 +219,11 @@ H5O_fill_new_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t INT32DECODE(p, fill->size); if (fill->size > 0) { H5_CHECK_OVERFLOW(fill->size, ssize_t, size_t); + + /* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */ + if (p + fill->size - 1 > p_end) + HGOTO_ERROR(H5E_RESOURCE, 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") HDmemcpy(fill->buf, p, (size_t)fill->size); @@ -299,9 +305,10 @@ H5O_fill_new_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t static void * H5O_fill_old_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, 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) + size_t p_size, const uint8_t *p) { H5O_fill_t *fill = NULL; /* Decoded fill value message */ + const uint8_t *p_end = p + p_size - 1; /* End of the p buffer */ void * ret_value; /* Return value */ FUNC_ENTER_NOAPI_NOINIT @@ -322,6 +329,10 @@ H5O_fill_old_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t /* Only decode the fill value itself if there is one */ if (fill->size > 0) { + /* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */ + if (p + fill->size - 1 > p_end) + HGOTO_ERROR(H5E_RESOURCE, 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") HDmemcpy(fill->buf, p, (size_t)fill->size); diff --git a/src/H5Olayout.c b/src/H5Olayout.c index 446694017bc..ef3724991f4 100644 --- a/src/H5Olayout.c +++ b/src/H5Olayout.c @@ -90,10 +90,11 @@ H5FL_DEFINE(H5O_layout_t); static void * H5O_layout_decode(H5F_t *f, hid_t H5_ATTR_UNUSED dxpl_id, 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) + size_t p_size, const uint8_t *p) { H5O_layout_t *mesg = 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_NOAPI_NOINIT @@ -174,6 +175,10 @@ H5O_layout_decode(H5F_t *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t H5_ATTR_UNUSED * 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_RESOURCE, H5E_OVERFLOW, NULL, "storage fill 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") @@ -191,6 +196,10 @@ H5O_layout_decode(H5F_t *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t H5_ATTR_UNUSED * case H5D_COMPACT: 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_RESOURCE, 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") From 39b7cf1fb4ba168c1249992badcabd976beea7b1 Mon Sep 17 00:00:00 2001 From: Binh-Minh Ribler Date: Mon, 30 Nov 2020 11:23:22 -0600 Subject: [PATCH 2/2] Fixed typo --- src/H5Ofill.c | 4 ++-- src/H5Olayout.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/H5Ofill.c b/src/H5Ofill.c index ef4f50b0a9d..bf132121b69 100644 --- a/src/H5Ofill.c +++ b/src/H5Ofill.c @@ -222,7 +222,7 @@ H5O_fill_new_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t /* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */ if (p + fill->size - 1 > p_end) - HGOTO_ERROR(H5E_RESOURCE, H5E_OVERFLOW, NULL, "fill size exceeds buffer size") + 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") @@ -331,7 +331,7 @@ H5O_fill_old_decode(H5F_t H5_ATTR_UNUSED *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t if (fill->size > 0) { /* Ensure that fill size doesn't exceed buffer size, due to possible data corruption */ if (p + fill->size - 1 > p_end) - HGOTO_ERROR(H5E_RESOURCE, H5E_OVERFLOW, NULL, "fill size exceeds buffer size") + 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") diff --git a/src/H5Olayout.c b/src/H5Olayout.c index ef3724991f4..b9b8cc6aa01 100644 --- a/src/H5Olayout.c +++ b/src/H5Olayout.c @@ -177,7 +177,7 @@ H5O_layout_decode(H5F_t *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t H5_ATTR_UNUSED * 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_RESOURCE, H5E_OVERFLOW, NULL, "storage fill size exceeds buffer size") + HGOTO_ERROR(H5E_OHDR, H5E_OVERFLOW, NULL, "storage fill 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, @@ -198,7 +198,7 @@ H5O_layout_decode(H5F_t *f, hid_t H5_ATTR_UNUSED dxpl_id, H5O_t H5_ATTR_UNUSED * 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_RESOURCE, H5E_OVERFLOW, NULL, "storage size exceeds buffer size") + 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,