Integer overflows in j2K

[Eharve14]

in the j2k.c file there is no check on max value of cp->tw or cp-th when the values are set.
cp->tw = (OPJ_UINT32)opj_int_ceildiv((OPJ_INT32)(image->x1 - cp->tx0), (OPJ_INT32)cp->tdx); cp->th = (OPJ_UINT32)opj_int_ceildiv((OPJ_INT32)(image->y1 - cp->ty0), (OPJ_INT32)cp->tdy);

These values are later multiplied and the result is used in an ops_calloc function that does not have a cast before the call. The product of these values are also used to iterate a loop with a loop iterator that is also a 32 bit integer, tileno, which is used to write to the allocated memory.

cp->tcps = (opj_tcp_t*) opj_calloc(cp->tw * cp->th, sizeof(opj_tcp_t)); if (!cp->tcps) { opj_event_msg(p_manager, EVT_ERROR, "Not enough memory to allocate tile coding parameters\n"); return OPJ_FALSE; }

for (tileno = 0; tileno < cp->tw * cp->th; tileno++) {
    opj_tcp_t *tcp = &cp->tcps[tileno];`

I was going to cast the calloc call, but it will create an issue in the tileno loop. I would like to add a check for the result of the multiplication or some cap on the maximum value of tw and th like in the struct l_cp as shown below:
/* Compute the number of tiles */ l_cp->tw = (OPJ_UINT32)opj_int_ceildiv((OPJ_INT32)(l_image->x1 - l_cp->tx0), (OPJ_INT32)l_cp->tdx); l_cp->th = (OPJ_UINT32)opj_int_ceildiv((OPJ_INT32)(l_image->y1 - l_cp->ty0), (OPJ_INT32)l_cp->tdy);

/* Check that the number of tiles is valid */
if (l_cp->tw == 0 || l_cp->th == 0 || l_cp->tw > 65535 / l_cp->th) {
    opj_event_msg(p_manager, EVT_ERROR,
                  "Invalid number of tiles : %u x %u (maximum fixed by jpeg2000 norm is 65535 tiles)\n",
                  l_cp->tw, l_cp->th);
    return OPJ_FALSE;
}`

[rouault]

I don't understand why there would be an overflow on multiplication given that the l_cp->tw > 65535 / l_cp->th test done at line 2342 checks that the number of tiles is not greater than 65535, hence uint32 overflow can't occur.

[Eharve14]

That is true on for the l_cp struct. Th similarly named cp struct is set with out the check. If you look at lines 7952 and 7954 the value for cp->tw and cp -> th are set with out a similar check. Subsequently on line 8024 the values are multiplied to allocate memory and iterate a loop.

[rouault]

If you look at lines 7952 and 7954 the value for cp->tw and cp -> th are set with out a similar check. Subsequently on line 8024 the values are multiplied to allocate memory and iterate a loop.

got it. fixed in #1401

[Eharve14]

Awesome, closed with #1401