Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345

fdu-sec · 2022-10-10T15:09:37Z

Description

Heap-buffer-overflow (/libde265/build/libde265/liblibde265.so+0x148fda) in void put_epel_hv_fallback(short*, long, unsigned short const*, long, int, int, int, int, short*, int)

Version

$ ./dec265 -h
 dec265  v1.0.8
--------------
usage: dec265 [options] videofile.bin
The video file must be a raw bitstream, or a stream with NAL units (option -n).

options:
  -q, --quiet       do not show decoded image
  -t, --threads N   set number of worker threads (0 - no threading)
  -c, --check-hash  perform hash check
  -n, --nal         input is a stream with 4-byte length prefixed NAL units
  -f, --frames N    set number of frames to process
  -o, --output      write YUV reconstruction
  -d, --dump        dump headers
  -0, --noaccel     do not use any accelerated code (SSE)
  -v, --verbose     increase verbosity level (up to 3 times)
  -L, --no-logging  disable logging
  -B, --write-bytestream FILENAME  write raw bytestream (from NAL input)
  -m, --measure YUV compute PSNRs relative to reference YUV
  -T, --highest-TID select highest temporal sublayer to decode
      --disable-deblocking   disable deblocking filter
      --disable-sao          disable sample-adaptive offset filter
  -h, --help        show help

Replay

git clone https://github.com/strukturag/libde265.git
cd libde265
mkdir build
cd build
cmake ../ -DCMAKE_CXX_FLAGS="-fsanitize=address"
make -j$(nproc)
./dec265/dec265 poc11-1
./dec265/dec265 poc11-2

ASAN

WARNING: pps header invalid
WARNING: CTB outside of image area (concealing stream error...)
WARNING: CTB outside of image area (concealing stream error...)
=================================================================
==61372==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b00002951c at pc 0x7f3e99904fdb bp 0x7ffe34d063b0 sp 0x7ffe34d063a0
READ of size 2 at 0x62b00002951c thread T0
    #0 0x7f3e99904fda in void put_epel_hv_fallback<unsigned short>(short*, long, unsigned short const*, long, int, int, int, int, short*, int) (/libde265/build/libde265/liblibde265.so+0x148fda)
    #1 0x7f3e999332ca in acceleration_functions::put_hevc_epel_hv(short*, long, void const*, long, int, int, int, int, short*, int) const (/libde265/build/libde265/liblibde265.so+0x1772ca)
    #2 0x7f3e99935213 in void mc_chroma<unsigned short>(base_context const*, seq_parameter_set const*, int, int, int, int, short*, int, unsigned short const*, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x179213)
    #3 0x7f3e99925b2d in generate_inter_prediction_samples(base_context*, slice_segment_header const*, de265_image*, int, int, int, int, int, int, int, PBMotion const*) (/libde265/build/libde265/liblibde265.so+0x169b2d)
    #4 0x7f3e9993290f in decode_prediction_unit(base_context*, slice_segment_header const*, de265_image*, PBMotionCoding const&, int, int, int, int, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x17690f)
    #5 0x7f3e9996d7e3 in read_prediction_unit(thread_context*, int, int, int, int, int, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x1b17e3)
    #6 0x7f3e9996f39a in read_coding_unit(thread_context*, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x1b339a)
    #7 0x7f3e99970250 in read_coding_quadtree(thread_context*, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x1b4250)
    #8 0x7f3e99970091 in read_coding_quadtree(thread_context*, int, int, int, int) (/libde265/build/libde265/liblibde265.so+0x1b4091)
    #9 0x7f3e99967726 in read_coding_tree_unit(thread_context*) (/libde265/build/libde265/liblibde265.so+0x1ab726)
    #10 0x7f3e999709ea in decode_substream(thread_context*, bool, bool) (/libde265/build/libde265/liblibde265.so+0x1b49ea)
    #11 0x7f3e9997270f in read_slice_segment_data(thread_context*) (/libde265/build/libde265/liblibde265.so+0x1b670f)
    #12 0x7f3e998d16d2 in decoder_context::decode_slice_unit_sequential(image_unit*, slice_unit*) (/libde265/build/libde265/liblibde265.so+0x1156d2)
    #13 0x7f3e998d1ec1 in decoder_context::decode_slice_unit_parallel(image_unit*, slice_unit*) (/libde265/build/libde265/liblibde265.so+0x115ec1)
    #14 0x7f3e998d0c0f in decoder_context::decode_some(bool*) (/libde265/build/libde265/liblibde265.so+0x114c0f)
    #15 0x7f3e998d093d in decoder_context::read_slice_NAL(bitreader&, NAL_unit*, nal_header&) (/libde265/build/libde265/liblibde265.so+0x11493d)
    #16 0x7f3e998d343e in decoder_context::decode_NAL(NAL_unit*) (/libde265/build/libde265/liblibde265.so+0x11743e)
    #17 0x7f3e998d3ab3 in decoder_context::decode(int*) (/libde265/build/libde265/liblibde265.so+0x117ab3)
    #18 0x7f3e998bae95 in de265_decode (/libde265/build/libde265/liblibde265.so+0xfee95)
    #19 0x55a40ac18bc9 in main (/libde265/build/dec265/dec265+0x6bc9)
    #20 0x7f3e993ecc86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
    #21 0x55a40ac169b9 in _start (/libde265/build/dec265/dec265+0x49b9)

