CVE-2024-38427

Repo Documentation NVD NIST Details

A logic flaw existed in the CIccTagXmlProfileSequenceId::ParseXml function of the DemoIccMAX Project where the function unconditionally returned false.

DemoIccMAX Project Overview

The DemoIccMAX project (formally known as RefIccMAX) provides an open-source set of libraries and tools for the interaction, manipulation, and application of iccMAX-based color management profiles based on the iccMAX profile specification, as well as legacy ICC profiles defined by earlier ICC profile specifications.

Bug Type

CWE-253: Incorrect Check of Function Return Value

A logic flaw in the CIccTagXmlProfileSequenceId::ParseXml function of the DemoIccMAX project resulted in the unconditional return value of false. This flaw allowed user-controllable inputs in the XML to be processed potentially leading to arbitrary code execution in the context of the user account credential. The code for CIccTagXmlProfileSequenceId::ParseXml was committed in 2015 with approximately 100K lines of code (LoC) and widely reused.

Bug Details

• Patch Date: May 20, 2024
• Reporter: David Hoyt
• Method: Manual Source Code Review
• Patch via Pull Request: 66

Prior Art

Over the past 3 years there have been many similar reports of color profile processing resulting in overflow conditions and memory corruption.

• Project Zero Issue 2225
• Project Zero Issue 2226
• CVE-2022-26730
• CVE-2023-32443

Call Graph for CIccTagXmlProfileSequenceId::ParseXml

When the return value is set to false unconditionally, the ParseXml helper function does not complete its intended parsing process. As a result, the ICC profile is left in an incomplete or inconsistent state. Despite this, the caller proceeds with the validation process, which can lead to erroneous validation results or crashes due to incomplete or corrupt data when processing nodes.

Call Graph for CIccTagXmlDict::ParseXml

When ParseXml returns true correctly, the profile is fully parsed, and tags are processed. The Validate function can assess the profile, producing meaningful warnings and errors.

You can review the Documentation at URL DemoIccMAX Documentation

Manual Code Review

Commit 889db62 was the entry point for code review. The codebase is approximately 100K lines of code, which is significant and requires a deep dive to understand the reference implementation.

I used basic command-line analysis tools like ctags and cscope with vim, and Doxygen with interactive SVG images to review the source code and call graphs. Knowing that any code utilizing ParseXml would be vulnerable, I started by setting up a ctags database using:

ctags -R .

Then setup cscope database using:

find . -type f \( -name "*.c" -o -name "*.cpp" -o -name "*.h" \) > cscope.files
cscope -b -k

Search for ParseXml:

cscope -L -0 ParseXml > cscope_output.txt

Contents of cscope_output.txt

...
IccXML/IccLibXML/IccMpeXml.h <global> 390 virtual bool ParseXml(xmlNode *pNode, std::string &parseStr);
IccXML/IccLibXML/IccProfileXml.h <global> 89 bool ParseXml(xmlNode *pNode, std::string &parseStr);
IccXML/IccLibXML/IccTagXml.cpp <global> 98 bool CIccTagXmlUnknown::ParseXml(xmlNode *pNode, std::string &parseStr)
IccXML/IccLibXML/IccTagXml.cpp <global> 255 bool CIccTagXmlText::ParseXml(xmlNode *pNode, std::string &parseStr)
IccXML/IccLibXML/IccTagXml.cpp <global> 266 bool CIccTagXmlUtf8Text::ParseXml(xmlNode *pNode, std::string &parseStr)
IccXML/IccLibXML/IccTagXml.cpp <global> 277 bool CIccTagXmlZipUtf8Text::ParseXml(xmlNode *pNode, std::string &parseStr)
IccXML/IccLibXML/IccTagXml.cpp <global> 302 bool CIccTagXmlZipXml::ParseXml(xmlNode *pNode, std::string &parseStr)
...

Functions that Indirectly Call into CIccTagXmlProfileSequenceId::ParseXml:

Total .cpp and .h file references:      226
Unique .cpp and .h files:        9

File-wise occurrence count:
IccXML/IccLibXML/IccMpeXml.cpp: 30
IccXML/IccLibXML/IccMpeXml.h: 19
IccXML/IccLibXML/IccProfileXml.cpp: 4
IccXML/IccLibXML/IccProfileXml.h: 1
IccXML/IccLibXML/IccTagXml.cpp: 58
IccXML/IccLibXML/IccTagXml.h: 47
IccXML/IccLibXML/build/Release/usr/local/include/IccMpeXml.h: 19
IccXML/IccLibXML/build/Release/usr/local/include/IccProfileXml.h: 1
IccXML/IccLibXML/build/Release/usr/local/include/IccTagXml.h: 47

CIccTagXmlProfileSequenceId::ParseXml return false

