Fix HDFFV-10591

Description:
    h52gif produced a segfault when a buffer overflow occurred because
    the data size was corrupted and became very large.  This commit added
    a check on the data size against the buffer size to prevent the segfault.
    It also added error reporting to h52gif to display an error message
    instead of silently exiting when the failure occurred.
Platforms tested:
    Linux/64 (jelly)

4 files changed, 63 insertions, 31 deletions

diff --git a/hl/src/H5IM.c b/hl/src/H5IM.c
index 2a7ed9b..6f7414b 100644
--- a/hl/src/H5IM.c
+++ b/hl/src/H5IM.c
@@ -274,7 +274,8 @@ herr_t H5IMget_image_info( hid_t loc_id,
         return -1;
 
     /* Try to find the attribute "INTERLACE_MODE" on the >>image<< dataset */
-    has_attr = H5LT_find_attribute(did, "INTERLACE_MODE");
+    if ((has_attr = H5LT_find_attribute(did, "INTERLACE_MODE")) < 0)
+        goto out;
 
     /* It exists, get it */
     if(has_attr == 1)
diff --git a/hl/tools/gif2h5/hdf2gif.c b/hl/tools/gif2h5/hdf2gif.c
index e32facb..f1a9593 100644
--- a/hl/tools/gif2h5/hdf2gif.c
+++ b/hl/tools/gif2h5/hdf2gif.c
@@ -18,8 +18,8 @@
 #include "h5tools.h"
 #include "h5tools_utils.h"
 
-#define IMAGE_WIDTH_MAX		65535	/* unsigned 16bits integer */
-#define IMAGE_HEIGHT_MAX	65535	/* unsigned 16bits integer */
+#define IMAGE_WIDTH_MAX     65535   /* unsigned 16bits integer */
+#define IMAGE_HEIGHT_MAX    65535   /* unsigned 16bits integer */
 
 
 int EndianOrder;
@@ -149,30 +149,41 @@ int main(int argc , char **argv)
             goto out;
         }
 
-        /* read image */
+        /* get image's information */
         if ( H5IMget_image_info( fid, image_name, &width, &height, &planes, interlace, &npals ) < 0 )
+        {
+            fprintf(stderr , "Unable to get information of the image. Aborting.\n");
             goto out;
+        }
 
-	if (width > IMAGE_WIDTH_MAX || height > IMAGE_HEIGHT_MAX){
-	    fprintf(stderr, "HDF5 image is too large. Limit is %d by %d.\n", IMAGE_WIDTH_MAX, IMAGE_HEIGHT_MAX);
-	    goto out;
-	}
+        if (width > IMAGE_WIDTH_MAX || height > IMAGE_HEIGHT_MAX)
+        {
+            fprintf(stderr, "HDF5 image is too large. Limit is %d by %d.\n", IMAGE_WIDTH_MAX, IMAGE_HEIGHT_MAX);
+            goto out;
+        }
 
-	/* tool can handle single plane images only. */
-	if (planes > 1){
-	    fprintf(stderr, "Cannot handle multiple planes image\n");
-	    goto out;
-	}
+        /* tool can handle single plane images only. */
+        if (planes > 1)
+        {
+            fprintf(stderr, "Cannot handle multiple planes image\n");
+            goto out;
+        }
 
         Image = (GIFBYTE*) malloc( (size_t) width * (size_t) height );
 
         if ( H5IMread_image( fid, image_name, Image ) < 0 )
+        {
+            fprintf(stderr , "Unable to read the image. Aborting.\n");
             goto out;
+        }
 
         if (npals)
         {
             if ( H5IMget_palette_info( fid, image_name, 0, pal_dims ) < 0 )
+            {
+                fprintf(stderr , "Unable to get information of the palette. Aborting.\n");
                 goto out;
+            }
 
             pal = (GIFBYTE*) malloc( (size_t) pal_dims[0] * (size_t) pal_dims[1] );
 
@@ -211,9 +222,9 @@ int main(int argc , char **argv)
             numcols = 256;
             for (i = 0 ; i < numcols ; i++)
             {
-	      Red[i] = (GIFBYTE)(255 - i);
-	      Green[i] = (GIFBYTE)(255 - i);
-	      Blue[i] = (GIFBYTE)(255 - i);
+                Red[i] = (GIFBYTE)(255 - i);
+                Green[i] = (GIFBYTE)(255 - i);
+                Blue[i] = (GIFBYTE)(255 - i);
             }
         }
         else
