Fix buffer overflow in nbit and scaleoffset filters when not first in the pipeline

release_docs/CHANGELOG.md

@@ -165,6 +165,15 @@ We would like to thank the many HDF5 community members who contributed to this r
 
 ## Library
 
+### Fixed a heap buffer overflow in the N-bit and scale-offset filters
+
+   The N-bit and scale-offset filters assume their input is exactly the size of a full
+   chunk, which is only true when they are the first filter in the pipeline. If another
+   filter such as deflate ran before them, the smaller buffer they received was
+   over-read or over-written when the chunk was written, because the only check was an
+   assert() that is compiled out of release builds. The filters now return an error
+   in that case, which makes the pipeline skip them (they are optional filters) and
+   store the chunk without them.
 ### Fixed H5Pmodify_filter() writing past the filter pipeline for a filter not in it
 
    `H5Pmodify_filter()` returned success when the requested filter was not in the pipeline, and wrote the new flags and client data into the slot one past the last used entry. When the pipeline already held `H5Z_MAX_NFILTERS` filters, that write went past the end of the filter array, and client data longer than `H5Z_COMMON_CD_VALUES` leaked its allocation. `H5Pmodify_filter()` now fails with "filter not in pipeline" in this case.

src/H5Znbit.c

@@ -1016,7 +1016,12 @@ H5Z__filter_nbit(unsigned flags, size_t cd_nelmts, const unsigned cd_values[], s
     } /* end if */
     /* output; compress */
     else {
-        assert(nbytes == (size_t)d_nelmts * (size_t)cd_values[4]);
+        /* The compression loop below walks d_nelmts elements of size cd_values[4], so the
+         * input must be exactly that large.  It is not if another filter (e.g. deflate)
+         * ran earlier in the pipeline and changed the buffer size.
+         */
+        if (nbytes != (size_t)d_nelmts * (size_t)cd_values[4])
+            HGOTO_ERROR(H5E_PLINE, H5E_CANTFILTER, 0, "nbit input size does not match the chunk's datatype");
 
         size_out = nbytes;
 

src/H5Zscaleoffset.c

@@ -1346,7 +1346,13 @@ H5Z__filter_scaleoffset(unsigned flags, size_t cd_nelmts, const unsigned cd_valu
         size_t used_bytes;
         size_t unused_bytes;
 
-        assert(nbytes == (size_t)d_nelmts * (size_t)p.size);
+        /* The pre-compression and packing code below reads d_nelmts elements of size p.size,
+         * so the input must be exactly that large.  It is not if another filter (e.g. deflate)
+         * ran earlier in the pipeline and changed the buffer size.
+         */
+        if (nbytes != (size_t)d_nelmts * (size_t)p.size)
+            HGOTO_ERROR(H5E_PLINE, H5E_CANTFILTER, 0,
+                        "scaleoffset input size does not match the chunk's datatype");
 
         /* before preprocess, convert to memory endianness order if needed */
         if (need_convert)

test/dsets.c

@@ -138,34 +138,34 @@ static const char *FILENAME[] = {"dataset",              /* 0 */
 #ifdef H5_HAVE_FILTER_SZIP
 #define DSET_CAN_APPLY_SZIP_NAME "can_apply_szip"
 #endif /* H5_HAVE_FILTER_SZIP */
-#define DSET_SET_LOCAL_NAME                  "set_local"
-#define DSET_SET_LOCAL_NAME_2                "set_local_2"
-#define DSET_SET_LOCAL_UPDATES_CD_NAME       "set_local_updates_cd"
-#define DSET_SET_LOCAL_UPDATES_CD_VLEN_NAME  "set_local_updates_cd_vlen"
-#define DSET_DEFLATE_VLEN_NAME               "deflate_vlen"
-#define DSET_ONEBYTE_SHUF_NAME               "onebyte_shuffle"
-#define DSET_NBIT_INT_NAME                   "nbit_int"
-#define DSET_NBIT_FLOAT_NAME                 "nbit_float"
-#define DSET_NBIT_DOUBLE_NAME                "nbit_double"
-#define DSET_NBIT_ARRAY_NAME                 "nbit_array"
-#define DSET_NBIT_COMPOUND_NAME              "nbit_compound"
-#define DSET_NBIT_COMPOUND_NAME_2            "nbit_compound_2"
-#define DSET_NBIT_COMPOUND_NAME_3            "nbit_compound_3"
-#define DSET_NBIT_INT_SIZE_NAME              "nbit_int_size"
-#define DSET_NBIT_FLT_SIZE_NAME              "nbit_flt_size"
-#define DSET_SCALEOFFSET_INT_NAME            "scaleoffset_int"
-#define DSET_SCALEOFFSET_INT_NAME_2          "scaleoffset_int_2"
-#define DSET_SCALEOFFSET_FLOAT_NAME          "scaleoffset_float"
-#define DSET_SCALEOFFSET_FLOAT_NAME_2        "scaleoffset_float_2"
-#define DSET_SCALEOFFSET_DOUBLE_NAME         "scaleoffset_double"
-#define DSET_SCALEOFFSET_DOUBLE_NAME_2       "scaleoffset_double_2"
-#define DSET_SCALEOFFSET_INT_FULLPREC_NAME   "scaleoffset_int_fullprec"
-#define DSET_SCALEOFFSET_FLOAT_FULLPREC_NAME "scaleoffset_float_fullprec"
-#define DSET_COMPARE_DCPL_NAME               "compare_dcpl"
-#define DSET_COMPARE_DCPL_NAME_2             "compare_dcpl_2"
-#define DSET_COPY_DCPL_NAME_1                "copy_dcpl_1"
-#define DSET_COPY_DCPL_NAME_2                "copy_dcpl_2"
-#define COPY_DCPL_EXTFILE_NAME               "ext_file"
+#define DSET_SET_LOCAL_NAME                 "set_local"
+#define DSET_SET_LOCAL_NAME_2               "set_local_2"
+#define DSET_SET_LOCAL_UPDATES_CD_NAME      "set_local_updates_cd"
+#define DSET_SET_LOCAL_UPDATES_CD_VLEN_NAME "set_local_updates_cd_vlen"
+#define DSET_DEFLATE_VLEN_NAME              "deflate_vlen"
+#define DSET_ONEBYTE_SHUF_NAME              "onebyte_shuffle"
+#define DSET_NBIT_INT_NAME                  "nbit_int"
+#define DSET_NBIT_FLOAT_NAME                "nbit_float"
+#define DSET_NBIT_DOUBLE_NAME               "nbit_double"
+#define DSET_NBIT_ARRAY_NAME                "nbit_array"
+#define DSET_NBIT_COMPOUND_NAME             "nbit_compound"
+#define DSET_NBIT_COMPOUND_NAME_2           "nbit_compound_2"
+#define DSET_NBIT_COMPOUND_NAME_3           "nbit_compound_3"
+#define DSET_NBIT_INT_SIZE_NAME             "nbit_int_size"
+#define DSET_NBIT_FLT_SIZE_NAME             "nbit_flt_size"
+#define DSET_NBIT_AFTER_DEFLATE_NAME        "nbit_after_deflate"
+#define DSET_SO_AFTER_DEFLATE_NAME          "scaleoffset_after_deflate"
+#define DSET_SCALEOFFSET_INT_NAME           "scaleoffset_int"
+#define DSET_SCALEOFFSET_INT_NAME_2         "scaleoffset_int_2"
+#define DSET_SCALEOFFSET_FLOAT_NAME         "scaleoffset_float"
+#define DSET_SCALEOFFSET_FLOAT_NAME_2       "scaleoffset_float_2"
+#define DSET_SCALEOFFSET_DOUBLE_NAME        "scaleoffset_double"
+#define DSET_SCALEOFFSET_DOUBLE_NAME_2      "scaleoffset_double_2"
+#define DSET_COMPARE_DCPL_NAME              "compare_dcpl"
+#define DSET_COMPARE_DCPL_NAME_2            "compare_dcpl_2"
+#define DSET_COPY_DCPL_NAME_1               "copy_dcpl_1"
+#define DSET_COPY_DCPL_NAME_2               "copy_dcpl_2"
+#define COPY_DCPL_EXTFILE_NAME              "ext_file"
 #ifndef H5_NO_DEPRECATED_SYMBOLS
 #define DSET_DEPREC_NAME         "deprecated"
 #define DSET_DEPREC_NAME_CHUNKED "deprecated_chunked"
@@ -5219,6 +5219,144 @@ test_nbit_flt_size(hid_t file)
     return FAIL;
 } /* end test_nbit_flt_size() */
 
+/*-------------------------------------------------------------------------
+ * Function:    test_filter_after_deflate
+ *
+ * Purpose:     Tests the nbit and scaleoffset filters when they come after
+ *              the deflate filter in the pipeline.
+ *
+ *              These filters expect a chunk of exactly nelmts * datatype
+ *              size bytes, which is only true when they are the first
+ *              filter.  Deflate shrinks the chunk, so the input they get
+ *              here is smaller than that.  The data is chosen so that
+ *              deflate does shrink it.  The filters must reject such an
+ *              input (and be skipped, as they are optional) instead of
+ *              reading or writing past the end of their buffers.  The
+ *              data must round-trip either way.
+ *
+ * Return:      Success:        0
+ *              Failure:        -1
+ *-------------------------------------------------------------------------
+ */
+#define AFTER_DEFLATE_NELMTS 64
+static herr_t
+test_filter_after_deflate(hid_t file)
+{
+#ifdef H5_HAVE_FILTER_DEFLATE
+    const char *names[2] = {DSET_NBIT_AFTER_DEFLATE_NAME, DSET_SO_AFTER_DEFLATE_NAME};
+    hsize_t     size[1]  = {AFTER_DEFLATE_NELMTS};
+    int         orig_data[AFTER_DEFLATE_NELMTS];
+    int         new_data[AFTER_DEFLATE_NELMTS];
+    size_t      precision = 17, offset = 4;
+    size_t      i;
+    int         which;
+    unsigned    filter_mask = 0;
+    hid_t       dataset = H5I_INVALID_HID, datatype = H5I_INVALID_HID, space = H5I_INVALID_HID;
+    hid_t       dc = H5I_INVALID_HID;
+
+    TESTING("nbit and scaleoffset filters after deflate");
+
+    /* test_missing_filter() unregisters deflate for the rest of the run */
+    if (H5Zfilter_avail(H5Z_FILTER_DEFLATE) != true) {
+        SKIPPED();
+        puts("    Deflate filter not available");
+        return SUCCEED;
+    }
+
+    /* Few distinct values, so deflate shrinks the chunk well below its raw size */
+    for (i = 0; i < AFTER_DEFLATE_NELMTS; i++)
+        orig_data[i] = (int)(i % 4) << offset;
+
+    if ((space = H5Screate_simple(1, size, NULL)) < 0)
+        TEST_ERROR;
+
+    for (which = 0; which < 2; which++) {
+        if ((datatype = H5Tcopy(H5T_NATIVE_INT)) < 0)
+            TEST_ERROR;
+        if (which == 0) {
+            if (H5Tset_precision(datatype, precision) < 0)
+                TEST_ERROR;
+            if (H5Tset_offset(datatype, offset) < 0)
+                TEST_ERROR;
+        }
+
+        if ((dc = H5Pcreate(H5P_DATASET_CREATE)) < 0)
+            TEST_ERROR;
+        if (H5Pset_chunk(dc, 1, size) < 0)
+            TEST_ERROR;
+        if (H5Pset_deflate(dc, 6) < 0)
+            TEST_ERROR;
+        if (which == 0) {
+            if (H5Pset_nbit(dc) < 0)
+                TEST_ERROR;
+        }
+        else if (H5Pset_scaleoffset(dc, H5Z_SO_INT, H5Z_SO_INT_MINBITS_DEFAULT) < 0)
+            TEST_ERROR;
+
+        if ((dataset = H5Dcreate2(file, names[which], datatype, space, H5P_DEFAULT, dc, H5P_DEFAULT)) < 0)
+            TEST_ERROR;
+        if (H5Dwrite(dataset, H5T_NATIVE_INT, H5S_ALL, H5S_ALL, H5P_DEFAULT, orig_data) < 0)
+            TEST_ERROR;
+        /* The chunk is filtered when it is evicted from the chunk cache */
+        if (H5Dclose(dataset) < 0)
+            TEST_ERROR;
+
+        if ((dataset = H5Dopen2(file, names[which], H5P_DEFAULT)) < 0)
+            TEST_ERROR;
+
+        /* Deflate (filter 0) must have run, otherwise the case above was not
+         * exercised; the second filter must have been skipped */
+        if (H5Dget_chunk_info(dataset, H5S_ALL, 0, NULL, &filter_mask, NULL, NULL) < 0)
+            TEST_ERROR;
+        if (filter_mask != 0x2) {
+            H5_FAILED();
+            printf("    %s: unexpected chunk filter mask 0x%x\n", names[which], filter_mask);
+            goto error;
+        }
+
+        memset(new_data, 0, sizeof(new_data));
+        if (H5Dread(dataset, H5T_NATIVE_INT, H5S_ALL, H5S_ALL, H5P_DEFAULT, new_data) < 0)
+            TEST_ERROR;
+        for (i = 0; i < AFTER_DEFLATE_NELMTS; i++)
+            if (new_data[i] != orig_data[i]) {
+                H5_FAILED();
+                printf("    %s: read different values than written at index %lu\n", names[which],
+                       (unsigned long)i);
+                goto error;
+            }
+
+        if (H5Dclose(dataset) < 0)
+            TEST_ERROR;
+        if (H5Pclose(dc) < 0)
+            TEST_ERROR;
+        if (H5Tclose(datatype) < 0)
+            TEST_ERROR;
+    }
+
+    if (H5Sclose(space) < 0)
+        TEST_ERROR;
+
+    PASSED();
+    return SUCCEED;
+
+error:
+    H5E_BEGIN_TRY
+    {
+        H5Dclose(dataset);
+        H5Pclose(dc);
+        H5Tclose(datatype);
+        H5Sclose(space);
+    }
+    H5E_END_TRY
+    return FAIL;
+#else
+    TESTING("nbit and scaleoffset filters after deflate");
+    SKIPPED();
+    puts("    Deflate filter not enabled");
+    return SUCCEED;
+#endif
+} /* end test_filter_after_deflate() */
+
 /*-------------------------------------------------------------------------
  * Function:    test_scaleoffset_int
  *
@@ -21596,6 +21734,7 @@ main(void)
                     nerrors += (test_nbit_compound_3(file) < 0 ? 1 : 0);
                     nerrors += (test_nbit_int_size(file) < 0 ? 1 : 0);
                     nerrors += (test_nbit_flt_size(file) < 0 ? 1 : 0);
+                    nerrors += (test_filter_after_deflate(file) < 0 ? 1 : 0);
                     nerrors += (test_scaleoffset_int(file) < 0 ? 1 : 0);
                     nerrors += (test_scaleoffset_int_2(file) < 0 ? 1 : 0);
                     nerrors += (test_scaleoffset_float(file) < 0 ? 1 : 0);