0x62b00002951c is located 12 bytes to the right of 25360-byte region [0x62b000023200,0x62b000029510)
allocated by thread T0 here:
    #0 0x7f3e99de3790 in posix_memalign (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdf790)
    #1 0x7f3e9990c1cb in ALLOC_ALIGNED(unsigned long, unsigned long) (/libde265/build/libde265/liblibde265.so+0x1501cb)
    #2 0x7f3e9990c99d in de265_image_get_buffer(void*, de265_image_spec*, de265_image*, void*) (/libde265/build/libde265/liblibde265.so+0x15099d)
    #3 0x7f3e9990ed1a in de265_image::alloc_image(int, int, de265_chroma, std::shared_ptr<seq_parameter_set const>, bool, decoder_context*, long, void*, bool) (/libde265/build/libde265/liblibde265.so+0x152d1a)
    #4 0x7f3e998f30cc in decoded_picture_buffer::new_image(std::shared_ptr<seq_parameter_set const>, decoder_context*, long, void*, bool) (/libde265/build/libde265/liblibde265.so+0x1370cc)
    #5 0x7f3e998d4824 in decoder_context::generate_unavailable_reference_picture(seq_parameter_set const*, int, bool) (/libde265/build/libde265/liblibde265.so+0x118824)
    #6 0x7f3e998d7332 in decoder_context::process_reference_picture_set(slice_segment_header*) (/libde265/build/libde265/liblibde265.so+0x11b332)
    #7 0x7f3e998dad70 in decoder_context::process_slice_segment_header(slice_segment_header*, de265_error*, long, nal_header*, void*) (/libde265/build/libde265/liblibde265.so+0x11ed70)
    #8 0x7f3e998d0246 in decoder_context::read_slice_NAL(bitreader&, NAL_unit*, nal_header&) (/libde265/build/libde265/liblibde265.so+0x114246)
    #9 0x7f3e998d343e in decoder_context::decode_NAL(NAL_unit*) (/libde265/build/libde265/liblibde265.so+0x11743e)
    #10 0x7f3e998d3ab3 in decoder_context::decode(int*) (/libde265/build/libde265/liblibde265.so+0x117ab3)
    #11 0x7f3e998bae95 in de265_decode (/libde265/build/libde265/liblibde265.so+0xfee95)
    #12 0x55a40ac18bc9 in main (/libde265/build/dec265/dec265+0x6bc9)
    #13 0x7f3e993ecc86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/libde265/build/libde265/liblibde265.so+0x148fda) in void put_epel_hv_fallback<unsigned short>(short*, long, unsigned short const*, long, int, int, int, int, short*, int)
