Out-of-bounds read in opj_j2k_update_image_data and opj_tgt_reset function

[boxerab]

http://seclists.org/oss-sec/2016/q1/128

[szukw000]

It seems that both images are broken. But the openjpeg library should
realize it.

winfried
github-#704.txt

[mayeut]

The crash for the second image is caused by an arithmetic overflow in pi.c
Line 374:

prci = opj_int_floordivpow2(opj_int_ceildiv(pi->x, (OPJ_INT32)(comp->dx << levelno)), (OPJ_INT32)res->pdx) - opj_int_floordivpow2(trx0, (OPJ_INT32)res->pdx);

When levelno >= 31 this overflows.
Replacing with

prci = opj_int_floordivpow2(opj_int_ceildiv64(pi->x, (OPJ_INT64)comp->dx << levelno), (OPJ_INT32)res->pdx) - opj_int_floordivpow2(trx0, (OPJ_INT32)res->pdx);

solves the issue. (opj_int_ceildiv64 does not exist in master)

There are/were multiple issues in openjpeg for levelno >= 31 (c.f. #215, #486, #394, #480, #388).
Using OPJ_UINT32 would only shift the problem to levelno>=32 (I guess levelno==32 in fact) and only for dx==1 in this specific issue.
A complete sweep of the code shall be done to track those shift by levelno & ensure that no arithmetic overflow can occur. Unfortunately, I don't have time to do this right now.

[boxerab]

Why not just modify the floor and ceiling methods: eg

static INLINE OPJ_INT32 opj_int_floordivpow2(OPJ_INT32 a, OPJ_INT64 b) {
    return (OPJ_INT32)(a >> b);
}

and just change all of the casts in the code from (OPJ_INT32) to (OPJ_INT64) for the power of 2 ?

[detonin]

@boxerab your proposed change is on opj_int_floordivpow2 but the issue seems to be on opj_int_ceildiv.
@mayeut instead of defining opj_int_ceildiv64, is there any objection to change current opj_int_ceildiv to opj_int_ceildiv(OPJ_INT32 a, OPJ_INT64 b) and change all occurence of opj_int_ceildiv accordingly ?

[boxerab]

Sorry, no time for this at the moment.

[mayeut]

@detonin, changing all occurrences of opj_int_ceildiv as you suggest can be done but this will lead to performance loss on 32-bit platforms for sure.

[rouault]

I can't reproduce issues on the 2 images with a -fsanitize=undefined,address build. Closing

[jubalh]

For the record, this was assigned CVE-2016-1923

[jubalh]

Anybody knows what exactly the fix for this was?