CIccDefaultEncProfileConverter::ConvertFromParams frees a borrowed colorEncodingParams element — heap use-after-free write, then double free (IccEncoding.cpp:259)

[colourbill-ctrl]

Found while reviewing CIccDefaultEncProfileConverter::ConvertFromParams for #1980 / #1984, and filed separately at @xsscx's go-ahead rather than folded into that PR.

Summary

ConvertFromParams deletes a borrowed element pointer belonging to the caller's
colorEncodingParams struct, without removing the entry from the struct. When the owner
later destroys that struct it writes through, and then frees, the dangling pointer.

The result is a heap use-after-free write followed by a virtual destructor dispatch on
freed memory and a second free, all on profile-controlled input.

Mechanism

IccEncoding.cpp:237 obtains the element:

CIccTagFloat32 *pLumMtx = (CIccTagFloat32*)pParams->FindElemOfType(icSigCeptLumaChromaMatrixMbr, icSigFloat32ArrayType);

CIccTagStruct::FindElemOfType (IccTagComposite.cpp:755) returns pEntry->pTag directly.
It is a borrowed pointer — the same API and the same treatment as pWhitePt, pMediaWhitePt,
pSurround and pSegCurve elsewhere in this very function, none of which are deleted.

IccEncoding.cpp:259 then deletes it anyway:

    pLumMtx->GetValues(pMtx->GetMatrix(), 0, 9);
    pLumMtx->GetValues(&lumMtx[0], 0, 9);
    pMpeTag->Attach(pMtx);
    delete pLumMtx;          // <-- borrowed; entry stays in pParams
    bHaveLumMtx = true;

Nothing removes the entry from m_ElemEntries / m_ElemVals, so the struct still lists it.
icConvertEncodingProfile destroys the struct at IccEncoding.cpp:666 (delete pParams),
and ~CIccTagStruct → Cleanup() (IccTagComposite.cpp:615-623) walks the element list:

  for (i=m_ElemVals->begin(); i!=m_ElemVals->end(); i++) {
    if (i->ptr) {
      i->ptr->SetParentObject(nullptr);   // UAF write into freed memory
      delete i->ptr;                      // virtual ~CIccTag dispatch, then second free
    }
  }

SetParentObject is a non-virtual inline setter (IccObject.h:117), so the first step is an
8-byte write at a fixed offset into the freed chunk. ~CIccTag is virtual
(IccTagBasic.h:215), so the following delete reads a vptr out of that chunk and dispatches
through it before freeing it a second time.

Note the ordering: the two GetValues calls above have already copied the matrix into
pMtx and into the local lumMtx[9], so nothing reads pLumMtx after the delete. The
delete is gratuitous — see the fix below.

Reproduction

Requires only a white point and a ceptLumaChromaMatrixMbr of 9 or more floats. The delete
is in the normal flow of the luma-matrix branch — it is not guarded by any failure
condition — so the dangling entry is created before the function decides any return status,
and every return path reaches the same teardown.

Source: lumamtx-double-free.cpp (attached below).

Built against an ASAN-only IccProfLib2 (clang++-18, -fsanitize=address -O0 -g):

==67426==ERROR: AddressSanitizer: heap-use-after-free on address 0x504000000118
WRITE of size 8 at 0x504000000118 thread T0
    #0 IIccObject::SetParentObject(IIccObject*) IccProfLib/IccObject.h:117:64
    #1 CIccTagStruct::Cleanup()                 IccProfLib/IccTagComposite.cpp:621:15
    #2 CIccTagStruct::~CIccTagStruct()          IccProfLib/IccTagComposite.cpp:288:3
    #4 main                                     lumamtx-double-free.cpp:111:3

0x504000000118 is located 8 bytes inside of 40-byte region [0x504000000110,0x504000000138)

freed by thread T0 here:
    #0 operator delete(void*)
    #1 CIccTagFloatNum<float, 1718367026>::~CIccTagFloatNum()  IccProfLib/IccTagBasic.cpp:6979:1
    #2 CIccDefaultEncProfileConverter::ConvertFromParams(...)  IccProfLib/IccEncoding.cpp:259:5

previously allocated by thread T0 here:
    #0 operator new(unsigned long, std::nothrow_t const&)
    #4 CIccTag::Create(icTagTypeSignature)  IccProfLib/IccTagBasic.cpp:339:10

SUMMARY: AddressSanitizer: heap-use-after-free IccProfLib/IccObject.h:117:64

The existing ASAN + UBSAN configuration catches the same thing one step earlier, and is
worth recording because it names the primitive precisely:

IccProfLib/IccTagComposite.cpp:621:15: runtime error: member call on address 0x504000000110
which does not point to an object of type 'IIccObject'
0x504000000110: note: object has invalid vptr

Why the gap matters

The two frees are ~400 lines apart, and the intervening code allocates heavily (pCond,
four CIccCamConverters, curve sets, a second matrix element, plus profile construction).
So the freed 40-byte chunk is likely to be reallocated before the second free. That
suppresses glibc's tcache double-free detection — which relies on a key field an intervening
allocation overwrites — meaning the clean free(): double free detected abort is not the
guaranteed outcome. Instead the vptr read at delete i->ptr comes from whatever now occupies
that memory, and a profile supplies large float arrays throughout.