Shadow bytes around the buggy address:
  0x0c567fffd250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c567fffd260: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c567fffd270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c567fffd280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c567fffd290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c567fffd2a0: 00 00 fa[fa]fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c567fffd2b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c567fffd2c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c567fffd2d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c567fffd2e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c567fffd2f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==61372==ABORTING

POC

https://github.com/FDU-Sec/poc/blob/main/libde265/poc11-1
https://github.com/FDU-Sec/poc/blob/main/libde265/poc11-2

Environment

Ubuntu 16.04
Clang 10.0.1
gcc 5.5

Credit

Peng Deng (Fudan University)

coldtobi · 2022-12-12T12:26:43Z

Hi

(in advance, sorry for the wall of text… I'm just trying to convey as much information as
possible and as my knowledge on h.265 is limited, I try to be as verbose
as possible so that errors in my thinking are more obviouls caught…)

I was debugging into this crash and like to share my findings.
Unfortunatly the h265 standard is too complicated for me to grasp
what the right thing would be to do…

I've using a lots of additional printfs for debugging output, the branch I'm using
is this one:
coldtobi@c4c3a3a

I've compiled it on Debian unstable with asan, cmake invocation:
cmake ../libde265 -DCMAKE_CXX_FLAGS="-fsanitize=address" -DCMAKE_BUILD_TYPE=Debug

The complete log of poc11-1 is here (using my debug code)

without -d: libdec265-poc11-1.txt
with -d: libdec265-poc11-1-withdump.txt

TL;DR: I think (not sure if I read the standard correctly here) that
the poc is sending a new SPS during a VCL, which would be not allowed as 7.4.2.4.2.

This tricks the decoder to mix different images generated with different SPS is the same decoded pictures,
leading to the seen out-of-bound memory accesses.

How to Fix?
I'm not sure how to fix that without breaking compatiblity with the standard,
-- frankly I wouldn't be able to recognize this…

Said that here I've got an fix to at least avoid the crashes, by
rejecting reference pictures if they are created by a different SPS.

One way would be just to look at the pointers of the SPS (fast and easy, but
may reject more than required),
or investigate if the SPS used for the image generations are "compatible".
[A] below could help for that.
The Standard C3.2 the paragraph starting
with "Otherwise, if the value of" seems to have a list of parameters that
needs to be matched, but I'm not sure if that list is sensible/complete…

(I'll submit as PR later, after cleaning it up, to have a better base
to discuss that.)

Some other observations:

[A] It seems that the standard allows (active) SPS, VPS and PPS to be re-transmitted,
-- if I read 7.4.2.4.2; page 69 last pargraph correctly -- during an active
VCS, but then it has to have the same content.
The idea "reject any reference images if they have not been created the image's sps"
might benefit from checking if a newly received SPS is identical to the one it
would replace.
in the sequence leading up to the asan failure, I've observed that bascially all
calls in the chain (e.g decode_substream(), read_coding_tree_unit(),
read_coding_quadtree(), read_coding_unit(), decode_quantization_parameters()) leading
to the asan-triggering generate_inter_prediction_samples()
is getting the sps and pps pointer from the image. In contrast, the function
generate_inter_prediction_samples() extracts it from the shdr.
I've seen in another poc (unfortuantly did not note which one) that this leaded
to a situation where the wrong sps was used, in the meaning that the sps's parameters
did not match the parameters of the image. Changing that fixed several crashes,
I'll try to follow up on that seperatly.

** verbose debug log**

I believe the poc11-1 does this evil:

