Fixes a bad memory read and unfreed memory in fsinfo code (#893)

* Fixes a bad memory read and unfreed memory in fsinfo code

The segfaul from CVE-2020-10810 was fixed some time ago, but the
illegal memory read and unfreed memory were not.

This fix tracks some buffer sizes and errors out gracefully on errors,
ensuring buffers are cleaned up and avoiding the H5FL infinite loop +
abort on library close.

* Committing clang-format changes

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

MANIFEST

@@ -1168,6 +1168,7 @@
 ./test/cork.c
 ./test/corrupt_stab_msg.h5
 ./test/cross_read.c
+./test/cve_2020_10810.h5
 ./test/dangle.c
 ./test/deflate.h5
 ./test/del_many_dense_attrs.c

release_docs/RELEASE.txt

@@ -924,6 +924,24 @@ Bug Fixes since HDF5-1.12.0 release
 ===================================
     Library
     -------
+    - Fixed an invalid read and memory leak when parsing corrupt file space
+      info messages
+
+        When the corrupt file from CVE-2020-10810 was parsed by the library,
+        the code that imports the version 0 file space info object header
+        message to the version 1 struct could read past the buffer read from
+        the disk, causing an invalid memory read. Not catching this error would
+        cause downstream errors that eventually resulted in a previously
+        allocated buffer to be unfreed when the library shut down. In builds
+        where the free lists are in use, this could result in an infinite loop
+        and SIGABRT when the library shuts down.
+
+        We now track the buffer size and raise an error on attempts to read
+        past the end of it.
+
+        (DER - 2021/08/12, HDFFV-11053)
+
+
     - Fixed CVE-2018-14460
 
         The tool h5repack produced a segfault when the rank in dataspace

src/H5Ocache.c

@@ -78,8 +78,8 @@ static herr_t H5O__cache_chk_free_icr(void *thing);
 static herr_t H5O__prefix_deserialize(const uint8_t *image, H5O_cache_ud_t *udata);
 
 /* Chunk routines */
-static herr_t H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t len, const uint8_t *image,
-                                     H5O_common_cache_ud_t *udata, hbool_t *dirty);
+static herr_t H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t chunk_size, const uint8_t *image,
+                                     size_t len, H5O_common_cache_ud_t *udata, hbool_t *dirty);
 static herr_t H5O__chunk_serialize(const H5F_t *f, H5O_t *oh, unsigned chunkno);
 
 /* Misc. routines */
@@ -287,7 +287,7 @@ H5O__cache_verify_chksum(const void *_image, size_t len, void *_udata)
  *-------------------------------------------------------------------------
  */
 static void *
