Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential buffer read overflows in H5PB_read #4279

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

jhendersonHDF
Copy link
Collaborator

H5PB_read previously did not account for the fact that the size of the read it's performing could overflow the page buffer pointer, depending on the calculated offset for the read. This has been fixed by adjusting the size of the read if it's determined that it would overflow the page.

H5PB_read previously did not account for the fact that the size of the
read it's performing could overflow the page buffer pointer, depending
on the calculated offset for the read. This has been fixed by adjusting
the size of the read if it's determined that it would overflow the page.
@jhendersonHDF jhendersonHDF added Merge - To 1.14 Priority - 1. High 🔼 These are important issues that should be resolved in the next release Component - C Library Core C library issues (usually in the src directory) Type - Bug Please report security issues to help@hdfgroup.org instead of creating an issue on GitHub labels Mar 29, 2024
@jhendersonHDF
Copy link
Collaborator Author

Found while testing fheap with AddressSanitizer at express test level 0 with free lists disabled. One of the tests was overflowing a page buffer during a read. In the trace below, H5F__super_ext_remove_msg caused the metadata cache to perform a speculative 512-byte read of an object header at address 3610 in the file, overflowing the page that has a size of 4096 bytes. This calculates whether the read will overflow the page and adjusts the read size as appropriate to cover just the rest of the page.

=================================================================
==20034==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210012d8d00 at pc 0x7fbecac6de02 bp 0x7ffe7b0e2330 sp 0x7ffe7b0e1af0
READ of size 512 at 0x6210012d8d00 thread T0
    #0 0x7fbecac6de01 in __interceptor_memcpy (/usr/lib64/libasan.so.8+0x6de01)
    #1 0x7fbeca0123ca in H5MM_memcpy /home/jhenderson/git/hdf5/src/H5MM.c:256
    #2 0x7fbeca196895 in H5PB_read /home/jhenderson/git/hdf5/src/H5PB.c:842
    #3 0x7fbec9e06b10 in H5F_block_read /home/jhenderson/git/hdf5/src/H5Fio.c:137
    #4 0x7fbec9c17252 in H5C__load_entry /home/jhenderson/git/hdf5/src/H5Centry.c:1065
    #5 0x7fbec9c38403 in H5C_protect /home/jhenderson/git/hdf5/src/H5Centry.c:3101
    #6 0x7fbec9ba3592 in H5AC_protect /home/jhenderson/git/hdf5/src/H5AC.c:1276
    #7 0x7fbeca0a786d in H5O_protect /home/jhenderson/git/hdf5/src/H5Oint.c:988
    #8 0x7fbeca0ca703 in H5O_msg_exists /home/jhenderson/git/hdf5/src/H5Omessage.c:787
    #9 0x7fbec9e17324 in H5F__super_ext_remove_msg /home/jhenderson/git/hdf5/src/H5Fsuper.c:1786
    #10 0x7fbeca00507a in H5MF_settle_raw_data_fsm /home/jhenderson/git/hdf5/src/H5MF.c:2740
    #11 0x7fbec9bfc7eb in H5C_flush_cache /home/jhenderson/git/hdf5/src/H5C.c:686
    #12 0x7fbec9ba040d in H5AC_flush /home/jhenderson/git/hdf5/src/H5AC.c:619
    #13 0x7fbec9df03e0 in H5F__flush_phase2 /home/jhenderson/git/hdf5/src/H5Fint.c:2289
    #14 0x7fbec9df16f0 in H5F__dest /home/jhenderson/git/hdf5/src/H5Fint.c:1419
    #15 0x7fbec9df77e7 in H5F_try_close /home/jhenderson/git/hdf5/src/H5Fint.c:2628
    #16 0x7fbec9df8413 in H5F__close /home/jhenderson/git/hdf5/src/H5Fint.c:2430
    #17 0x7fbeca4b9c41 in H5VL__native_file_close /home/jhenderson/git/hdf5/src/H5VLnative_file.c:782
    #18 0x7fbeca46df13 in H5VL__file_close /home/jhenderson/git/hdf5/src/H5VLcallback.c:4309
    #19 0x7fbeca487340 in H5VL_file_close /home/jhenderson/git/hdf5/src/H5VLcallback.c:4340
    #20 0x7fbec9decc18 in H5F__close_cb /home/jhenderson/git/hdf5/src/H5Fint.c:217
    #21 0x7fbec9fccb3d in H5I__dec_ref /home/jhenderson/git/hdf5/src/H5Iint.c:973
    #22 0x7fbec9fccdf0 in H5I__dec_app_ref /home/jhenderson/git/hdf5/src/H5Iint.c:1044
    #23 0x7fbec9fcd08a in H5I_dec_app_ref /home/jhenderson/git/hdf5/src/H5Iint.c:1089
    #24 0x7fbec9dd0998 in H5Fclose /home/jhenderson/git/hdf5/src/H5F.c:1059
    #25 0x4059af in reopen_file /home/jhenderson/git/hdf5/test/fheap.c:559
    #26 0x407481 in test_man_start_5th_recursive_indirect /home/jhenderson/git/hdf5/test/fheap.c:6124
    #27 0x40f5a7 in main /home/jhenderson/git/hdf5/test/fheap.c:16209
    #28 0x7fbec962c6b6 in __libc_start_call_main (/lib64/libc.so.6+0x236b6)
    #29 0x7fbec962c774 in __libc_start_main_alias_1 (/lib64/libc.so.6+0x23774)
    #30 0x402570 in _start ../sysdeps/x86_64/start.S:115

