xsscx
commented
1 year ago @maxderhak - Please reconsider this Pull Request given the recent Commit https://github.com/InternationalColorConsortium/DemoIccMAX/commit/f184f38b8ba8f96e32b2ea757b99a301e1838f41 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 https://github.com/InternationalColorConsortium/DemoIccMAX/commit/f184f38b8ba8f96e32b2ea757b99a301e1838f41.
- 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
- 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.
- Index Calculation: The calculation of indices (n000, n001, n010, n011) is implicit and assumes a specific layout of m_pData without explicit boundary checks.
- Error Handling: There is no explicit handling of out-of-bounds conditions except for adjusting ix, iy, u, and t.
Proposed Patch
- 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.
- 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.
- 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 https://github.com/InternationalColorConsortium/DemoIccMAX/commit/f184f38b8ba8f96e32b2ea757b99a301e1838f41 the Issue may be Reproduced as follows:
1. Build with Address Sanitizer
2. Run TestsPoC
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 + 1942CIccCLUT::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.0000CIccCLUT::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 https://github.com/InternationalColorConsortium/DemoIccMAX/pull/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/
Patches for IccTagLut.cpp in CIccCLUT::Interp2d and CIccCLUT::Interp3d functions.
It has been found that there is a Bus Crash in IccTagLut.cpp at line 2447.
CIccCLUT::Interp2d Patch Summary | CVE-2023-48736
CIccCLUT::Interp2d Expected Output
Patch Validation
CIccCLUT::Interp2d Crash Details
CIccCLUT::Interp3d Patch Summary | CVE-2023-46866
Description of Overflow in https://github.com/InternationalColorConsortium/DemoIccMAX/issues/54
PoC
This Patch seeks to avoid Overflows a la PR 53.
Patch Validation
Knowledgebase
https://bugs.chromium.org/p/project-zero/issues/detail?id=2225 https://bugs.chromium.org/p/project-zero/issues/detail?id=2226 https://srd.cx/cve-2022-26730/ https://srd.cx/cve-2023-32443/ Clang Static Analyzer Report https://xss.cx/2023/10/29/src/demomax-clang-static-analysis/ Pull Request https://github.com/InternationalColorConsortium/DemoIccMAX/pull/53 Issue https://github.com/InternationalColorConsortium/DemoIccMAX/issues/54