-H5O__cache_deserialize(const void *image, size_t H5_ATTR_NDEBUG_UNUSED len, void *_udata, hbool_t *dirty)
+H5O__cache_deserialize(const void *image, size_t len, void *_udata, hbool_t *dirty)
 {
     H5O_t *         oh        = NULL;                     /* Object header read in */
     H5O_cache_ud_t *udata     = (H5O_cache_ud_t *)_udata; /* User data for callback */
@@ -333,7 +333,7 @@ H5O__cache_deserialize(const void *image, size_t H5_ATTR_NDEBUG_UNUSED len, void
         oh->proxy = NULL;
 
     /* Parse the first chunk */
-    if (H5O__chunk_deserialize(oh, udata->common.addr, udata->chunk0_size, (const uint8_t *)image,
+    if (H5O__chunk_deserialize(oh, udata->common.addr, udata->chunk0_size, (const uint8_t *)image, len,
                                &(udata->common), dirty) < 0)
         HGOTO_ERROR(H5E_OHDR, H5E_CANTINIT, NULL, "can't deserialize first object header chunk")
 
@@ -736,7 +736,7 @@ H5O__cache_chk_verify_chksum(const void *_image, size_t len, void *_udata)
  *-------------------------------------------------------------------------
  */
 static void *
-H5O__cache_chk_deserialize(const void *image, size_t H5_ATTR_NDEBUG_UNUSED len, void *_udata, hbool_t *dirty)
+H5O__cache_chk_deserialize(const void *image, size_t len, void *_udata, hbool_t *dirty)
 {
     H5O_chunk_proxy_t * chk_proxy = NULL;                         /* Chunk proxy object */
     H5O_chk_cache_ud_t *udata     = (H5O_chk_cache_ud_t *)_udata; /* User data for callback */
@@ -763,7 +763,7 @@ H5O__cache_chk_deserialize(const void *image, size_t H5_ATTR_NDEBUG_UNUSED len,
         HDassert(udata->common.cont_msg_info);
 
         /* Parse the chunk */
-        if (H5O__chunk_deserialize(udata->oh, udata->common.addr, udata->size, (const uint8_t *)image,
+        if (H5O__chunk_deserialize(udata->oh, udata->common.addr, udata->size, (const uint8_t *)image, len,
                                    &(udata->common), dirty) < 0)
             HGOTO_ERROR(H5E_OHDR, H5E_CANTINIT, NULL, "can't deserialize object header chunk")
 
@@ -1275,7 +1275,7 @@ done:
  *-------------------------------------------------------------------------
  */
 static herr_t
-H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t len, const uint8_t *image,
+H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t chunk_size, const uint8_t *image, size_t len,
                        H5O_common_cache_ud_t *udata, hbool_t *dirty)
 {
     const uint8_t *chunk_image;          /* Pointer into buffer to decode */
@@ -1295,6 +1295,7 @@ H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t len, const uint8_t *image
     HDassert(oh);
     HDassert(H5F_addr_defined(addr));
     HDassert(image);
+    HDassert(len);
     HDassert(udata->f);
     HDassert(udata->cont_msg_info);
 
@@ -1315,14 +1316,16 @@ H5O__chunk_deserialize(H5O_t *oh, haddr_t addr, size_t len, const uint8_t *image
     oh->chunk[chunkno].addr = addr;
     if (chunkno == 0)
         /* First chunk's 'image' includes room for the object header prefix */
-        oh->chunk[0].size = len + (size_t)H5O_SIZEOF_HDR(oh);
+        oh->chunk[0].size = chunk_size + (size_t)H5O_SIZEOF_HDR(oh);
     else
-        oh->chunk[chunkno].size = len;
+        oh->chunk[chunkno].size = chunk_size;
     if (NULL == (oh->chunk[chunkno].image = H5FL_BLK_MALLOC(chunk_image, oh->chunk[chunkno].size)))
         HGOTO_ERROR(H5E_OHDR, H5E_CANTALLOC, FAIL, "memory allocation failed")
     oh->chunk[chunkno].chunk_proxy = NULL;
 
     /* Copy disk image into chunk's image */
+    if (len < oh->chunk[chunkno].size)
+        HGOTO_ERROR(H5E_OHDR, H5E_CANTCOPY, FAIL, "attempted to copy too many disk image bytes into buffer")
     H5MM_memcpy(oh->chunk[chunkno].image, image, oh->chunk[chunkno].size);
 
     /* Point into chunk image to decode */

src/H5Ofsinfo.c

@@ -91,11 +91,12 @@ H5FL_DEFINE_STATIC(H5O_fsinfo_t);
  */
 static void *
 H5O__fsinfo_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_fsinfo_t * fsinfo = NULL;    /* File space info message */
-    H5F_mem_page_t ptype;            /* Memory type for iteration */
-    unsigned       vers;             /* message version */
+    H5O_fsinfo_t * fsinfo = NULL; /* File space info message */
+    H5F_mem_page_t ptype;         /* Memory type for iteration */
+    unsigned       vers;          /* message version */
+    const uint8_t *p_end     = p + p_size;
     void *         ret_value = NULL; /* Return value */
 
     FUNC_ENTER_STATIC
@@ -136,8 +137,12 @@ H5O__fsinfo_decode(H5F_t *f, H5O_t H5_ATTR_UNUSED *open_oh, unsigned H5_ATTR_UNU
                 fsinfo->threshold = threshold;
                 if (HADDR_UNDEF == (fsinfo->eoa_pre_fsm_fsalloc = H5F_get_eoa(f, H5FD_MEM_DEFAULT)))
                     HGOTO_ERROR(H5E_FILE, H5E_CANTGET, NULL, "unable to get file size")
-                for (type = H5FD_MEM_SUPER; type < H5FD_MEM_NTYPES; type++)
+                for (type = H5FD_MEM_SUPER; type < H5FD_MEM_NTYPES; type++) {
+                    if (p + H5_SIZEOF_HADDR_T > p_end)
+                        HGOTO_ERROR(H5E_FILE, H5E_CANTDECODE, NULL,
+                                    "ran off end of input buffer while decoding")
                     H5F_addr_decode(f, &p, &(fsinfo->fs_addr[type - 1]));
+                }
                 break;
 
             case H5F_FILE_SPACE_ALL:

test/ohdr.c

@@ -456,6 +456,59 @@ error:
     return FAIL;
 } /* test_ohdr_swmr() */
 
+/*
+ * Tests bad object header messages.
+ *
+ * Currently tests for CVE-2020-10810 fixes but can be expanded to handle
+ * other CVE badness.
+ */
+
+/* This is a generated file that can be obtained from:
+ *
+ * https://nvd.nist.gov/vuln/detail/CVE-2020-10810
+ *
+ * It was formerly named H5AC_unpin_entry_POC
+ */
+#define CVE_2020_10810_FILENAME "cve_2020_10810.h5"
+
+static herr_t
+test_ohdr_badness(hid_t fapl)
+{
+    hid_t fid = H5I_INVALID_HID;
+
+    /* CVE-2020-10810 involved a malformed fsinfo message
+     * This test ensures the fundamental problem is fixed. Running it under
+     * valgrind et al. will ensure that the memory leaks and invalid access
+     * are fixed.
+     */
+    TESTING("Fix for CVE-2020-10810");
+
+    H5E_BEGIN_TRY
+    {
+        /* This should fail due to the malformed fsinfo message. It should
+         * fail gracefully and not segfault.
+         */
+        fid = H5Fopen(CVE_2020_10810_FILENAME, H5F_ACC_RDWR, fapl);
+    }
+    H5E_END_TRY;
+
+    if (fid >= 0)
+        FAIL_PUTS_ERROR("should not have been able to open malformed file");
+
+    PASSED();
+
+    return SUCCEED;
+
+error:
+    H5E_BEGIN_TRY
+    {
+        H5Fclose(fid);
+    }
+    H5E_END_TRY;
+
+    return FAIL;
+}
+
 /*
  *  To test objects with unknown messages in a file with:
  *      a) H5O_BOGUS_VALID_ID:
@@ -2047,6 +2100,9 @@ main(void)
         } /* high */
     }     /* low */
 
+    /* Verify bad ohdr message fixes work */
+    test_ohdr_badness(fapl);
+
     /* Verify symbol table messages are cached */
     if (h5_verify_cached_stabs(FILENAME, fapl) < 0)
         TEST_ERROR