Realistic outcomes, most to least likely: abort or segfault on untrusted input; heap
corruption with a wrong-type destructor running against a live object; and, with successful
grooming plus an info leak to defeat ASLR, control-flow hijack. I have not built a working
exploit and am not claiming one is easy — but this is an RCE-shaped primitive rather than a
crash-only bug, and it needs no malformed lengths or error paths to reach.

Suggested fix

Delete the delete:

     pLumMtx->GetValues(pMtx->GetMatrix(), 0, 9);
     pLumMtx->GetValues(&lumMtx[0], 0, 9);
     pMpeTag->Attach(pMtx);
-    delete pLumMtx;
     bHaveLumMtx = true;

Both GetValues calls have already copied out everything needed, and pParams owns the
element and will free it correctly. This restores the same borrowed-pointer discipline the
rest of the function already follows.

Adjacent defect at the same site

Two lines above, IccEncoding.cpp:252-253:

    if (!pMtx->SetSize(3, 3))
      return icEncConvertMemoryError;

This returns without releasing pMtx, pMpeTag or pIcc, unlike every other error path in
the function, which does delete pMpeTag; delete pIcc;. A leak rather than a memory-safety
bug, but it is the same three lines and should be fixed in the same change.

Provenance

git log -S "delete pLumMtx" puts the line at 1f0a9dd2 (2015-09-29) — the same commit
already bisected for #1817 and #1982.

Related

• Research: validate ceptWhitePointLuminanceMbr #1980 / fix(#1980): reject a non-physical encoding white-point luminance #1984 — the wlum validation in the same function; that PR deliberately does not
  touch this.
• Bisect: 1f0a9dd CIccCamConverter::ReferenceConditions() dbz #1817, CI Report: dbz at IccCAM.cpp:306 #1950 — earlier defects in this converter's viewing-conditions block.

Happy to take this one with a CTest if you would like it assigned to me, @xsscx.

Reproduction source

// Requires only a white point and a ceptLumaChromaMatrixMbr of >= 9 floats.
// Build:
//   clang++-18 -fsanitize=address -g -O0 -std=c++17 lumamtx-double-free.cpp \
//     -I<repo>/IccProfLib -I<build>/IccProfLib -L<build>/IccProfLib -lIccProfLib2d

#include "IccEncoding.h"
#include "IccProfile.h"
#include "IccTag.h"
#include "IccTagComposite.h"
#include "IccTagBasic.h"

#include <cstdio>
#include <cstring>

#ifdef USEICCDEVNAMESPACE
using namespace iccDEV;
#endif

static bool attachFloats(CIccTagStruct *pParams, icSignature sig,
                         const icFloatNumber *vals, icUInt32Number n)
{
  CIccTagFloat32 *pTag = (CIccTagFloat32 *)CIccTag::Create(icSigFloat32ArrayType);
  if (!pTag)
    return false;
  if (!pTag->SetSize(n)) {
    delete pTag;
    return false;
  }
  for (icUInt32Number i = 0; i < n; i++)
    (*pTag)[i] = vals[i];
  if (!pParams->AttachElem(sig, pTag)) {
    delete pTag;
    return false;
  }
  return true;
}

int main()
{
  CIccTagStruct *pParams = (CIccTagStruct *)CIccTag::Create(icSigTagStructType);
  if (!pParams || !pParams->SetTagStructType(icSigColorEncodingParamsSruct)) {
    std::fprintf(stderr, "setup failed\n");
    return 1;
  }

  const icFloatNumber white[2] = {0.3127f, 0.3290f};

  // The trigger. Identity, so icMatrixInvert3x3 downstream succeeds.
  const icFloatNumber lumaMtx[9] = {
    1.0f, 0.0f, 0.0f,
    0.0f, 1.0f, 0.0f,
    0.0f, 0.0f, 1.0f
  };

  if (!attachFloats(pParams, icSigCeptWhitePointChromaticityMbr, white, 2) ||
      !attachFloats(pParams, icSigCeptMediumWhitePointChromaticityMbr, white, 2) ||
      !attachFloats(pParams, icSigCeptLumaChromaMatrixMbr, lumaMtx, 9)) {
    std::fprintf(stderr, "could not build params\n");
    delete pParams;
    return 1;
  }

  icHeader hdr;
  memset(&hdr, 0, sizeof(hdr));
  hdr.deviceClass = icSigColorSpaceClass;
  hdr.colorSpace = icSigRgbData;
  hdr.pcs = icSigXYZData;
  hdr.version = icVersionNumberV5;
  hdr.renderingIntent = icPerceptual;

  IIccEncProfileConverter *pConverter = IIccEncProfileConverter::GetHandler();
  if (!pConverter) {
    std::fprintf(stderr, "no handler\n");
    delete pParams;
    return 1;
  }

  CIccProfilePtr newIcc = NULL;
  icStatusEncConvert stat = pConverter->ConvertFromParams(newIcc, pParams, &hdr);
  std::fprintf(stderr, "ConvertFromParams returned %d\n", (int)stat);

  // What icConvertEncodingProfile does at IccEncoding.cpp:666.
  std::fprintf(stderr, "deleting params struct...\n");
  delete pParams;

  std::fprintf(stderr, "survived -- no sanitizer report\n");
  if (newIcc)
    delete newIcc;
  return 0;
}

[github-actions]

Thanks for opening this issue.

The triage bot added initial labels based on the template and issue text. Maintainers may adjust labels as they review scope, reproduction details, CTest/regression needs, and security impact.

If you have questions, contact a listed Maintainer.