Check for overflow when calculating on-disk attribute data size

[e4t]

A bogus hdf5 file may contain dataspace messages with sizes which lead to the on-disk data sizes to exceed what is addressable. When calculating the size, make sure, the multiplication does not
overflow.
The test case was crafted in a way that the overflow caused the size to be 0.

This fixes CVE-2021-37501 / Bug #2458.

Also remove duplicate code.

[byrnHDF]

Shouldn't this macro be surrounded by "#ifndef NDEBUG" like the rest of the macros?
If not then maybe it needs to be a different kind of macro - do we have a section for runtime NDEBUG type macros?

[e4t]

@byrnHDF

Shouldn't this macro be surrounded by "#ifndef NDEBUG" like the rest of the macros? If not then maybe it needs to be a different kind of macro - do we have a section for runtime NDEBUG type macros?

I don't think so. This is not mean for debugging. It is meant to catch 'bogus' HDF5 files which were not created by HDF5 itself. All the CVEs reported for HDF5 so far had hand crafted reproducers, not ones that have been generated with HDF5 itself.

With debugging enabled, I'm sure a lot of bogus files will be identified, but debugging does a lot more and slows down execution even more.

In some environments where HDF5 is used it may be irrelevant to check for bogus and possible malicious data, but in general one may want to have enough validation in place to make sure invalid files are caught.

Now, the problem at hand seems to be rather benign and I don't believe that this overflow can be used for a serious exploit (the reproducer leads to an invalid heap access on read). Also, I'm unable to foresee any way it may. However, people trying to target certain institutions will spend the effort to look for such ways. The fact that HDF5 has caught the attention of folks researching potential security vulnerabilities - I've counted > 60 CVEs over the past 6 years - may suggest that the 'bad guys' could do so likewise.

[e4t]

As for the macro location, if there is a better place to put it, let me know, I'd be happy to move it there. I've created a macro instead of coding it directly as I assumed it may be useful in other places as well.

[byrnHDF]

I don't know the answer, I just noticed something not like the others. We will need a library dev to answer.

[vchoi-hdfgroup]

I suggest a fix in place in H5O__attr_decode() of H5Oattr.c as follows:

• if(ds_size != 0 && ((attr->shared->data_size/ds_size) != dt_size))

•    HGOTO_ERROR(H5E_RESOURCE, H5E_OVERFLOW, NULL, "data size exceeds addressable range")
  

Note that dt_size cannot be zero, if so, it will flag as error few lines back:

if (0 == (dt_size = H5T_get_size(attr->shared->dt)))
    HGOTO_ERROR(H5E_ATTR, H5E_CANTGET, NULL, "unable to get datatype size")

With the above fix, h5dump output on the corrupted file will be something like this:

GROUP "input_1" {h5dump error: error getting attribute information

}

I also suggest to add a test to verify the fix with the corrupted file.

[e4t]

I had created a more generic macro in the hope it may be useful in other cases as well. But it can certainly be done explicitly. If we do so and since we already know that dt_sizew != 0, we could take it as the divisor and simply do:

if ((attr->shared->data_size/dt_size) != ds_size)
       HGOTO_ERROR(...)

[e4t]

Thanks for merging!
@byrnHDF , @vchoi-hdfgroup , @derobins , @lrknox would you like me to add reproducers for CVEs we had fixed earlier as well? I can give it a go, but would only do so if there is interest.

[Mingli-Yu]

Does the change will merge to 1.8 branch or does the hdf5 1.8.21 have the issue? Thanks!