undefined shifts

[michaelni]

FFmpeg / Google ossfuzz did run into 2 bad shifts while encoding with libtheora through FFmpeg.

The first likely should be *8 instead of <<3, theres are 13 more matches to stride<< i dont know if these can be negative too

The 2nd case shifts into the sign bit which is undefined. It likely was meant to be unsigned

The stack traces from UndefinedBehaviorSanitizer are below

	state.c:654:19: runtime error: left shift of negative value -96
    #0 0x87b6f0 in oc_state_ref_bufs_init theora/lib/state.c:654:19
    #1 0x879e0c in oc_state_init theora/lib/state.c:736:17
    #2 0x85dc43 in oc_enc_init theora/lib/encode.c:1159:7
    #3 0x85dc43 in th_encode_alloc theora/lib/encode.c:1346:17
    #4 0x409499 in encode_init /src/ffmpeg/libavcodec/libtheoraenc.c:231:18
    #5 0x4d27ab in avcodec_open2 /src/ffmpeg/libavcodec/avcodec.c:326:19
    #6 0x4d11b5 in LLVMFuzzerTestOneInput /src/ffmpeg/tools/target_enc_fuzzer.c:153:15
    #7 0x436ab3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #8 0x422212 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #9 0x427abc in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
    #10 0x450ff2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #11 0x79fee856e082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
    #12 0x4183dd in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior state.c:654:19 in

	state.c:399:37: runtime error: left shift of 1 by 63 places cannot be represented in type 'ogg_int64_t' (aka 'long')
    #0 0x87ebd8 in oc_state_border_init theora/lib/state.c:399:37
    #1 0x87a9c4 in oc_state_frarray_init theora/lib/state.c:524:3
    #2 0x879de9 in oc_state_init theora/lib/state.c:735:7
    #3 0x85dc43 in oc_enc_init theora/lib/encode.c:1159:7
    #4 0x85dc43 in th_encode_alloc theora/lib/encode.c:1346:17
    #5 0x409499 in encode_init /src/ffmpeg/libavcodec/libtheoraenc.c:231:18
    #6 0x4d27ab in avcodec_open2 /src/ffmpeg/libavcodec/avcodec.c:326:19
    #7 0x4d11b5 in LLVMFuzzerTestOneInput /src/ffmpeg/tools/target_enc_fuzzer.c:153:15
    #8 0x436ab3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #9 0x422212 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #10 0x427abc in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
    #11 0x450ff2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #12 0x7b6e9cb8d082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
    #13 0x4183dd in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior state.c:399:37 in

[apoleon]

It looks like CVE-2024-56431 has been assigned for those issues or a similar problem.

[petterreinholdtsen]

I suspect #19 is a fix for this issue.

[petterreinholdtsen]

After looking a bit closer, I believe #19 fixes CVE-2024-56431, and this issue is about a different (and possibly related) issue.

[petterreinholdtsen]

I had a closer look, and agree that switching from bit shifting signed values to multiplication seem like a good solution for the former class of problems.

The latter on the other hand seem to be best solved by switch to an unsigned 64 bit type, ie like this:

diff --git a/lib/state.c b/lib/state.c
index 12be4f6..8be7517 100644
--- a/lib/state.c
+++ b/lib/state.c
@@ -388,7 +388,7 @@ static void oc_state_border_init(oc_theora_state *_state){
         /*Otherwise, check to see if it straddles the border.*/
         else if(x<crop_x0&&crop_x0<x+8||x<crop_xf&&crop_xf<x+8||
          y<crop_y0&&crop_y0<y+8||y<crop_yf&&crop_yf<y+8){
-          ogg_int64_t mask;
+          ogg_uint64_t mask;
           int         npixels;
           int         i;
           mask=npixels=0;
@@ -396,7 +396,7 @@ static void oc_state_border_init(oc_theora_state *_state){
             int j;
             for(j=0;j<8;j++){
               if(x+j>=crop_x0&&x+j<crop_xf&&y+i>=crop_y0&&y+i<crop_yf){
-                mask|=(ogg_int64_t)1<<(i<<3|j);
+                mask|=(ogg_uint64_t)1<<(i<<3|j);
                 npixels++;
               }
             }

[petterreinholdtsen]

I believe all undefined bit shifts are fixed in the master branch on https://github.com/xiph/theora . Please report remaining issues there if you find any. I plan to wrap up a new release using master the coming weekend.