CVE-2018-5785: fix issues with zero bitmasks #1148
Conversation
In the case where a BMP file declares compression 3 (BI_BITFIELDS) with header size <= 56, all bitmask values keep their initialization value 0. This may lead to various undefined behavior later e.g. when doing 1 << (l_comp->prec - 1). This issue does not affect files with bit count 16 because of a check added in 16240e2 which sets default values to the color masks if they are all 0. This commit adds similar checks for the 32 bit case. Also, if a BMP file declares compression 3 with header size >= 56 and intentional 0 bitmasks, the same issue will be triggered in both the 16 and 32 bit count case. This commit adds checks to bmp_read_info_header() rejecting BMP files with "intentional" 0 bitmasks. These checks might be removed in the future when proper handling of zero bitmasks will be available in openjpeg2. fixes uclouvain#1057 (CVE-2018-5785)
| @@ -831,6 +846,12 @@ opj_image_t* bmptoimage(const char *filename, opj_cparameters_t *parameters) | |||
| bmpmask32toimage(pData, stride, image, 0x00FF0000U, 0x0000FF00U, 0x000000FFU, | |||
| 0x00000000U); | |||
| } else if (Info_h.biBitCount == 32 && Info_h.biCompression == 3) { /* bitmask */ | |||
| if ((Info_h.biRedMask == 0U) && (Info_h.biGreenMask == 0U) && | |||
There was a problem hiding this comment.
what happens if one or two of the component mask is 0 and not the 3 ? Wouldn't there be an issue too ?
There was a problem hiding this comment.
Well, I see two different issues:
- First issue: file declares BI_BITFIELDS and header size <= 56.
In this case I think we should just ignore the BI_BITFIELDS and proceed using default bitmask values.
My patch fixes this issue at line 848.
- Second issue: file declares BI_BITFIELDS and header size >= 56 and one or more zero bitmasks.
This is the issue you speaking about.
Actually, we should be able to handle this case gracefully. After all, it's only one or two missing color channels right ? But this is not working right now, and there has to be a temporary solution. My opinion is: if we can't handle this case correctly, then we should declare it unsupported and forbid it.
This is what my patch does at line 438, 448 and 458.
There was a problem hiding this comment.
OK got it. Actually given your checks at line 438, 448, 458, when we are at line 849 either the 3 values are at 0, or they are all non-zero.
A few comments on the checks added in bmp_read_info_header():
This PR addresses #1057 (CVE-2018-5785).