Skip to content

Update: Fix the Compile Error in IccUtil.cpp, Fix a return value in IccTagXml.cpp #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 20, 2024

Conversation

xsscx
Copy link
Collaborator

@xsscx xsscx commented May 14, 2024

Change the unconditional return statement at the end of the CIccTagXmlProfileSequenceId::ParseXml function from false to true, Fixed the Compile Error in IccUtil.cpp due to integer value 4294967295 is outside the valid range of values [0, 3] for this enumeration type.

This is the only code change proposed for this PR.

CVE-2024-38427 | Incorrect Function Return Value in CIccTagXmlProfileSequenceId::ParseXml

TL;DR

A logic flaw existed in the CIccTagXmlProfileSequenceId::ParseXml function of the DemoIccMAX Project where the function unconditionally returned false has been assigned CVE-2024-38427.

CVE-2024-38427 CIccTagXmlProfileSequenceId::ParseXml

CVSS 3.1 Base Score: 8.8 AV:N/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H

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 Details

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

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.

Prior Art

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

PoC

PoC for CVE-2022

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 bool CIccTagXmlProfileSequenceId::ParseXml	(	xmlNode *	pNode, std::string &	parseStr )

Understanding the Call Graph Nodes

The nodes in the call graph represent function calls made within CIccTagXmlProfileSequenceId::ParseXml.

Functions & Purposes:

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.

Call Graph for bool CIccTagXmlProfileSequenceId::ParseXml	(	xmlNode *	pNode, std::string &	parseStr )

DemoIccMAX Documentation

Finding a Logical Flaw

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

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, leading to 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:

    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:

    Issues:

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

DORKs for IccXmlLib or IccProfLib

Finding Open Source Repositories

inurl:github.com "CIccTagXmlProfileSequenceId"
inurl:gitlab.com "CIccTagXmlProfileSequenceId"

Identifying Web Applications and Services:

"ICC profile XML parsing" inurl:app
"embed ICC profile into images" inurl:service
"extract ICC profile from images" inurl:service

Locating Documentation and Tutorials

"CIccTagXmlProfileSequenceId::ParseXml" intitle:documentation
"ICC profile parsing tutorial" intitle:guide

Finding Vulnerable Instances

"XML parsing error" "ICC profile" inurl:log
"XML parsing failure" "ICC profile" inurl:error
intext:"libiccxml" OR intext:"iccproflib" "International Color Consortium" filetype:pdf OR filetype:txt OR filetype:md OR filetype:xml OR filetype:txt OR filetype:cpp
"Libiccxml" OR "iccproflib"
"iccxmllib" OR "iccproflib"

Observations

The use of using convert_type = std::codecvt_utf8<wchar_t>; and its related functionality in the DemoIccMAX Project is significant and potentially non-trivial to refactor comprehensively.

Details

File: DemoIccMAX-master/IccProfLib/IccTagDict.cpp:88:27
Code: using convert_type = std::codecvt_utf8<wchar_t>;

File: DemoIccMAX-master/IccProfLib/IccTagDict.cpp:216:24 'convert_type' is deprecated
Code: std::wstring_convert<convert_type, wchar_t> converter;

Knowledgebase

My Prior Research

My Current CVE's for the DemoIccMAX Project

xsscx added 2 commits May 13, 2024 21:54
Change the unconditional return statement at the end of the CIccTagXmlProfileSequenceId::ParseXml function from false to true.
…or: integer value 4294967295 is outside the valid range

Update IccUtil.cpp Hoyt's PATCH for ccProfLib/IccUtil.cpp:2064:8: error: integer value 4294967295 is outside the valid range

Distribution Compile Error:
-----------------------------
make -j$(nproc) 2>&1 | grep 'error:'

DemoIccMAX-a9a15564e3418176d60eef88c946831779f95658/IccProfLib/IccUtil.cpp:2064:8: error: integer value 4294967295 is outside the valid range of values [0, 1] for this enumeration type [-Wenum-constexpr-conversion]
DemoIccMAX-a9a15564e3418176d60eef88c946831779f95658/IccProfLib/IccUtil.cpp:2068:5/Users/xss/Downloads/DemoIccMAX-a9a15564e3418176d60eef88c946831779f95658/IccProfLib/IccUtil.cpp:2064:8: :error:  integer value 4294967295 is outside the valid range of values [0, 1] for this enumeration type [-Wenum-constexpr-conversion]warning:
DemoIccMAX-a9a15564e3418176d60eef88c946831779f95658/IccProfLib/IccUtil.cpp:2085:8: error: integer value 4294967295 is outside the valid range of values [0, 3] for this enumeration type [-Wenum-constexpr-conversion]
DemoIccMAX-a9a15564e3418176d60eef88c946831779f95658/IccProfLib/IccUtil.cpp:2085:8: error: integer value 4294967295 is outside the valid range of values [0, 3] for this enumeration type [-Wenum-constexpr-conversion]




Hoyt's Patch
---------------
diff -u   ../IccProfLib/IccUtil.cpp.dist ../IccProfLib/IccUtil.cpp
--- ../IccProfLib/IccUtil.cpp.dist	2024-05-14 08:10:38
+++ ../IccProfLib/IccUtil.cpp	2024-05-14 08:23:25
@@ -2054,39 +2054,36 @@

 const icChar *CIccInfo::GetMeasurementFlareName(icMeasurementFlare val)
 {
-  switch ((int)val) {
+  switch (val) { // Directly switch on the enum without casting to int
   case icFlare0:
-    return "Flare 0";
+    return "Flare 0"; // Properly handle the 0% flare

   case icFlare100:
-    return "Flare 100";
-
-  case icMaxEnumFlare:
-    return "Max Flare";
+    return "Flare 100"; // Properly handle the 100% flare

   default:
-    sprintf(m_szStr, "Unknown Flare '%d'", (int)val);
+    // Handle icMaxEnumFlare explicitly here if needed
+    if (val == icMaxEnumFlare) {
+      return "Max Flare"; // Special handling for the max sentinel value
+    }
+    // General default case for truly unexpected values
+          std::snprintf(m_szStr, sizeof(m_szStr), "Unknown Flare '%d'", (int)val);
     return m_szStr;
   }
 }

+
 const icChar *CIccInfo::GetMeasurementGeometryName(icMeasurementGeometry val)
 {
-  switch ((int)val) {
+  switch (val) {  // Directly use enum type, casting is not needed
   case icGeometryUnknown:
     return "Geometry Unknown";
-
   case icGeometry045or450:
     return "Geometry 0-45 or 45-0";
-
   case icGeometry0dord0:
     return "Geometry 0-d or d-0";
-
-  case icMaxEnumGeometry:
-    return "Max Geometry";
-
-  default:
-    sprintf(m_szStr, "Unknown Geometry '%d'", (int)val);
+  default:
+          std::snprintf(m_szStr, sizeof(m_szStr), "Unknown Geometry '%d'", (int)val);
     return m_szStr;
   }
 }


Compile Result:
make -j$(nproc) 2>&1 | grep 'error:'
@xsscx xsscx changed the title Update IccTagXml.cpp Fix return value Update: Fix the Compile Error in IccUtil.cpp, Fix a return value in IccTagXml.cpp May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants