Closed Bug 1544180 Opened 5 years ago Closed 5 years ago

Latent out-of-bounds write in TexSubImage2DWithoutUnpackSubimage

Categories

(Core :: Graphics, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: deian, Assigned: sotaro)

Details

(Keywords: csectype-intoverflow, sec-low, Whiteboard: [post-critsmash-triage][adv-main68+])

Attachments

(1 file)

I believe there is a potential out-of-bounds write in TexSubImage2DWithoutUnpackSubimage [1]:

  static void TexSubImage2DWithoutUnpackSubimage(
      GLContext* gl, GLenum target, GLint level, GLint xoffset, GLint yoffset,
      GLsizei width, GLsizei height, GLsizei stride, GLint pixelsize,
      GLenum format, GLenum type, const GLvoid* pixels) {
  ...
    unsigned char* newPixels =
        new (fallible) unsigned char[width * height * pixelsize]; // width * height * pixelsize overflows to allocate small buffer (smaller than width * pixelsize)

    if (newPixels) {
      unsigned char* rowDest = newPixels;
      const unsigned char* rowSource = (const unsigned char*)pixels;
      for (int h = 0; h < height; h++) {
        memcpy(rowDest, rowSource, width * pixelsize);                      // copy width * pixelsize bytes

I'm not sure if this is easy to trigger, but seemed worth reporting. Apologies
if this turns out to not be a concern.

[1] https://searchfox.org/mozilla-central/source/gfx/gl/GLUploadHelpers.cpp#147-154

Group: firefox-core-security → core-security
Component: General → Graphics
Product: Firefox → Core
Group: core-security → gfx-core-security

width and height are never more than 16384. At 16Ki*16Ki, 4bpp is 1GiB, but if we allow 8bpp (or higher) here, that's 2GiB, which would overflow on 32-bit.

Priority: -- → P2

Sotaro, could you help with this one? Thank you!

Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)

TexSubImage2DWithoutUnpackSubimage() is called under UploadImageDataToTexture().

There are 2 call sequences.
[1]

  • UploadImageDataToTexture()
  • TexSubImage2DHelper()
  • TexSubImage2DWithoutUnpackSubimage()

[2]

  • UploadImageDataToTexture()
  • TexImage2DHelper()
  • TexSubImage2DHelper()
  • TexSubImage2DWithoutUnpackSubimage()

UploadImageDataToTexture() supports the following surface formats. All pixel sizes of surface formats are less than or equal to 4. Then TexSubImage2DWithoutUnpackSubimage() does not exceed 2GiB and does not overflow on 32-bit.

  • SurfaceFormat::B8G8R8X8
  • SurfaceFormat::R8G8B8A8
  • SurfaceFormat::R8G8B8X8
  • SurfaceFormat::R5G6B5_UINT16
  • SurfaceFormat::A8
  • SurfaceFormat::A16

https://searchfox.org/mozilla-central/source/gfx/gl/GLUploadHelpers.cpp#293

Flags: needinfo?(sotaro.ikeda.g)

Similar buffer allocation exists in TexImage2DHelper(). It is "new unsigned char[paddedWidth * paddedHeight * pixelsize]".

By comment 3, overflow does not happen on current gecko.

Should we perhaps use CheckedInt here to ensure that even if we change things in the future we catch any overflows that may occur?

Flags: needinfo?(jgilbert)

And maybe a runtime assert in UploadImageDataToTexture about the image size because I'm sure we have other spots that will keel over if we ever exceed that size.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-highsec-low
Summary: Potential out-of-bounds write in TexSubImage2DWithoutUnpackSubimage → Latent out-of-bounds write in TexSubImage2DWithoutUnpackSubimage

Yep, CheckedInt would be perfect here.

Flags: needinfo?(jgilbert)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main68+]
Alias: CVE-2019-11726

This unfortunately does not qualify for a bounty because the latent bug is unreachable.

Flags: sec-bounty? → sec-bounty-

Because this is a latent issue, I'm unassigned the CVE and putting this into the Firefox rollup. (The CVE is burned due to exposure though.)

Alias: CVE-2019-11726
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: