Closed jgottula closed 3 years ago
Thanks a lot for sharing your "bonus security thoughts" and your observations in general. I really appreciate your efforts.
I've added the required checks for None
where querying an XPath may not return something. Further, exceptions that occur during the parsing attempt are now handled as well. I omitted log messages, although they might be helpful, but I'd like you to check if that works better now first.
I'd like you to check if that works better now first.
Sure. I'll install the issue-113
git branch, blast it with nmap a bit, and let you know.
Sorry for the much-longer-than-intended turnaround time on testing this. Had some unfortunate things come up all of a sudden. (Bugs. Not the programming kind.)
I finally got around to installing the issue-113
branch on my machine today. Seems to work 100% fine in regular operation.
And, when I have nmap
spam the machine with port-scanning nonsense, I no longer see any error spew from wsdd
in the journal. LGTM. π
Sorry for the much-longer-than-intended turnaround time on testing this. Had some unfortunate things come up all of a sudden. (Bugs. Not the programming kind.)
No problem. I appreciate your efforts and sometimes other things are more important than a neat side project like this. π
I finally got around to installing the
issue-113
branch on my machine today. Seems to work 100% fine in regular operation.And, when I have
nmap
spam the machine with port-scanning nonsense, I no longer see any error spew fromwsdd
in the journal. LGTM.
Great, Thanks again. I'll merge with master.
No problem. I appreciate your efforts and sometimes other things are more important than a neat side project like this. π
I think the "bug incident" happened the exact day I was about to test this; and it may have actually been right in the middle of me tweaking my Arch package to build from the git version when all of a sudden it was like "oh shit, we've got a bug problem on our hands", and immediately room accommodations and computer usage and everything else got derailed for a good week or two. π
I will tell you what though, I spent an awful lot of time debugging!
This is similar in subject matter to an earlier issue I filed, #79.
Was doing some intensive nmap scans today against my system running wsdd 0.6.4, and noticed that wsdd would consistently vomit these three same exception stack traces to my system log.
I inserted some extra code to print out the request body to make it easier to see exactly what data nmap was sending in each case, and why that data was causing the logic in wsdd to hit exceptions rather than proactively ignoring it (silently or with a log message or whatever).
Gist containing the three exception log contents
The first two exceptions (despite coming from two distinct XML data bodies generated by nmap that are obviously nonsense-for-WSD) appear to have the same root cause in
WSDMessageHandler.handle_message
: the data is some sort of XML; butElement.find
can't locate XPath'./soap:Header'
; and so it returnsNone
, which is the documented return value if it cannot find the requested XPath; and soheader is None
; but there's noif header is None
check-and-bailout prior to the nextElement.find
call, which is done onheader
at line 374 under the assumption that it's a validElement
rather thanNone
.The third exception happens when nmap sends a completely empty body. In this case, the
ElementTree.fromstring
call at the start ofWSDMessageHandler.handle_message
on line 372 is aliased to methodElementTree.XML
(with default parserElementTree.XMLParser
), which constructsparser = ElementTree.XMLParser(target=TreeBuilder())
, callsparser.feed(text)
, and then returnsparser.close()
, which should be anElement
instance. Deep within the bowels of the C code for the expat parser, the logic gets grumpy that it's seen literally zero bytes at the point that it's told that parsing should be done, so it sets an error code that eventually bubbles up as aElementTree.ParseError
Python exception, with attributecode = xml.parsers.expat.errors.XML_ERROR_NO_ELEMENTS
.A simple band-aid fix would be to explicitly check for the case where
content_length <= 0
, since it's clear that an empty buffer is unexpected and causes an XML parse exception.That's an overly specific fix, though, and I suspect that (maybe in addition to the
content_length
sanity check) it would be wise to wrap theElementTree.fromstring
call in atry
block and handle theElementTree.ParseError
situation. That would cover a wider variety of potentially unparseable inputs.(I suppose with or without that exception handler, either way you're basically going to be telling the user that something just sent you garbage XML; but maybe it'd be preferable to not have the whole traceback and just do a one-line log message, or maybe even just silently discard it. I dunno.)
Something that I noticed on the top-level Python XML documentation page, is that there's a big red warning saying that the default Python XML modules "are not secure against erroneous or maliciously constructed data" and urging people who are parsing untrusted data to consult the XML Vulnerabilities table further down that page, as well as consider using the
defusedxml
package as a drop-in replacement that provides much better-protected versions of the default Python XML interfaces.Given that nmap is generating not-particularly-valid-for-this-protocol-at-least XML and sending it to wsdd, and that in theory nmap (or anything else) could send not just invalid XML but malicious XML and use wsdd as an exploit vector, I thought it might be a good idea to suggest considering using the
defusedxml
package.This has some vague connections to #48. It's not necessarily directly related to the traffic amplification situation; but it's in the same spirit of "sometimes people configure wsdd in a way that allows random crap on the WAN to get to it" and "we ought to be careful about how we handle unverified data to avoid exploits or other problems".
So, yeah. Some bonus security thoughts there.