0x6210012d8d00 is located 0 bytes after 4096-byte region [0x6210012d7d00,0x6210012d8d00)
allocated by thread T0 here:
    #0 0x7fbecacd7f97 in calloc (/usr/lib64/libasan.so.8+0xd7f97)
    #1 0x7fbeca198f5e in H5PB_write /home/jhenderson/git/hdf5/src/H5PB.c:1220

@lrknox lrknox merged commit 50d30bd into HDFGroup:develop Mar 29, 2024
54 checks passed
lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Mar 29, 2024
H5PB_read previously did not account for the fact that the size of the
read it's performing could overflow the page buffer pointer, depending
on the calculated offset for the read. This has been fixed by adjusting
the size of the read if it's determined that it would overflow the page.
lrknox added a commit that referenced this pull request Mar 29, 2024
* Take user block into account when returning chunk addresses (#4236)

Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take
the size of the user block into account when reporting addresses. Since
the #1 use of these functions is to root around in the file for the raw
data, this is kind of a problem.

Fixes GitHub issue #3003

* Fix a minor warning in h5test.c (#4242)

* Turn on -Werror for Java in GitHub -Werror workflows (#4243)

* Update Windows CI to not install ninja (#4230)

* Rework Fortran macros to use the proper code. (#4240)

* Correct reference copy for 16 API (#4244)

* Determine MPI LOGICAL during build, used in tests. (#4246)

* Skip userblock test in chunk_info.c for multi-file VFDs (#4249)

* Match generators with real cmake -G output on Windows (#4252)

* Add Julia GitHub Actions. (#4123)

* Re-revert to using autoreconf in autogen.sh (#4253)

We previously tried removing the per-tool invocation of the Autotools
and instead simply invoked autoreconf (PR #1906). This was reverted
when it turned out that the NAG Fortran compiler had trouble with an
undecorated -shared linker flag.

It turns out that this is due to a bug in libtool 2.4.2 and earlier.
Since this version of libtool is over a decade old, we're un-reverting
the change. We've added a release note for anyone who has to build
from source on elderly platforms.

Fixes #1343

* Rewrite H5T__path_find_real for clarity (#4225)

* Move conversion path free logic to helper function

* Add tgz extensions on names (#4255)

* Remove an error check regarding large cache objects (#4254)

* Remove an error check regarding large cache objects

In PR#4231 an assert() call was converted to a normal HDF5 error
check. It turns out that the original assert() was added by a
developer as a way of being alerted that large cache objects
existed instead of as a guard against incorrect behavior, making
it unnecessary in either debug or release builds.

The error check has been removed.

* Update RELEASE.txt

* File format security issues (#4234)

* Add job timeout to cygwin workflow (#4260)

* Replace user-define with user-defined (#4261)

* Improve the CMake clang -fsanitize=memory flags (#4267)

-fsanitize=memory is almost useless without
using -fsanitize-memory-track-origins=2 and we shoud probably add
-fno-optimize-sibling-calls as well.

* Add documentation (H5M) (#4259)

* Add documentation (H5P) (#4262)

* MPI type correction (#4268)

* corrected type for MPI_*_f2c APIs

* fixed return type of callback

* reset compilation flags of logical test program

* Clean up test/cmpd_dtransform.c (#4270)

* Clean up test/cmpd_dtransform.c

* Fix uninitialized memory warning from sanitizers
* FAIL_STACK_ERROR --> TEST_ERROR
* Emit output
* Delete test file when done

* Fix typo

* H5Fdelete() --> remove()

* Fix uninitialized memory issues in packet table (#4271)

* replace deprecated CMAKE_COMPILER_IS_GNU** (#4272)

* Prevent stack overflows in H5E__push_stack (#4264)

* Minor fixes after merge of file format security fixes (#4263)

* Update H5_IS_BUFFER_OVERFLOW to account for 'size' of 0

* Invert a few checks to avoid function call

* CHECK --> CHECK_PTR in tmisc.c (#4274)

* Add release note for CVE-2017-17507 (#4275)

* Update Cygwin installation guide (#4265)

* Addresses configuration fortran testing flags (#4276)

* turn warnings to errors in fortran configure test

* Intel fortran test fix

* Merge julia workflows into standard ci format (#4273)

* Fix range check in H5_addr_overlap (#4278)

When the H5_addr_overlap macro was updated to use H5_RANGE_OVERLAP,
it failed to take into account that H5_RANGE_OVERLAP expects the
range to be inclusive. This lead to an assertion failure in
H5MM_memcpy due to a memcpy operation on overlapping memory.
This has been fixed by subtracting 1 from the calculated high
bound values passed to H5_RANGE_OVERLAP

* Fix potential buffer read overflows in H5PB_read (#4279)

H5PB_read previously did not account for the fact that the size of the
read it's performing could overflow the page buffer pointer, depending
on the calculated offset for the read. This has been fixed by adjusting
the size of the read if it's determined that it would overflow the page.
@jhendersonHDF jhendersonHDF deleted the pb_read_overflow branch April 4, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 1. High 🔼 These are important issues that should be resolved in the next release Type - Bug Please report security issues to help@hdfgroup.org instead of creating an issue on GitHub
Projects
Status: Needs Merged
Development

Successfully merging this pull request may close these issues.

3 participants