bool CIccTagXmlProfileSequenceId::ParseXml(xmlNode *pNode, std::string &parseStr)
{
  pNode = icXmlFindNode(pNode, "ProfileSequenceId");

  if (!pNode)
    return false;

  m_list->clear();

  for (pNode = icXmlFindNode(pNode->children, "ProfileIdDesc"); pNode; pNode = icXmlFindNode(pNode->next, "ProfileIdDesc")) {
    CIccProfileIdDesc desc;
    const icChar *szDesc = icXmlAttrValue(pNode, "id");

    if (szDesc && *szDesc)
      icXmlGetHexData(&desc.m_profileID, szDesc, sizeof(desc.m_profileID));

    xmlAttr *langCode;

    for (pNode = icXmlFindNode(pNode, "LocalizedText"); pNode; pNode = icXmlFindNode(pNode->next, "LocalizedText")) {
      if ((langCode = icXmlFindAttr(pNode, "languageCountry")) &&
        pNode->children) {
          xmlNode *pText;

          for (pText = pNode->children; pText && pText->type != XML_TEXT_NODE; pText = pText->next);

          if (pText) {
            icUInt32Number lc = icGetSigVal(icXmlAttrValue(langCode));
            CIccUTF16String str((const char*)pText->content);
            desc.m_desc.SetText(str.c_str(), (icLanguageCode)(lc>>16), (icCountryCode)(lc & 0xffff));
          }
          else {
            desc.m_desc.SetText("");
          }
      }
    }
    m_list->push_back(desc);
  }

  return false;
}

The critical point is the return false; This unconditional return value passes unsanitized XML to the caller, potentially leading to arbitrary code execution in the context of the user credential. Let's examine how the code handles data with this unconditional return value and compare it to the corrected return value of true.

Confirm false is incorrect unconditional return value

Taking a look at program execution flow, this is with the function CIccTagXmlProfileSequenceId::ParseXml and the return value == false stepping thru the code in lldb.

* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x000000010121a900 libIccProfLib2.2.dylib`CIccProfile::Validate(this=0x00007ff7bfefede0, sReport="Warning! - Unknown NULL: Unregistered CMM signature.\r\nWarning! - spectralViewingConditionsTag::>illuminantXYZ - XYZNumber appears to be normalized! Y value should reflect absolute luminance.\r\nWarning! - spectralViewingConditionsTag::>surroundXYZ - XYZNumber appears to be normalized! Y value should reflect absolute luminance.\r\n", sSigPath="") const at IccProfile.cpp:2818:10
   2815	    rv = icMaxStatus(rv, i->pTag->Validate(sSigPath + icGetSigPath(i->TagInfo.sig), sReport, this));
   2816	  }
   2817
-> 2818	  return rv;
   2819	}
   2820
   2821	/**

Key Observations:

• Incomplete Parsing: The profile may not be fully parsed, leading to missing or partially processed tags.
• Erroneous Validation: The Validate function may produce incorrect warnings or errors because it is validating an incomplete profile.
• Potential Crashes: Incomplete data structures can lead to crashes or undefined behavior during validation.

Changed Behavior with Correct Return Value (true):

When ParseXml returns true correctly:

Complete Parsing: The profile is fully parsed, and all tags are processed correctly. Accurate Validation: The Validate function can accurately assess the profile, producing meaningful warnings and errors. Stability: The validation process is less likely to encounter crashes due to incomplete data structures.

Reviewing the Code References now that the unconditional return value has been corrected:

Total .cpp and .h file references:      240
Unique .cpp and .h files:        9

File-wise occurrence count:
IccXML/IccLibXML/IccMpeXml.cpp: 37
IccXML/IccLibXML/IccMpeXml.h: 21
IccXML/IccLibXML/IccProfileXml.cpp: 4
IccXML/IccLibXML/IccProfileXml.h: 1
IccXML/IccLibXML/IccTagXml.cpp: 59
IccXML/IccLibXML/IccTagXml.h: 48
IccXML/IccLibXML/build/Release/usr/local/include/IccMpeXml.h: 21
IccXML/IccLibXML/build/Release/usr/local/include/IccProfileXml.h: 1
IccXML/IccLibXML/build/Release/usr/local/include/IccTagXml.h: 48

The key difference is that there are now 240 references instead of 226, indicating additional code paths are exercised with this changed return value.

CIccTagXmlProfileSequenceId::ParseXml return true

Confirm true is correct unconditional return value

The return value is corrected to true in CIccTagXmlProfileSequenceId::ParseXml:

-  return false;
+  return true;

Stepping through the updated code with lldb confirms the fix:

* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step over
    frame #0: 0x00000001000027cf iccFromXml`main + 927
iccFromXml`main:
->  0x1000027cf <+927>: callq  0x100002c95               ; symbol stub for: CIccProfile::Validate(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>) const
    0x1000027d4 <+932>: movl   %eax, %r14d
    0x1000027d7 <+935>: movq   %r15, %r12
    0x1000027da <+938>: testb  $0x1, -0xc0(%rbp)

With the corrected return value of true, the ICC profile parsing, validation, and saving processes complete successfully without errors. The program performs all necessary memory cleanup operations, preventing leaks, and new parsing errors with XML data.

Analysis of XML Unit Test Errors

There are unit test errors that indicate issues with parsing specific XML elements and types.

Import and File Parsing Failures

Example: Failed to parse import RefEstimationImport.xml file.
Implication: The parser failed to process the entire import file, which could indicate further function in the code need review and adjustments.

Tag Member Parsing Failures

Example: Failed to parse tag member float16NumberType.
Implication: The parser encountered issues while parsing specific tag members, likely due to unsupported or incorrectly defined tag types.

Tag Parsing Failures

Example: Unable to parse float16ArrayType (float16NumberType) tag.
Implication: The parser failed to handle specific tags, which may be due to missing implementations or unsupported tag types in the current parser.

Element Parsing Failures

Example: Unable to parse element (CalculatorElement) tag.
Implication: Specific elements within the XML files could not be parsed. This could indicate the inability to process element types.

General Parsing Failures

Example: Unable to parse CMYK-3DLUTs.xml file.
Implication: The parser could not process the entire XML file, possibly due to logic flaws or other memory issues.

Sample Parser Output

+ ../iccFromXml LCDDisplay.xml LCDDisplay.icc
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 401
Number of items parsed: 1203
Number of items parsed: 401
Number of items parsed: 401
Number of items parsed: 1203
Number of items parsed: 401
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 9
Number of items parsed: 3
Number of items parsed: 9
Number of items parsed: 3
Number of items parsed: 1203
Number of items parsed: 401
Number of items parsed: 401
Profile parsed and saved correctly

+ ../iccFromXml LaserProjector.xml LaserProjector.icc
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 256
Number of items parsed: 401
Number of items parsed: 2
Error parsing GridPoints.
Unable to parse element of type CIccMpeXmlEmissionCLUT
Unable to parse element (EmissionCLutElement) starting on line 131
Unable to Parse "multiProcessElementType" (multiProcessElementType) Tag on line 24

Detailed Report

1. Import and File Parsing Failures

   Files Affected:

   • RefEstimationImport.xml
   • 17ChanPart1.xml
   • 17ChanWithSpots-MVIS.xml
   • 18ChanWithSpots-MVIS.xml
   • CMYK-3DLUTs.xml
   • CMYK-3DLUTs2.xml
   • CMYKOGP-MVIS-Smooth.xml
   • ISO22028-Encoded-bg-sRGB.xml
   • ISO22028-Encoded-sRGB.xml
   • LaserProjector.xml
   • NamedColor.xml
   • RefDecC.xml
   • RefDecH.xml
   • RefIncW.xml
   • argbRef.xml
   • calcExercizeOps.xml
   • sRgbEncodingOverrides.xml
   • srgbCalc++Test.xml
   • srgbCalcTest.xml
   • srgbRef.xml

   Issues:

   • Entire files could not be parsed.
   • Likely due to structural issues or format errors.

2. Tag Member Parsing Failures

   Tags Affected:

   • float16NumberType
   • float32NumberType

   Issues:

   • Specific tag members could not be parsed.
   • Likely due to unsupported or incorrectly defined tag types.

3. Tag Parsing Failures

   Tags Affected:

   • float16ArrayType
   • float32ArrayType
   • multiProcessElementType
   • tagArrayType
   • tagStructType
   • uInt16ArrayType

   Issues:

   • Specific tags could not be parsed.
   • Likely due to missing implementations or unsupported tag types in the parser.

4. Element Parsing Failures

   Elements Affected:

   • CalculatorElement
   • EmissionCLutElement
   • CIccMpeXmlCLUT
   • CIccMpeXmlCalculator
   • CIccMpeXmlEmissionCLUT
   • CIccMpeXmlExtCLUT
   • CIccMpeXmlTintArray

   Issues:

   • Specific elements could not be parsed.
   • Likely due to structural issues or unsupported element types.

Codebase Size and Bug Discovery

Bug Prevalence in Large Codebases

In large codebases (100K LoC or more), bugs due to incorrect logic, including wrong unconditional return values, can constitute a significant portion of all bugs. Studies show that about 5-10% of bugs in large codebases are due to incorrect logic flows, which can include wrong return values, especially those affecting control flow and data validation.

Complexity and Code Quality

The DemoIccMAX project, being 100K LoC, represents a substantial codebase where maintaining code quality and consistency is challenging. The complexity of managing such a large codebase often leads to overlooked edge cases and logic errors.