(- Generating VPS #1) (in log: in memory at 0x617000000090)

Generating SPS #1 (with seq_parameter_set_id=0 and bit_depth_chroma=8) (in memory at 0x629000000210)
(- Generating PPS #1 ) pic_parameter_set_id=0 -- in memory at 0x625000002910)
slice #1 (SHDR at 0x61b000000780)
- generates output image ID=0 with VPS#1 PPS#1 and SPS#1
slice #2 (SHDR at 0x61b000000e80)
- generates output image ID=1 with VPS#1 PPS#1 and SPS#1
- generates (missing reference) image ID=2 SPS#1 for POC=-2

(- output of image #0)

Generating SPS #2 (with seq_parameter_set_id=0 and bit_depth_chroma=9) -- memory location 0x629000005210
This new SPS #2 replaces SPS #1 - (PPS#1 is removed from decoder_context::pps[])
(- Generating PPS #2 with pic_parameter_set_id=0 , memory location #0x62500000c910)
slice #3 (SHDR at 0x61b000001580)
- generating image ID=3 with VPS#1 *SPS#2* PPS#2)
- it seems that this image is referencing the image with ID=2.
  noting that this image has been created with SPS#1 so it has only a 8 bits chroma.
- "remove ID 0" -- DPB[0] set to unused.

At this point the DPB contains:

 DPB  0: POC=0   , ID=  0 img=0x616000000080 short-term ------	(vps=0x617000000090 sps=0x629000000210 pps=0x625000002910)
 DPB  1: POC=-1  , ID=  1 img=0x616000000380 short-term output	(vps=0x617000000090 sps=0x629000000210 pps=0x625000002910)
 DPB  2: POC=-2  , ID=  2 img=0x616000000680 short-term ------	(vps=(nil) sps=0x629000000210 pps=(nil))
 DPB  3: POC=-3  , ID=  3 img=0x616000000980 short-term ------	(vps=0x617000000090 sps=0x629000005210 pps=0x62500000c910)

(- output of image #1)

slice #4
- generating image ID=4 with VPS#1 *SPS#2* PPS#2 (taking the place in dpb at dpb[0])
- generating (missing reference) image ID=5 with VPS#1 *SPS#2* PPS#2 for POC=-8
- generating (missing reference) image ID=6 with VPS#1 *SPS#2* PPS#2 for POC=-17
- generating (missing reference) image ID=7 with VPS#1 *SPS#2* PPS#2 for POC=-23
- generating (missing reference) image ID=8 with VPS#1 *SPS#2* PPS#2 for POC=0
- "remove ID 1" -- DPB[1] set to unused.

Now do_some_work() is invoked, and image_units[0] is being handled:

image_units[0] contains (taken from gdb, not in the logs):
- image -> 0x616000000980 (image ID=3)
- imgunit->get_next_unprocessed_slice_segment() returns slice #3 (0x61b000001580)

Taking a look at the SHDR, I can reconfirm that the reference set contains:

  shdr->RefPicList[2][MAX_NUM_REF_PICS] contains [2 , 2 , ( remaining 0s) ], [2 , 2 , ( remaining 0s) ]
  shdr->RefPicList_POC[2][MAX_NUM_REF_PICS] contains [-2 , -2 , ( remaining 0s) ], [-2 , -2 , ( remaining 0s) ]
  shdr->RefPicList_PicState[2][MAX_NUM_REF_PICS] contains [1 , 1 , ( remaining 0s) ], [1 , 1 , ( remaining 0s)
  shrs->LongTermRefPic[2][MAX_NUM_REF_PICS] is all zero.

- decoder_context::decode_slice_unit_parallel() is now started, the DPB is at this point of time (after
the call to remove_images_from_dpb(sliceunit->shdr->RemoveReferencesList) in decoder_context::decode_slice_unit_parallel())

  ####DPB start of decoder_context::decode_slice_unit_parallel()
 DPB  0: POC=-1  , ID=  4 img=0x616000000080 short-term output	(vps=0x617000000090 sps=0x629000005210 pps=0x62500000c910)
 DPB  1: POC=-1  , ID=  1 img=0x616000000380 unused     ------	(vps=0x617000000090 sps=0x629000000210 pps=0x625000002910)
 DPB  2: POC=-2  , ID=  2 img=0x616000000680 short-term ------	(vps=(nil) sps=0x629000000210 pps=(nil))
 DPB  3: POC=-3  , ID=  3 img=0x616000000980 short-term ------	(vps=0x617000000090 sps=0x629000005210 pps=0x62500000c910)
 DPB  4: POC=-8  , ID=  5 img=0x616000000c80 short-term ------	(vps=(nil) sps=0x629000005210 pps=(nil))
 DPB  5: POC=-17 , ID=  6 img=0x616000000f80 short-term ------	(vps=(nil) sps=0x629000005210 pps=(nil))
 DPB  6: POC=-23 , ID=  7 img=0x616000001280 short-term ------	(vps=(nil) sps=0x629000005210 pps=(nil))
 DPB  7: POC=0   , ID=  8 img=0x616000001580 short-term ------	(vps=(nil) sps=0x629000005210 pps=(nil))

eventually, decoding will call generate_inter_prediction_samples(), and then hit my debugigng trap emitting
this line:

#######################  TROUBLE AHEAD: REF PICTURE DIFFERS IS BITDEPTH ####################### 
 shdr=0x61b000001580 , img=0x616000000980 (ID=3), refPic=0x616000000680 (ID=2) refIdx: 1 -> dpb[2] refPic->BitDepth_C=8, img->BitDepth_C=9 refPic->BitDepth_Y=8, img->BitDepth_Y=8

this confirms that image ID=3, which has been created with SPS#2 is using as reference image ID=2, which
has been created with SPS#1 -- with different storage units in img->pixels[{1,2}];
later in generate_inter_prediction_samples() the bit depth of the reference picture is used to
determine the template parameter pixel_t, IOW which flavour or mc_chroma<pixel_t>() or mc_luma<pixel_t>() is to be called.
as this function is doing pointer arithemtics, if RefPic's pixel is uint16_t and Img's pixel_t is uint8_t, it will
eventually explode:
(This is for the asan error that is triggered for poc11-1: )
- function: put_epel_hv_fallback<uint16_t>(), , line: 353: case 5: v = (-4*p[0]+28*p[1]+46*p[2]-6*p[3])>>shift1; break;
- p=0x62b00002951c
  - calculated from&src[y*src_stride - extra_left];
    with y=8, src_stride=176, extra-left=1 so it is &src[1408] (or 2814 bytes into the src)
  - caller has set src to 0x62b000028a1e, (so p=0x62b00002951c)
  - src is calculated in mc_chroma(): its template/pixel_t type is uint16_t.
    src_ptr = &ref[xIntOffsC + yIntOffsC*ref_stride];
    ref=refPic->get_image_plane(1) -> 0x62b000023200 (rePic->pixel[1] = 0x62b000023200; 25360 bytes in size)
    xIntOffsC=191, yIntOffsC=63, refStride=176 --> so src_ptr becomes
    - src_ptr = &ref[11279] (or as <pixel_t>=uint16_t 22556 bytes into rePic->pixel[1]
    - src_ptr = 0x62b000028a1e
  - the code tries to access 0x62b00002951c + (sizeof(uint16_t)*3) = 0x62b00002951c + 6 = 0x62b00002951c + 6 = 0x62b000029522
  - however, pixel[1] ends at 0x62b000023200 + 25360 = 0x62b000029510, so 0x62b00002951c is outside of the allocated area (as asan correctly reports:

           ==731779==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b00002951c at pc 0x7ffff75622db bp 0x7ffffffdca20 sp 0x7ffffffdca18
            0x62b00002951c is located 12 bytes to the right of 25360-byte region [0x62b000023200,0x62b000029510)

See strukturag#345 for my analysis and details… (This PR is just for discussion.) (The CVE references are obtained from the Debian security tracker, which links the issues.) This makes the following POCs stop failing: - poc3 (strukturag#337) - poc7-1 (strukturag#341) CVE-2022-43239 (note: does NOT fix poc7-2) - poc8-2, poc8-3, poc8-4 (strukturag#342) CVE-2022-43244 (note: does NOT fix poc8-1) - poc11-1, poc11-2 (strukturag#345) CVE-2022-43249 - poc12 (strukturag#346) - poc13 (strukturag#347) CVE-2022-43252 - poc16 (strukturag#350)

coldtobi · 2022-12-12T13:23:49Z

libdec265-poc11-1-withdump.txt
PS: Those issues might be related; at least when rejecting "inappropiate" reference picutures (as in PR #365 ), the following POC stop failing:

poc3 (Heap-buffer-overflow in sse-motion.cc: ff_hevc_put_hevc_epel_pixels_8_sse #337)
poc7-1 (Heap-buffer-overflow in motion.cc: mc_chroma<unsigned short> #341) CVE-2022-43239 (note: does NOT fix poc7-2)
poc8-2, poc8-3, poc8-4 (Heap-buffer-overflow in fallback-motion.cc: in void put_qpel_fallback<unsigned short> #342) CVE-2022-43244 (note: does NOT fix poc8-1)
poc11-1, poc11-2 (Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345) CVE-2022-43249
poc12 (Heap-buffer-overflow in fallback-motion.cc: in put_qpel_0_0_fallback_16 #346)
poc13 (Heap-buffer-overflow in fallback-motion.cc in put_epel_16_fallback #347) CVE-2022-43252
poc16 (Stack-buffer-overflow in sao.cc:248: void apply_sao_internal<unsigned short> #350)

coldtobi · 2022-12-12T13:57:38Z

According to Debian this is CVE-2022-43249

(as e.g mc_chroma is using the sps to determine picture properties, like pic_width_in_luma_samples and pic_height_in_luma_samples, I *think* this is more correct. This PR is for discussion. (See strukturag#345.) It makes the failures go away, but that does not mean it's correct :) The following poc will be stop failing if (only) this patch is applied: - poc2 strukturag#336 - CVE-2022-43238 - poc4 strukturag#338 - CVE-2022-43241 - poc6-1, poc6-2 strukturag#340 - CVE-2022-43242 - poc7-1, poc7-2 strukturag#341 - CVE-2022-43239 - poc8-1 strukturag#342 - CVE-2022-43244 - poc9-3 strukturag#343 - CVE-2022-43236 - poc10-2, poc10-3 strukturag#344 - CVE-2022-43237 - poc16 strukturag#350 - poc19 strukturag#353 The following are still failing if only this patch is applied, but they stop failing if strukturag#365 is applied as well, but will still fail with ONLY strukturag#365 applied (IOW, both are needed) - poc1 strukturag#335 - CVE-2022-43240 - poc3 strukturag#337 - CVE-2022-43235 - poc5 strukturag#339 - CVE-2022-43423 - poc9-1,poc9-2, poc9-4 strukturag#343 - CVE-2022-43236 - poc14 strukturag#348 - CVE-2022-43253 - poc15 strukturag#349 - CVE-2022-43248 - poc17-1, poc17-2 strukturag#351 - poc18 strukturag#352 - CVE-2022-43245

@farindk

This is an attempt to improve the mitigations from strukturag#365 and strukturag#366 and picks up an idea I described at strukturag#345: > One way would be just to look at the pointers of the SPS (fast and easy, but > may reject more than required), or investigate if the SPS used for the image > generations are "compatible". This changes do exactly this: It (very conservativly) checks if the old and new sps have identical information -- except the reference picture set, which I believe is supposed to be updated by new sps'). If they are basically identical, the old sps will be used instead of the new one, (of course, reference image set is updated from the new one) I'm using standalone operator== and helper functions to avoid changing ABI of the library; if an ABI bump would be done, of course this should go to the respective classes. I've tested the patch with several videos, they still play fine. @farindk I'd really appreciate to receive your feedback; the reason is that I want to fix the many open CVE's for the package as it is currently in Debian and other distributions… -- Cheers, tobi

This is an attempt to improve the mitigations from strukturag#365 and strukturag#366 and picks up an idea I described at strukturag#345: > One way would be just to look at the pointers of the SPS (fast and easy, but > may reject more than required), or investigate if the SPS used for the image > generations are "compatible". This changes do exactly this: It (very conservativly) checks if the old and new sps have identical information -- except the reference picture set, which I believe is supposed to be updated by new sps'). If they are basically identical, the old sps will be used instead of the new one, (of course, reference image set is updated from the new one) I'm using standalone operator== and helper functions to avoid changing ABI of the library; if an ABI bump would be done, of course this should go to the respective classes.

farindk · 2023-01-24T11:25:09Z

Thanks for the extensive analysis and patch.
Since the actual cause is that reference image and current image have different bit depths, what about simply checking if they match (fbd0b3a)?

While your patch tries to prevent the situation by detecting it high-level, it is complicated to compare SPSs for "compatibility" and whether they are simply repeated (51f07f1). I think, this is very error prone. Checking for matching bit-depth directly, on the other hand, secures the function itself, which may also be helpful if used in another context. And it also allows us to output a warning what actually is wrong with the input stream.

There might be a performance penalty in carrying out this check for each PB, but here I simply hope for a good branch prediction.

coldtobi · 2023-01-24T12:08:21Z

what about simply checking if they match (fbd0b3a)?

This was my first approach, and while it fixes that specific exploit bitdepth is not the only paramter to be checked; for example I saw also crashes out of the poc if the dimensions where not matching the expectaions.
I'm pretty sure that there are many more, and I think my approach here is more robust than to individually check every different scenario… That would feel like a road to play whack-a-mole for me…

Comparing the sps is of course a "brute force" attempt to fix that;
For example (if I understood it correclty), sps updates are only allowed in certain cirmumstances, I read something in the standards.

The patch to check for compatiblity was only for to avoid over-rejecting. My reference videos played well without it, but I thought "better save here". This patch is also erroring on the "it's incompatible if in doubt" side.

farindk · 2023-01-24T15:07:22Z

Should be fixed with #373

farindk · 2023-01-24T15:14:07Z

@coldtobi The ideal check would be on several levels. I think it is important that each function checks that its input parameters are valid. Furthermore, there can be another high-level check whether the SPS packets in the input stream are valid. I see this mainly as a better error reporting instead of a wall of low-level errors.
However, the valid SPS checks are difficult as you can see in the size of your patch, and I won't be able to know the rules without inspecting the standard again. I'm sure the SPS may change considerably, e.g. when a TV program changes its resolution (think about an 1280x720 to 1920x1080 change). That will be the same whack-a-mole game. Just that you have the danger of rejecting streams that are valid or that only have a minor fault at the seam of the concatenated streams.

Hence, I'll go with the low-level fix for the moment, but leave your pull-requests open so that we can look at this again to improve error reporting.

coldtobi mentioned this issue Dec 12, 2022

Try to mitigate asan failures. #365

Open

coldtobi mentioned this issue Dec 12, 2022

Use the sps from the image #366

Open

coldtobi mentioned this issue Jan 13, 2023

Don't update sps if they are only repeated #372

Open

farindk added a commit that referenced this issue Jan 24, 2023

fix-345 by checking in MC whether bit-depths match (#345)

fbd0b3a

farindk added the fix available label Jan 24, 2023

farindk closed this as completed Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345

Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345

fdu-sec commented Oct 10, 2022

coldtobi commented Dec 12, 2022 •

edited

Loading

coldtobi commented Dec 12, 2022 •

edited

Loading

coldtobi commented Dec 12, 2022

farindk commented Jan 24, 2023 •

edited

Loading

coldtobi commented Jan 24, 2023

farindk commented Jan 24, 2023

farindk commented Jan 24, 2023 •

edited

Loading

Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345

Heap-buffer-overflow in fallback-motion.cc: void put_epel_hv_fallback<unsigned short>( #345

Comments

fdu-sec commented Oct 10, 2022

Description

Version

Replay

ASAN

POC

Environment

Credit

coldtobi commented Dec 12, 2022 • edited Loading

coldtobi commented Dec 12, 2022 • edited Loading

coldtobi commented Dec 12, 2022

farindk commented Jan 24, 2023 • edited Loading

coldtobi commented Jan 24, 2023

farindk commented Jan 24, 2023

farindk commented Jan 24, 2023 • edited Loading

coldtobi commented Dec 12, 2022 •

edited

Loading

coldtobi commented Dec 12, 2022 •

edited

Loading

farindk commented Jan 24, 2023 •

edited

Loading

farindk commented Jan 24, 2023 •

edited

Loading