Validate location (offset) of the accumulated metadata when comparing (#2231)

Initially, the accumulated metadata location is initialized to HADDR_UNDEF
- the highest available address. Bogus input files may provide a location
or size matching this value. Comparing this address against such bogus
values may provide false positives. This make sure, the value has been
initilized or fail the comparison early and let other parts of the
code deal with the bogus address/size.
Note: To avoid unnecessary checks, we have assumed that if the 'dirty'
member in the same structure is true the location is valid.

This fixes CVE-2018-13867 / Bug #2230.

Signed-off-by: Egbert Eich <eich@suse.com>

2 files changed, 24 insertions, 7 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index bc04d93..a8e9011 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -172,6 +172,21 @@ Bug Fixes since HDF5-1.13.3 release
 ===================================
     Library
     -------
+    - Fix CVE-2018-13867 / GHSA-j8jr-chrh-qfrf
+ 
+      Validate location (offset) of the accumulated metadata when comparing.
+
+      Initially, the accumulated metadata location is initialized to HADDR_UNDEF
+      - the highest available address. Bogus input files may provide a location
+      or size matching this value. Comparing this address against such bogus
+      values may provide false positives. Thus make sure, the value has been
+      initialized or fail the comparison early and let other parts of the
+      code deal with the bogus address/size.
+      Note: To avoid unnecessary checks, it is assumed that if the 'dirty'
+      member in the same structure is true the location is valid.
+
+      (EFE - 2022/10/10 GH-2230)
+
     - Fix CVE-2021-45830 / GHSA-5h2h-fjjr-x9m2
 
       Make H5O__fsinfo_decode() more resilient to out-of-bound reads.
diff --git a/src/H5Faccum.c b/src/H5Faccum.c
index 9e3dfda..5e39215 100644
--- a/src/H5Faccum.c
+++ b/src/H5Faccum.c
@@ -125,8 +125,9 @@ H5F__accum_read(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t si
             HDassert(!accum->buf || (accum->alloc_size >= accum->size));
 
             /* Current read adjoins or overlaps with metadata accumulator */
-            if (H5F_addr_overlap(addr, size, accum->loc, accum->size) || ((addr + size) == accum->loc) ||
-                (accum->loc + accum->size) == addr) {
+            if (H5F_addr_defined(accum->loc) &&
+                (H5F_addr_overlap(addr, size, accum->loc, accum->size) || ((addr + size) == accum->loc) ||
+                 (accum->loc + accum->size) == addr)) {
                 size_t  amount_before; /* Amount to read before current accumulator */
                 haddr_t new_addr;      /* New address of the accumulator buffer */
                 size_t  new_size;      /* New size of the accumulator buffer */
@@ -438,7 +439,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
             /* Check if there is already metadata in the accumulator */
             if (accum->size > 0) {
                 /* Check if the new metadata adjoins the beginning of the current accumulator */
-                if ((addr + size) == accum->loc) {
+                if (H5F_addr_defined(accum->loc) && (addr + size) == accum->loc) {
                     /* Check if we need to adjust accumulator size */
                     if (H5F__accum_adjust(accum, file, H5F_ACCUM_PREPEND, size) < 0)
                         HGOTO_ERROR(H5E_IO, H5E_CANTRESIZE, FAIL, "can't adjust metadata accumulator")
@@ -463,7 +464,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
                     accum->dirty_off = 0;
                 } /* end if */
                 /* Check if the new metadata adjoins the end of the current accumulator */
-                else if (addr == (accum->loc + accum->size)) {
+                else if (H5F_addr_defined(accum->loc) && addr == (accum->loc + accum->size)) {
                     /* Check if we need to adjust accumulator size */
                     if (H5F__accum_adjust(accum, file, H5F_ACCUM_APPEND, size) < 0)
                         HGOTO_ERROR(H5E_IO, H5E_CANTRESIZE, FAIL, "can't adjust metadata accumulator")
@@ -484,7 +485,8 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
                     accum->size += size;
                 } /* end if */
                 /* Check if the piece of metadata being written overlaps the metadata accumulator */
-                else if (H5F_addr_overlap(addr, size, accum->loc, accum->size)) {
+                else if (H5F_addr_defined(accum->loc) &&
+                         H5F_addr_overlap(addr, size, accum->loc, accum->size)) {
                     size_t add_size; /* New size of the accumulator buffer */
 
                     /* Check if the new metadata is entirely within the current accumulator */
@@ -744,7 +746,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
             /* (Note that this could be improved by updating the accumulator
              *  with [some of] the information just read in. -QAK)
              */
-            if (H5F_addr_overlap(addr, size, accum->loc, accum->size)) {
+            if (H5F_addr_defined(accum->loc) && H5F_addr_overlap(addr, size, accum->loc, accum->size)) {
                 /* Check for write starting before beginning of accumulator */
                 if (H5F_addr_le(addr, accum->loc)) {
                     /* Check for write ending within accumulator */
@@ -866,7 +868,7 @@ H5F__accum_free(H5F_shared_t *f_sh, H5FD_mem_t H5_ATTR_UNUSED type, haddr_t addr
     file = f_sh->lf;
 
     /* Adjust the metadata accumulator to remove the freed block, if it overlaps */
-    if ((f_sh->feature_flags & H5FD_FEAT_ACCUMULATE_METADATA) &&
+    if ((f_sh->feature_flags & H5FD_FEAT_ACCUMULATE_METADATA) && H5F_addr_defined(accum->loc) &&
         H5F_addr_overlap(addr, size, accum->loc, accum->size)) {
         size_t overlap_size; /* Size of overlap with accumulator */