Add some XML validation to avoid xmpsdk bugs

[kevinbackhouse]

Fixes: #1877

Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=37363

Add an XML validation pass to check that the XML is valid, before calling the xmpsdk library. This should help to avoid bugs in xmpsdk. #1877 is a stack overflow, caused by a deeply nested XML tree, so the validator checks that the tree isn't too deep. (Currently set to a maximum depth of 1000 - that might need to be tweaked.)

[codecov]

Codecov Report

Merging #1878 (1bb6467) into main (c9f253f) will increase coverage by 0.01%.
The diff coverage is 78.66%.

❗ Current head 1bb6467 differs from pull request most recent head 5703dbc. Consider uploading reports for the commit 5703dbc to get more accurate results

@@            Coverage Diff             @@
##             main    #1878      +/-   ##
==========================================
+ Coverage   60.77%   60.79%   +0.01%     
==========================================
  Files          96       96              
  Lines       18887    18962      +75     
  Branches     9498     9516      +18     
==========================================
+ Hits        11478    11527      +49     
- Misses       5116     5134      +18     
- Partials     2293     2301       +8     

Impacted Files	Coverage Δ
src/xmp.cpp	76.81% <78.66%> (-0.39%)	⬇️
src/value.cpp	72.72% <0.00%> (-0.47%)	⬇️
src/iptc.cpp	64.61% <0.00%> (-0.39%)	⬇️
src/convert.cpp	53.50% <0.00%> (-0.35%)	⬇️
src/actions.cpp	61.13% <0.00%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9f253f...5703dbc. Read the comment docs.

[clanmills]

I haven't studied this to find out what exactly what you're doing. If you'd like me to look carefully, I'll do that on Saturday. I'm really surprised at the detail of the new errors (line/col), isn't possible to just throw "illegal token" and leave the user to figure it in more detail.

Did you write the XMLValidator class from scratch, or was it obtained from the internet?

[kevinbackhouse]

@clanmills: XMLValidator is new code, written by me. It's a fairly basic use of libexpat, just to check that the XML is valid before we run xmpsdk on it. In particular, it checks that the tree isn't too deeply nested, which should prevent the crash that OSS-Fuzz is complaining about.

The detailed error messages with line/column number are displaying the error information produced by libexpat. Those messages are probably only useful for debugging, so I'll dial them down to "INFO" level so that they aren't displayed by default.

[kevinbackhouse]

@clanmills: Do you think this is ok to merge? Also, do you think we should backport it to 0.27-maintenance? (It's fixes a stack overflow due to unbounded recursion, so it's a relatively mild denial of service vulnerability.)

[clanmills]

Apologies for not reading your messages. I saw the emails about this and wrongly thought "I'm not involved". Thanks for pinging me on the chat-server. Correct move.

Good work on class XMLValidator. Looks sensible to me. It's a pity expat doesn't deal with this. I wonder if somebody can cause an infinite loop of XML attributes.

And thank you for "dialing down" the message level.

Let's hope your optimism about closing similar reports is well judged and calm returns.

[kevinbackhouse]

@Mergifyio backport 0.27-maintenance

[mergify]

Command backport 0.27-maintenance: success

Backports have been created

• #1880 Add some XML validation to avoid xmpsdk bugs (backport #1878) has been created for branch 0.27-maintenance