Too many nested tags will lead to stack space exhaustion, resulting in signal 11 (SIGSEGV) #249
Comments
|
I only tested it on version 1.12.1，however, the ixmlNode_free() in 1.12.1 and the ixmlNode_free() in the latest version remain unchanged. |
|
CVE-2021-28302 appears to have been assigned to this issue. |
|
Hi @xzjpgithub Looks serious indeed. IXML has been around for quite a while, as this report suggests, the code is not very secure. A proper fix should either detect that the has not been closed before and reject the DOM. I think we have discussed this issue before, but maybe it is time to return to it. Shouldn't we replace IXML? Personally I am not a fan of this code. It is a separate library inside libupnp. Parsers are tricky, and even more are the recursive implementations of them. Does anyone know a nice substitute? As always, patches are welcome. |
|
hi, may be you can try tinyxml2 , https://github.com/leethomason/tinyxml2 |
Looks very nice, indeed, just two files and we're done! But it is C++ and libupnp is C. We could start requiring C++ and compile the whole project with it, but I am not quite sure if the embedded world would happily agree with that. |
|
Hello! We are looking forward to patching this issue in GNU Guix, any update about this? Thanks, |
|
Still not, but we have not forgotten. It turns out that Tinyxml2 does not support XML namespaces, and we need it. So we will need a quick fix to the issue instead of putting Tinyxml2 inside. |
|
@mrjimenez I see thank you, no rush, I'll receive a notification from here when there is progress, thank you :-) |
|
I have just tried it on my desktop, the server does not crash. Maybe it is a problem with the size of the document. What should we do? Seems like we can't blindly read the XML. Or else we get subject to this kind of attack |
|
Hi folks, To make it more crash prone, I did <Anomaly>""" + "<a>"*100000000 + """</Anomaly>""" + """And the server did not crash. Anyway, @xzjpgithub , I did an implementation of a non-recursive ixmlNode_free() here #306 , which unfortunately is still leaking memory, but is a start. Could you try with this version and see if the server still crashes? If anyone would like to give me some help there revising the code, I would appreciate. |
|
Hi @xzjpgithub and @leo-lb , The leak has been resolved and the new ixmlDocument_free() has been committed. Since I have never been able to reproduce the crash, and since this new version of the function is non-recursive, I would like a test feedback from you to close this issue and CVE-2021-28302. Regards, |
|
oh, sorry for long time no responsing. The more small size of the stack, the more the payload cause to a crash. May be it's easily to reproduce the crash if you use "ulimit -s 1024"(default value is 8192(KB)) |
|
Ok, good suggestion. But could you test with the latest version of the library? The function ixmlNode_free() is no longer recursive, so we would like to make sure that the problem is gone under your test conditions. |
|
|
@xzjpgithub: What's up with that? |
hi, this poc caused a crash.
When parsing xml, if there if too many "" in it ,that causes a crash.
The problem is in the function Parser_parseDocument() ， After parses all < a >, it can't find the closed node, and finally enters the errorhandler. When using ixmlDocument_free()， When releases gRootDoc, ixmlNode_free() will release the child node recursively, which will consume stack space. If the recursive depth is not limited, it will cause crash. POC and crash are below.
I suggest adding an interface that limits the depth of recursion.
the stack size of my device
$ulimit -s
8192
poc:
android crash
tombstone:
The text was updated successfully, but these errors were encountered: