UndefinedBehaviorSanitizer: multiple signed integer overflow #833

pietroborrello · 2022-04-29T15:34:13Z

Describe the bug

UndefinedBehaviorSanitizer: multiple signed integers overflow in the codebase. I attach different testcases that trigger different overflows.

To Reproduce

Built libsndfile using clang-10 according to the oss-fuzz script with CXXFLAGS='-O1 -fsanitize=address -fsanitize=array-bounds,bool,builtin,enum,float-divide-by-zero,function,integer-divide-by-zero,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr'

commit: 4b01368

Example UBSAN Output

$ ./sndfile_alt_fuzzer id:000006,sig:06,src:001126,time:22443,op:havoc,rep:4,trial:0
INFO: Seed: 3120870912
INFO: Loaded 1 modules   (33759 inline 8-bit counters): 33759 [0x8977c3, 0x89fba2), 
INFO: Loaded 1 PC tables (33759 PCs): 33759 [0x6f6b48,0x77a938), 
sndfile_alt_fuzzer: Running 1 inputs 1 time(s) each.
Running: id:000006,sig:06,src:001126,time:22443,op:havoc,rep:4,trial:0
src/au.c:324:54: runtime error: signed integer overflow: 1684960000 + 779316836 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/au.c:324:54 in 
src/au.c:326:29: runtime error: signed integer overflow: 1684960000 + 779316836 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/au.c:326:29 in 
src/au.c:327:40: runtime error: signed integer overflow: 1684960000 + 779316836 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/au.c:327:40 in 
Executed id:000006,sig:06,src:001126,time:22443,op:havoc,rep:4,trial:0 in 1 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***

or for example

$ ./sndfile_alt_fuzzer id:000011,sig:06,src:001848,time:296603,op:havoc,rep:4,trial:4
INFO: Seed: 3162641945
INFO: Loaded 1 modules   (33759 inline 8-bit counters): 33759 [0x8977c3, 0x89fba2), 
INFO: Loaded 1 PC tables (33759 PCs): 33759 [0x6f6b48,0x77a938), 
sndfile_alt_fuzzer: Running 1 inputs 1 time(s) each.
Running: id:000011,sig:06,src:001848,time:296603,op:havoc,rep:4,trial:4
src/mat4.c:323:41: runtime error: signed integer overflow: -587202559 * 553648128 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:41 in 
src/mat4.c:323:48: runtime error: signed integer overflow: 553648128 * 8 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:48 in 
src/mat4.c:107:35: runtime error: signed integer overflow: 8 * -587202559 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:107:35 in 
Executed id:000011,sig:06,src:001848,time:296603,op:havoc,rep:4,trial:4 in 1 ms

testcases:
int overflow.zip

The text was updated successfully, but these errors were encountered:

evpobr · 2022-05-02T07:40:06Z

Looks like OSS-Fuzz 25359.

evpobr · 2022-05-02T07:42:38Z

Dublicate of #789

evpobr · 2022-05-02T07:49:56Z

Closed as dublicate.

pietroborrello · 2022-05-02T08:17:18Z

Hello, thank you for your reply. The issue 789 linked seems a subset of the bugs reported here. Not sure about the oss-fuzz report, as I do not have access to it