@@ -246,7 +257,7 @@ int main(int argc , char **argv)
             if (j==i)
             {
                 /* wasn't found */
-	      pc2nc[i] = (GIFBYTE)nc;
+                pc2nc[i] = (GIFBYTE)nc;
                 r1[nc] = Red[i];
                 g1[nc] = Green[i];
                 b1[nc] = Blue[i];
diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index ab601f5..85359cd 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -317,6 +317,18 @@ Bug Fixes since HDF5-1.10.5 release
 
     Library
     -------
+    - Fixed CVE-2018-17435
+
+      The tool h52gif produced a segfault when the size of an attribute
+      message was corrupted and caused a buffer overflow.
+
+      The problem was fixed by verifying the attribute message's size
+      against the buffer size before accessing the buffer.  h52gif was
+      also fixed to display the failure instead of silently exiting
+      after the segfault was eliminated.
+
+      (BMR - 2020/6/19, HDFFV-10591)
+
     - Improved peformance when creating a large number of small datasets by
       retrieving default property values from the API context instead of doing
       skip list searches.
diff --git a/src/H5Oattr.c b/src/H5Oattr.c
index 7a17c6a..c8dd822 100644
--- a/src/H5Oattr.c
+++ b/src/H5Oattr.c
@@ -109,7 +109,10 @@ H5FL_EXTERN(H5S_extent_t);
  USAGE
     void *H5O_attr_decode(f, mesg_flags, p)
         H5F_t *f;               IN: pointer to the HDF5 file struct
-        unsigned mesg_flags;    IN: Message flags to influence decoding
+        H5O_t    *open_oh;      IN: pointer to the object header
+        unsigned mesg_flags;    IN: message flags to influence decoding
+        unsigned *ioflags;      IN: flags for decoding
+        size_t   p_size;        IN: size of buffer *p
         const uint8_t *p;       IN: the raw information buffer
  RETURNS
     Pointer to the new message in native order on success, NULL on failure
@@ -120,16 +123,16 @@ H5FL_EXTERN(H5S_extent_t);
 --------------------------------------------------------------------------*/
 static void *
 H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
-    unsigned *ioflags, size_t H5_ATTR_UNUSED p_size, const uint8_t *p)
+    unsigned *ioflags, size_t p_size, const uint8_t *p)
 {
-    H5A_t		*attr = NULL;
-    H5S_extent_t	*extent;	/*extent dimensionality information  */
-    size_t		name_len;   	/*attribute name length */
-    size_t		dt_size;   	/* Datatype size */
-    hssize_t		sds_size;   	/* Signed Dataspace size */
-    hsize_t		ds_size;   	/* Dataspace size */
-    unsigned            flags = 0;      /* Attribute flags */
-    H5A_t		*ret_value = NULL;      /* Return value */
+    H5A_t        *attr = NULL;
+    H5S_extent_t *extent;       /*extent dimensionality information  */
+    size_t       name_len;      /*attribute name length */
+    size_t       dt_size;       /* Datatype size */
+    hssize_t     sds_size;      /* Signed Dataspace size */
+    hsize_t      ds_size;       /* Dataspace size */
+    unsigned     flags = 0;     /* Attribute flags */
+    H5A_t        *ret_value = NULL;      /* Return value */
 
     FUNC_ENTER_NOAPI_NOINIT
 
@@ -138,7 +141,7 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
     HDassert(p);
 
     if(NULL == (attr = H5FL_CALLOC(H5A_t)))
-	HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed")
+        HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed")
 
     if(NULL == (attr->shared = H5FL_CALLOC(H5A_shared_t)))
         HGOTO_ERROR(H5E_FILE, H5E_NOSPACE, NULL, "can't allocate shared attr structure")
@@ -146,7 +149,7 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
     /* Version number */
     attr->shared->version = *p++;
     if(attr->shared->version < H5O_ATTR_VERSION_1 || attr->shared->version > H5O_ATTR_VERSION_LATEST)
-	HGOTO_ERROR(H5E_ATTR, H5E_CANTLOAD, NULL, "bad version number for attribute message")
+        HGOTO_ERROR(H5E_ATTR, H5E_CANTLOAD, NULL, "bad version number for attribute message")
 
     /* Get the flags byte if we have a later version of the attribute */
     if(attr->shared->version >= H5O_ATTR_VERSION_2) {
@@ -176,7 +179,7 @@ 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))
@@ -200,7 +203,7 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
      * What's actually shared, though, is only the extent.
      */
     if(NULL == (attr->shared->ds = H5FL_CALLOC(H5S_t)))
-	HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed")
+        HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed")
 
     /* Decode attribute's dataspace extent */
     if((extent = (H5S_extent_t *)(H5O_MSG_SDSPACE->decode)(f, open_oh,
@@ -238,6 +241,11 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags,
 
     /* Go get the data */
     if(attr->shared->data_size) {
+        /* Ensure that data size doesn't exceed buffer size, in case of
+           it's being corrupted in the file */
+        if(attr->shared->data_size > p_size)
+            HGOTO_ERROR(H5E_RESOURCE, H5E_OVERFLOW, NULL, "data size exceeds buffer size")
+
         if(NULL == (attr->shared->data = H5FL_BLK_MALLOC(attr_buf, attr->shared->data_size)))
             HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed")
         H5MM_memcpy(attr->shared->data, p, attr->shared->data_size);