Draft: common: fix sycc420_to_rgb buffer overflow

[msabwat]

This adds a check so that we do not increment Y/Cb/Cr components if we reach the last pixel.

Fixes #1347

I am wondering if I should prevent the loop from reaching this point instead of adding a check?

Also it needs more testing, it does not overflow anymore, but I am not sure I did not break anything else.

[StayPirate]

Please consider https://github.com/uclouvain/openjpeg/issues/1347#issuecomment-858573954 before merging. Thanks.

[msabwat]

I did some tests, and they do not seem to add failed tests after the patch with or without asan

@StayPirate done

[ValZapod]

There is also a problem that sYCC decoding is not bit perfect between ffmpeg's libopenjpeg wrapper and ffmpeg native decoder jpeg2000.

https://drive.google.com/file/d/1A4k7p2nYMom-ytksrnPbs-JUfsB74keD/view?usp=sharing

You can try to use ffmpeg -vf extractplanes=y [, u, v]

[rouault]

hum, so this breaks the NR-DEC-issue411-ycc420.jp2-115-decode-md5 test case of the test suite. Either my suggestion is wrong, either this test case needs refreshing

[msabwat]

hum, so this breaks the NR-DEC-issue411-ycc420.jp2-115-decode-md5 test case of the test suite. Either my suggestion is wrong, either this test case needs refreshing

How can this work if we only increment one component of the pixel? are there cases where sycc420_to_rgb will just convert luma without bllue and red (it seems odd to me, but I may be wrong)

[ValZapod]

will just convert luma without bllue and red (it

There is no blue and red. Also all grayscale is ONLY Y component.

[rouault]

How can this work if we only increment one component of the pixel? are there cases where sycc420_to_rgb will just convert luma without bllue and red (it seems odd to me, but I may be wrong)

you only increment blue and red one out of 4 pixels due to the 4:2:0 subsampling.

[ValZapod]

either this test case needs refreshing

You need to check whether those pixels that are wrong are out-of-gamut for BT.601 full range matrix (and that is actually a lot of triplets, obviously 255, y, x, where x and y are not 128 are one of those examples, but actually even in the middle a lot values are like that). Cause there is no 1 algorithm of how to represent those values...

[rouault]

upon reflection, my suggestion to remove ++cb and ++cr completely seems wrong to me. The current code of master should be fine. I'm not sure why the overflow comes from, but the remedy should likely be something else

[msabwat]

upon reflection, my suggestion to remove ++cb and ++cr completely seems wrong to me. The current code of master should be fine. I'm not sure why the overflow comes from, but the remedy should likely be something else

Sorry I didn't answer before, I wanted to test other things before I get back to you. Ran the test and the decoded image is :

There seems to be an additional line in the bottom ( I don't know if it's relevant), in the tests there is a comparison with the md5 hash, is it okay if I take the decoded file for the current master as a base ?

If I understand correctly, checking for the bounds is not enough, I need to investigate why the loop goes too far?

[StayPirate]

is there any update on this?

[msabwat]

I have a better understanding of subsampling, but I still need some time to understand the sample that causes the overflow and the issue in the code to fix it the right way. I'll get back asap

[ZaquL]

hum, so this breaks the NR-DEC-issue411-ycc420.jp2-115-decode-md5 test case of the test suite. Either my suggestion is wrong, either this test case needs refreshing

https://github.com/uclouvain/openjpeg-data/blob/master/input/nonregression/issue411-ycc420.jp2

This file decodes in -c:v jpeg2000 with 1 byte difference with -c:v libopenjpeg. One byte 0x57 is 0x56 in ffmpeg's native decoder.

ZAQU.zip

[opoplawski]

Any progress here?

[opoplawski]

Any progress here?

[SimonWoidig]

Would appreciate if any update about this vulnerability was made!

[msabwat]

Sure, I will have a look and let you know

On Thu, Nov 16, 2023, 12:30 PM Šimon Woidig @.***> wrote:

Would appreciate if any update about this vulnerability was made!

— Reply to this email directly, view it on GitHub https://github.com/uclouvain/openjpeg/pull/1362#issuecomment-1814264805, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXTX4BJABQPNF2LANC3ZODYEX2ORAVCNFSM46M3FWYKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBRGQZDMNBYGA2Q . You are receiving this because you authored the thread.Message ID: @.***>

[msabwat]

After comparing with ffmpeg's decoder, it seems the problem is about the EOC marker, which is missing. I am still reading openjpeg code to see if I can prevent the file from being decoded. Because if decoding ends, the image is processed with wrong data, which is causing the overflow. The initial fix which was to try to act on that is wrong.

I will test with another decoder, and will maybe have a new proposition of a fix for review by the end of this week.

[msabwat]

With Grok, it refuses to decompress the file for another reason:

[goose@blueduck build]$ ./bin/grk_decompress -i ../../openjpeg/poc.j2k -o test.jpg
[2023-11-24 17:28:42.724] [error] Non-compliant Rsiz value 0x2e00 in SIZ marker
[2023-11-24 17:28:42.724] [error] grk_decompress: failed to read the header

The file shouldn't be decompressed if:

• there is no EOC marker found
• Rsiz value is non compliant

@rouault, would it be acceptable to do this?

[rouault]

would it be acceptable to do this?

I've the feeling the changes you suggest would just hide the root issue in sycc420_to_rgb() by rejecting the reproducer early. Unless they are really related to the issue in sycc420_to_rgb(), but a deeper analysis should be done to understand the root cause of the heap-buffer-overflow

[msabwat]

@rouault All right! will do. I went by ZaQul's earlier comments saying that ybcbr's code should not be touched, but if you feel it is wrong, I will get back to it.

If the file is not valid, isn't it the role of the decoder to refuse the image if it does not comply with the spec?

[rouault]

I went by ZaQul's earlier comments saying that ybcbr's code should not be touched, but if you feel it is wrong, I will get back to it.

There's no "sacred" code. If it is buggy, it should be fixed. I have no informed opinion where the source of the error is.

If the file is not valid, isn't it the role of the decoder to refuse the image if it does not comply with the spec?

We already accept a number of non-compliances. This is on a case by case basis. Sometimes this can be just a small/pedantic non-compliance that can be recovered and helps decoding files that are available in the wild. Sometimes the file is just broken and we cannot do anything with it but rejecting it. Here my point was that rejecting the reproducer file could still let the issue unfixed for other less broken files if the issue is in the sycc420_to_rgb code. But again we should have a detailed analysis of the root cause to see where the fix should be.

[msabwat]

There's no "sacred" code.

I agree.

But again we should have a detailed analysis of the root cause to see where the fix should be.

I will focus on that, thank you for taking the time to answer with such valuable information.

[deekal2]

Is this issue being actively looked into? This vulnerability is still unpatched after two years.

The last release for this component was 2 years ago. Is this component being maintained?

Is there an alternative to OpenJPEG?

[msabwat]

Closing since I don't have enough time to actively look into it.