Fix CVE-2024-50615 #1003
Fix CVE-2024-50615 #1003
Conversation
|
@leethomason Can you please have a look at this? To me the patch looks good. It would be nice to have a new release as soon as this is merged! |
|
I think this will fail to parse valid XML like (I did take only very very quick look, so pardon me if this is somehow handled well, but from 10sec look this patch looks to introduce new+wrong behaviour in release mode, while it probably does fix the debug build not triggering asserts) To be fair, this patch does also fix some of the type mismatch between |
|
@ped7g you're right. As Character Reference does not have a limit on the number of characters and can have a long leading zero, I updated the code to remove the limit check. |
|
So now you are technically trading asserts in code for test-coverage -> that's IMHO improvement, tests can be run both in Release and Debug = better robustness to catch bugs in code, and the lib can be used also in debug build without asserts (as long as the code fails gracefully even in those "not intended" states which would originally trigger assert, but that's needed for release build any way). It still feels to me like this whole routine is not completely bulletproof, but I would need to better understand how it is being used (for example right now after another quick look I'm not sure why Right now I would say the PR improves current code, but I guess the lib maintainer should take another look if he likes the change and it's evolving in direction he prefer. (seems I will be using tinyXML2 on my current project rather soon, so I will be able to test these myself and maybe come back with deeper insight and more suggestions, but maybe I should focus on the project code and not nitpick what already works) |
| @@ -473,10 +473,10 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) | |||
| *length = 0; | |||
|
|
|||
| if ( *(p+1) == '#' && *(p+2) ) { | |||
| unsigned long ucs = 0; | |||
| TIXMLASSERT( sizeof( ucs ) >= 4 ); | |||
| unsigned long long ucs = 0; | |||
There was a problem hiding this comment.
These should be stdint types, not long long. Also - why the change? How is overflow being addressed?
| @@ -2666,6 +2666,34 @@ int main( int argc, const char ** argv ) | |||
| doc.PrintError(); | |||
| } | |||
|
|
|||
| { | |||
|
Also, as @ped7g notes, it isn't clear that it's bullet proof. The asserts are there to find bugs, not provide safety, and this does need safety. (But I'm also not totally sure.) |
|
@van-duc-nguyen and @ped7g the code this is trying to fix really isn't good. I did a bit of clean up, and tried an alternate fix. Check out #1009 if you would. |
|
@leethomason Your fix looks nice to me. I use long long for 64-bit to prevent overflow from 32-bit, but your fix does not require a 64-bit variable. This might be an issue if the maximum Unicode code point changes in the future, but I don’t think it will change in the short term. |
|
Fixed with #1003, which uses the test cases from this PR. With my thanks! |
No description provided.