Avoid negative bit shift operatoin in huffdec.c (CVE-2024-56431).

A crash was discovered using input fuzzying, in th_decode_ceaderin() where the len value in the oc_fuff_tree_unpack() can end up as -1. Added a check to ensure this do not happen.

Based on feedback from Timothy B. Terriberry.

The issue was discovered using gcc sanitazion, which reported the following:

huffdec.c:228:27: runtime error: shift exponent -1 is negative
    #0 0x5d471012bfd0 in oc_huff_tree_unpack /home/uos/libtheora-18570/theora/lib/huffdec.c:228
    #1 0x5d471012c134 in oc_huff_trees_unpack /home/uos/libtheora-18570/theora/lib/huffdec.c:392
    #2 0x5d471010a98c in oc_setup_unpack /home/uos/libtheora-18570/theora/lib/decinfo.c:169
    #3 0x5d471010a98c in oc_dec_headerin /home/uos/libtheora-18570/theora/lib/decinfo.c:238
    #4 0x5d471010a98c in th_decode_headerin /home/uos/libtheora-18570/theora/lib/decinfo.c:266
    #5 0x5d47100fd638 in TheoraDecoder::initialize() /home/uos/libtheora-18570/libtheora-18570/fuzzer.cpp:66
    #6 0x5d47100ffa76 in TheoraDecoder::Run() /home/uos/libtheora-18570/libtheora-18570/fuzzer.cpp:180
    #7 0x5d47100ffe48 in main /home/uos/libtheora-18570/libtheora-18570/fuzzer.cpp:240
    #8 0x7cc9a5e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #9 0x7cc9a5e29e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #10 0x5d47100f9964 in _start (/home/uos/libtheora-18570/libtheora-18570/poc1+0x83964)

Fixes github pull request #19.

changed the description

added 1 commit

• 85571700 - Fix the runtime error: shift exponent -1 is negative in huffdec.c

Compare with previous version

Note, a proof of concept demonstrating the crash is supposed to be available from https://github.com/UnionTech-Software/libtheora-CVE-2024-56431-PoC . I lacked fuzzing/datasource/datasource.hpp and fuzzing/memory.hpp when I tried to test it, so I can not verify that it work.

I forgot to mention that this change is lifted from https://github.com/xiph/theora/pull/19 .

Using the fuzzying code from https://gitlab.xiph.org/xiph/flac/-/tree/b19f3a6114c8d6d98905c3ae8ae51eda84680918/oss-fuzz I was able build the proof of concept code and verify that it no longer report the errors initially reported for 2024-56431.

mentioned in commit d7f8887b

mentioned in merge request !31 (merged)

mentioned in commit c4724d24

mentioned in commit 9770df7d

mentioned in commit 619f9a40

mentioned in commit b51ac556

added 10 commits

• 85571700...b51ac556 - 9 commits from branch master
• 8da4f659 - Fix the runtime error: shift exponent -1 is negative in huffdec.c

Compare with previous version

Just for the record, I do not understand your proposal. Is this what you suggest instead of the patch from github?

diff --git a/lib/huffdec.c b/lib/huffdec.c
index cc1828d..2b5d82b 100644
--- a/lib/huffdec.c
+++ b/lib/huffdec.c
@@ -224,7 +224,12 @@ int oc_huff_tree_unpack(oc_pack_buf *_opb,unsigned char _tokens[256][2]){
         _tokens[ntokens][1]=(unsigned char)(len+neb);
         ntokens++;
       }
-      code_bit=0x80000000U>>len-1;
+      if(len<=0)break;
+      if (len > 0) {
+        code_bit = 0x80000000U >> (len - 1);
+      } else {
+        return TH_EBADHEADER;
+      }
       while(len>0&&(code&code_bit)){
         code^=code_bit;
         code_bit<<=1;

added 9 commits

• 8da4f659...13c10914 - 7 commits from branch master
• eecd1ad6 - Fix the runtime error: shift exponent -1 is negative in huffdec.c
• 356d36ce - Approach proposed by Timothy B. Terriberry.

Compare with previous version

resolved all threads

LGTM assuming you are going to squash these before merging.

added 1 commit

• 5665f86b - Avoid negative bit shift operatoin in huffdec.c (CVE-2024-56431).

Compare with previous version

changed title from Fix the runtime error: shift exponent -1 is negative in huffdec.c to Avoid negative bit shift operatoin in huffdec.c (CVE-2024-56431).

changed the description