CVE-2023-48736: Patch for CIccCLUT::Interp2d and Interp3d in IccTagLut.cpp by xsscx · Pull Request #58 · InternationalColorConsortium/DemoIccMAX

@maxderhak - Please reconsider this Pull Request given the recent Commit f184f38 does not address these issues in this proposed Patch for CIccCLUT::Interp2d and CIccCLUT::Interp3d.

CIccCLUT::Interp2d Patch Summary

• This proposed CIccCLUT::Interp2d patch improves the safety and correctness of the Interp2d function in Commit f184f38.
• The Patch for CIccCLUT::Interp2d addresses potential out-of-bounds memory access by implementing robust boundary checks and early exit in case of invalid conditions.
• The patch for CIccCLUT::Interp2d enhances readability and maintainability by clearly defining index calculations and handling edge cases more effectively.

Compare & Contrast

The proposed patch for the CIccCLUT::Interp2d function introduces several modifications aimed at preventing heap overflow and ensuring robust interpolation. Compare and contrast these changes with the original function:

Original Function

1. Boundary Handling:
   The original function decreases ix and iy by 1 if they equal mx or my, respectively, and sets u or t to 1.0. This approach can potentially lead to incorrect interpolation near the edges.
2. Index Calculation:
   The calculation of indices (n000, n001, n010, n011) is implicit and assumes a specific layout of m_pData without explicit boundary checks.
3. Error Handling:
   There is no explicit handling of out-of-bounds conditions except for adjusting ix, iy, u, and t.

Proposed Patch

1. Boundary Handling:
   The patch uses >= to check if ix and iy exceed mx - 1 and my - 1, respectively. This approach is more robust, preventing ix and iy from going out of bounds.
2. Index Calculation:
   The patch explicitly calculates indices for each corner of the interpolation square. This clarity helps understand and validate the indexing logic.
   The addition of boundary checks (if (n000Index > maxValidIndex || …)) is a significant improvement. It ensures that the function does not attempt to access m_pData beyond its allocated size.
3. Error Handling:
   If an out-of-bounds condition is detected, the function fills destPixel with zeros and returns early. This is a safe fallback and prevents potential undefined behavior.

Reproduction of Issue

With Commit f184f38 the Issue may be Reproduced as follows:

1. Build with Address Sanitizer
2. Run Tests

PoC

CalcTest %  ../iccApplyNamedCmm -debugcalc rgbExercise8bit.txt 0 1 calcExercizeOps.icc 1
...
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x603400002420)
    frame #0: 0x000000010126bd97 libIccProfLib2.2.dylib`CIccCLUT::Interp2d(this=0x0000614000000040, destPixel=0x0000611000000a40, srcPixel=0x000060300000484c) const at IccTagLut.cpp:2454:10
   2451   dF3 =  t*  u;
   2452
   2453   for (i=0; i<m_nOutput; i++, p++) {
-> 2454     pv = p[n000]*dF0 + p[n001]*dF1 + p[n010]*dF2 + p[n011]*dF3;
   2455
   2456     destPixel[i] = pv;
   2457   }
