963843 - OpenH264: "runtime error: signed integer overflow" to pTCoeff

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase.264

In order to reproduce you need to compile OpenH264 with UBSan support.

export CC="$LLVM_HOME/build/bin/clang -fsanitize=undefined -fno-sanitize=vptr"
export CXX="$LLVM_HOME/build/bin/clang++ -fsanitize=undefined -fno-sanitize=vptr"
export LD="$LLVM_HOME/build/bin/clang++"
export LDFLAGS="-fsanitize=undefined"
make


codec/decoder/./core/src/parse_mb_syn_cavlc.cpp:784:32: runtime error: signed integer overflow: 134216161 * 26 cannot be represented in type 'int'

pTCoeff[j] = iLevel[i] * kpDequantCoeff[j & 0x07];


I will hide this bug first because I am not sure how you would like me to treat this class of bugs.


Tested with https://github.com/cisco/openh264/commit/58c33b8ee8

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

So this relies on rounding behavior that in theory a compiler could assume won't happen, but I think in that case the only impact would be incorrect decoding.  It's possible this is intentional rounding, or maybe due to feeding it non-sensical input data.

I definitely don't like the mixing of bitwise and multiply operators with no parens, however!  Recipe for mistakes....

I don't believe there's any security risk here.

Comment 2

[wayne]

Hi Randell and Christoph, this is due to illegal input data resulting in data overflow for 32-bit.
For now, I think it is not a security issue, but I'll look into this to give a further illustration.

Comment 3

[wayne]

We've discussed that, as Randell said, it will only affect the decoder result. For decoder, the iLevel[i] is a valid input from the scope of entropy residual decoding. It is not quite efficient for decoder to have such errors for now. 
If you think it is necessary, we may discuss more on it.

BTW: As it is not crash bug, we may delay it after some time (Chinese Spring Festival is coming:) )

Comment 4

[wayne]

Hi Christoph, the bug has been solved by early fix from others.
Could you please check it with latest cisco master branch?
btw: the codec has other similar issues not reflected by this bitstream, and we'll deal with this in future.

Comment 5

[wayne]

Sorry, above comments are for issue #963836.

For this issue, we discussed and decide not to consider it as a bug.
Thanks.

Comment 6

[wayne]

btw: the data overflow can be early-checked through other method, which is already contained in our local test. You can check later after that fix is merged.

Comment 7

[wayne]

Hi Christoph, the bug has been fixed in cisco master branch.
Could u please check it? Thanks.

Comment 8

[Christoph Diehl [:posidron]]

Fixed.

Tested with https://github.com/cisco/openh264/commit/75f8b41bcd