The clang sanitizer warns of a possible signed integer overflow when calculating the `dataend` value in `mat4_read_header()`. ``` src/mat4.c:323:41: runtime error: signed integer overflow: 205 * -100663296 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:41 in src/mat4.c:323:48: runtime error: signed integer overflow: 838860800 * 4 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:48 in ``` Cast the offending `rows` and `cols` ints to `st_count_t` (the type of `dataend` before performing the calculation, to avoid the issue. CVE: CVE-2022-33065 Fixes: libsndfile#789 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

At several points in au_read_header(), we calculate the functional end of the data segment by adding the (int)au_fmt.dataoffset and the (int)au_fmt.datasize. This can overflow the implicit int_32 return value and cause undefined behavior. Instead, precalculate the value and assign it to a 64-bit (st_count_t)data_end variable. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast hdr.frames to st_count_t, to provide the calculation with enough numeric space to avoid an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The sds_2byte_read() function seems to be composing its uint_32 sample buffer by shifting the kth and kth+1 bytes into the top 2 bytes of the sample. It does this by adding the left-shifted bytes together, which while reasonable in this context, scares the clang sanitizer into thinking that the ints might overflow. Instead, bitwise-OR the bytes together - which should accomplish the same arithmetic operation, without risking an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

aiff_read_basc_chunk() tries to count the AIFF header size by keeping track of the bytes returned by psf_binheader_readf(). Though improbable, it is technically possible for these added bytes to exceed the int-sized `count` accumulator. Use a 64-bit sf_count_t type for `count`, to ensure that it always has enough numeric space. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When reading the IRCAM header, it is possible for the calculated blockwidth to exceed the bounds of a signed int32. Use a 64bit st_count_t to store the blockwidth. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast the components of the blockwidth calculation to sf_count_t to avoid overflowing integers during calculation. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The psf_binheader_readf() function attempts to count and return the number of bytes traversed in the header. During this accumulation, it is possible to overflow the int-sized byte_count variable. Avoid this overflow by checking that the accumulated bytes do not exceed INT_MAX and throwing an error if they do. This implies that files with multi-gigabyte headers threaten to produce this error, but I imagine those files don't really exist - and this error is better than the undefined behavior which would have resulted previously. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When computing the ADPCM-state signal-estimate in the nms_adpcm_update() function, it is possible for the signal-state components to overflow their integer bounds, leading to undefined behavior. Calculate the signal estimate within an int64_t to provide enough numeric space, and clamp the resulting value to within the int boundaries. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When calculating sf.frames from the blocks_total PNMS variable, it is theoretically possible to overflow the blocks_total int boundaries, leading to undefined behavior. Cast blocks_total to a long-sized sf_count_t before the calculation, to provide it with enough numeric space and because that is the final typing regardless. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Cast the int-sized bytewidth variable to a long-sized sf_count_t type prior to calculating the blockwidth, to provide the calculation with enough numeric space and sf_count_t is the final typing regardless. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When checking for mismatches between the filelength and riff_size, it is possible to overflow the temporary riff_size value used in the comparison by adding a static offset; which is probably fine, but it is offensive to overflow fuzzers. Since filelength is always a positive value, simply move the offset to the other side of the comparison operator as a negative value, avoid the possibility of an overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When calculating sf.frames, pre-cast samplesperblock to sf_count_t, to provide the calculation with enough numeric space to avoid overflows. Other changes in this commit are syntactic, and only to satisfy the git pre-commit syntax checker. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The clang sanitizer warns of a possible signed integer overflow when calculating the `dataend` value in `mat4_read_header()`. ``` src/mat4.c:323:41: runtime error: signed integer overflow: 205 * -100663296 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:41 in src/mat4.c:323:48: runtime error: signed integer overflow: 838860800 * 4 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:48 in ``` Cast the offending `rows` and `cols` ints to `sf_count_t` (the type of `dataend` before performing the calculation, to avoid the issue. CVE: CVE-2022-33065 Fixes: libsndfile#789 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

At several points in au_read_header(), we calculate the functional end of the data segment by adding the (int)au_fmt.dataoffset and the (int)au_fmt.datasize. This can overflow the implicit int_32 return value and cause undefined behavior. Instead, precalculate the value and assign it to a 64-bit (sf_count_t)data_end variable. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast hdr.frames to sf_count_t, to provide the calculation with enough numeric space to avoid an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The sds_*byte_read() functions compose their uint_32 sample buffers by shifting 7bit samples into a 32bit wide buffer, and adding them together. Because the 7bit samples are stored in 32bit ints, code fuzzers become concerned that the addition operation can overflow and cause undefined behavior. Instead, bitwise-OR the bytes together - which should accomplish the same arithmetic operation, without risking an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com> Do the same for the 3byte and 4byte read functions.

aiff_read_basc_chunk() tries to count the AIFF header size by keeping track of the bytes returned by psf_binheader_readf(). Though improbable, it is technically possible for these added bytes to exceed the int-sized `count` accumulator. Use a 64-bit sf_count_t type for `count`, to ensure that it always has enough numeric space. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When reading the IRCAM header, it is possible for the calculated blockwidth to exceed the bounds of a signed int32. Use a 64bit sf_count_t to store the blockwidth. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast the components of the blockwidth calculation to sf_count_t to avoid overflowing integers during calculation. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The psf_binheader_readf() function attempts to count and return the number of bytes traversed in the header. During this accumulation, it is possible to overflow the int-sized byte_count variable. Avoid this overflow by checking that the accumulated bytes do not exceed INT_MAX and throwing an error if they do. This implies that files with multi-gigabyte headers threaten to produce this error, but I imagine those files don't really exist - and this error is better than the undefined behavior which would have resulted previously. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When calculating sf.frames from the blocks_total PNMS variable, it is theoretically possible to overflow the blocks_total int boundaries, leading to undefined behavior. Cast blocks_total to a long-sized sf_count_t before the calculation, to provide it with enough numeric space and because that is the final typing regardless. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Cast the int-sized bytewidth variable to a long-sized sf_count_t type prior to calculating the blockwidth, to provide the calculation with enough numeric space and sf_count_t is the final typing regardless. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When checking for mismatches between the filelength and riff_size, it is possible to overflow the temporary riff_size value used in the comparison by adding a static offset; which is probably fine, but it is offensive to overflow fuzzers. Since filelength is always a positive value, simply move the offset to the other side of the comparison operator as a negative value, avoid the possibility of an overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When calculating sf.frames from the blocks_total PNMS variable, it is theoretically possible to overflow the blocks_total int boundaries, leading to undefined behavior. Cast blocks_total to a long-sized sf_count_t before the calculation, to provide it with enough numeric space and because that is the final typing regardless. CVE: CVE-2022-33065 Fixes: #833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Cast the int-sized bytewidth variable to a long-sized sf_count_t type prior to calculating the blockwidth, to provide the calculation with enough numeric space and sf_count_t is the final typing regardless. CVE: CVE-2022-33065 Fixes: #833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When checking for mismatches between the filelength and riff_size, it is possible to overflow the temporary riff_size value used in the comparison by adding a static offset; which is probably fine, but it is offensive to overflow fuzzers. Since filelength is always a positive value, simply move the offset to the other side of the comparison operator as a negative value, avoid the possibility of an overflow. CVE: CVE-2022-33065 Fixes: #833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When calculating sf.frames, pre-cast samplesperblock to sf_count_t, to provide the calculation with enough numeric space to avoid overflows. Other changes in this commit are syntactic, and only to satisfy the git pre-commit syntax checker. CVE: CVE-2022-33065 Fixes: #833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The clang sanitizer warns of a possible signed integer overflow when calculating the `dataend` value in `mat4_read_header()`. ``` src/mat4.c:323:41: runtime error: signed integer overflow: 205 * -100663296 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:41 in src/mat4.c:323:48: runtime error: signed integer overflow: 838860800 * 4 cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/mat4.c:323:48 in ``` Cast the offending `rows` and `cols` ints to `sf_count_t` (the type of `dataend` before performing the calculation, to avoid the issue. CVE: CVE-2022-33065 Fixes: libsndfile#789 Fixes: libsndfile#833 Upstream-Status: Backport [9a82911] Signed-off-by: Alex Stewart <alex.stewart@ni.com>

At several points in au_read_header(), we calculate the functional end of the data segment by adding the (int)au_fmt.dataoffset and the (int)au_fmt.datasize. This can overflow the implicit int_32 return value and cause undefined behavior. Instead, precalculate the value and assign it to a 64-bit (sf_count_t)data_end variable. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast hdr.frames to sf_count_t, to provide the calculation with enough numeric space to avoid an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The sds_*byte_read() functions compose their uint_32 sample buffers by shifting 7bit samples into a 32bit wide buffer, and adding them together. Because the 7bit samples are stored in 32bit ints, code fuzzers become concerned that the addition operation can overflow and cause undefined behavior. Instead, bitwise-OR the bytes together - which should accomplish the same arithmetic operation, without risking an int-overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com> Do the same for the 3byte and 4byte read functions.

aiff_read_basc_chunk() tries to count the AIFF header size by keeping track of the bytes returned by psf_binheader_readf(). Though improbable, it is technically possible for these added bytes to exceed the int-sized `count` accumulator. Use a 64-bit sf_count_t type for `count`, to ensure that it always has enough numeric space. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

When reading the IRCAM header, it is possible for the calculated blockwidth to exceed the bounds of a signed int32. Use a 64bit sf_count_t to store the blockwidth. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

Pre-cast the components of the blockwidth calculation to sf_count_t to avoid overflowing integers during calculation. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

The psf_binheader_readf() function attempts to count and return the number of bytes traversed in the header. During this accumulation, it is possible to overflow the int-sized byte_count variable. Avoid this overflow by checking that the accumulated bytes do not exceed INT_MAX and throwing an error if they do. This implies that files with multi-gigabyte headers threaten to produce this error, but I imagine those files don't really exist - and this error is better than the undefined behavior which would have resulted previously. CVE: CVE-2022-33065 Fixes: libsndfile#833 Signed-off-by: Alex Stewart <alex.stewart@ni.com>

It is possible (though functionally incorrect) for the signal estimate calculation in nms_adpcm_update() to overflow the int value of s_e, resulting in undefined behavior. Since adpcm state signal values are never practically larger than 16 bits, use smaller numeric sizes throughout the file to avoid the overflow. CVE: CVE-2022-33065 Fixes: libsndfile#833 Authored-by: Arthur Taylor <art@ified.ca> Signed-off-by: Alex Stewart <alex.stewart@ni.com> Rebased-by: Alex Stewart <alex.stewart@ni.com>