Target 0: (iccApplyNamedCmm) stopped.
(lldb) fr va
(CIccCLUT *) this = 0x0000614000000040
(icFloatNumber *) destPixel = 0x0000611000000a40
(const icFloatNumber *) srcPixel = 0x000060300000484c
(icUInt8Number) mx = '\x01'
(icUInt8Number) my = '\x01'
(icFloatNumber) x = NaN
(icFloatNumber) y = -1000.5
(icUInt32Number) ix = 0
(icUInt32Number) iy = 4294966296
(icFloatNumber) u = NaN
(icFloatNumber) t = -4.2949673E+9
(icFloatNumber) nt = 4.2949673E+9
(icFloatNumber) nu = NaN
(int) i = 0
(icFloatNumber *) p = 0x0000603400002420
(icFloatNumber) dF0 = NaN
(icFloatNumber) dF1 = NaN
(icFloatNumber) dF2 = NaN
(icFloatNumber) dF3 = NaN
(icFloatNumber) pv = 4.59051364E-41
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x603400002420)
  * frame #0: 0x000000010126bd97 libIccProfLib2.2.dylib`CIccCLUT::Interp2d(this=0x0000614000000040, destPixel=0x0000611000000a40, srcPixel=0x000060300000484c) const at IccTagLut.cpp:2454:10
    frame #1: 0x0000000101076099 libIccProfLib2.2.dylib`CIccMpeCLUT::Apply(this=0x0000603000004330, pApply=0x0000603000004720, dstPixel=0x0000611000000a40, srcPixel=0x000060300000484c) const at IccMpeBasic.cpp:5665:12
    frame #2: 0x00000001010c6435 libIccProfLib2.2.dylib`CIccApplyMpe::Apply(this=0x0000603000004720, pDestPixel=0x0000611000000a40, pSrcPixel=0x000060300000484c) at IccTagMPE.h:213:84
    frame #3: 0x00000001010c62e0 libIccProfLib2.2.dylib`CIccSubCalcApply::Apply(this=0x00006020000010b0, pDestPixel=0x0000611000000a40, pSrcPixel=0x000060300000484c) at IccMpeCalc.h:432:99
    frame #4: 0x00000001010c5f69 libIccProfLib2.2.dylib`CIccOpDefSubElement::Exec(this=0x0000602000000690, op=0x00000001000cdc28, os=0x00007ff7bfef7b60) at IccMpeCalc.cpp:374:17
    frame #5: 0x00000001010ac97d libIccProfLib2.2.dylib`CIccCalculatorFunc::ApplySequence(this=0x00006030000043c0, pApply=0x0000608000000a20, nOps=40670, ops=0x00000001000c3800) const at IccMpeCalc.cpp:3663:21
    frame #6: 0x00000001010ad378 libIccProfLib2.2.dylib`CIccCalculatorFunc::Apply(this=0x00006030000043c0, pApply=0x0000608000000a20) const at IccMpeCalc.cpp:3690:8
    frame #7: 0x00000001010b891d libIccProfLib2.2.dylib`CIccMpeCalculator::Apply(this=0x0000606000000e00, pApply=0x0000608000000a20, pDestPixel=0x00007ff7bfefe930, pSrcPixel=0x00007ff7bfefe750) const at IccMpeCalc.cpp:4797:22
    frame #8: 0x00000001010c6435 libIccProfLib2.2.dylib`CIccApplyMpe::Apply(this=0x0000608000000a20, pDestPixel=0x00007ff7bfefe930, pSrcPixel=0x00007ff7bfefe750) at IccTagMPE.h:213:84
    frame #9: 0x00000001012aee00 libIccProfLib2.2.dylib`CIccTagMultiProcessElement::Apply(this=0x00006070000005d0, pApply=0x0000606000000f20, pDestPixel=0x00007ff7bfefe930, pSrcPixel=0x00007ff7bfefe750) const at IccTagMPE.cpp:1467:15
    frame #10: 0x0000000100fa1403 libIccProfLib2.2.dylib`CIccXformMpe::Apply(this=0x000060e000003ca0, pApply=0x0000604000002550, DstPixel=0x00007ff7bfefe930, SrcPixel=0x00007ff7bfefe750) const at IccCmm.cpp:7324:9
    frame #11: 0x0000000100fbd1bc libIccProfLib2.2.dylib`CIccApplyNamedColorCmm::Apply(this=0x0000604000002510, DstPixel=0x00007ff7bfefe930, SrcPixel=0x00007ff7bfefe750) at IccCmm.cpp:9511:18
    frame #12: 0x0000000100fb1e1d libIccProfLib2.2.dylib`CIccCmm::Apply(this=0x00007ff7bfefe2e0, DstPixel=0x00007ff7bfefe930, SrcPixel=0x00007ff7bfefe750) at IccCmm.cpp:8481:20
    frame #13: 0x000000010000bbb7 iccApplyNamedCmm`CIccNamedColorCmm::Apply(this=0x00007ff7bfefe2e0, DstPixel=0x00007ff7bfefe930, SrcPixel=0x00007ff7bfefe750) at IccCmm.h:1769:95
    frame #14: 0x000000010000997a iccApplyNamedCmm`main(argc=6, argv=0x00007ff7bfeff5a8) at iccApplyNamedCmm.cpp:483:30
    frame #15: 0x00007ff806f533a6 dyld`start + 1942

CIccCLUT::Interp2d Expected Output with this Patch

 CalcTest %  ../iccApplyNamedCmm -debugcalc rgbExercise8bit.txt 0 1 calcExercizeOps.icc 1
...
End Calculator Apply

   0.9505    1.0000    1.0891   ;  255.0000  255.0000  255.0000

CIccCLUT::Interp2d Testing

This Patch for CIccCLUT::Interp2d has completed all applicable Tests contain within the Project. Additionally, the function has been Fuzzed with a wide range of well-known input and has passed a well-defined range of Unit Tests to be a merge candidate.

CIccCLUT::Interp3d Patch Summary

• This PR Patch for CIccCLUT::Interp2d and Interp3d in IccTagLut.cpp #58 also addresses the same Issues described above.

Please consider Merging this PR to address Boundary Handling, Index Calculations and Error Handling in these 2 functions/