Are the user commands properly XML encoded/escaped?

[rgl]

Looking at

winrm/request.go

Line 83 in 1d17eaf

command = "<![CDATA[" + command + "]]>"

leaves me doubtful of whether that is properly encoding/escaping the user supplied command.

Why is that just putting the user supplied command in a CDATA section without further encoding/escaping? Why is this even trying to encode/escape the data using a string concatenation? I mean, shouldn't the XML API have a proper way to encode a literal text (as-in the XmlNode.InnerText property of an .NET XML element)?

For example, to properly use a CDATA section you must make sure there is no nested CDATA sections nor the user command has the ]]> string in it, etc.

[masterzen]

Hi,

Thanks for your comment. While that's true there's no escaping of the CDATA, I doubt it would form a valid command anyway :)

I find it particularly difficult to answer your (I suppose rhetorical) question with something else than: because I probably didn't thought about it at that time or that I didn't care enough for my use case at that particular time :)

Now, if you look deeper you'll notice that this project isn't using the Go standard XML parser/encoder but a naive homegrown DOM system (for a lot of reason including that there wasn't any DOM stuff available without native dependencies at the time of writing the library) to implement the SOAP messages building. This DOM thing doesn't implement escaping and assume things are properly escaped (the name of the library tells it all).

The fun fact is that Go doesn't expose a CDATA escaping function, so we have to add our own which might look like the the private golang one.

Would you want to submit a PR with a fix !
That would be terrific :)

Thanks,

[rgl]

I should have warned you, but this kind of stuff is like a pet peeve of mine because I've been hurt by broken implementations so many times.

I was curious to known if there was a reason of why it is like it is (e.g. whether the WinRM/PSRemoting SOAP stack was broken and only worked with this kind of XML or because of any other reason), so if you have any more reasons, please let me known ;-)

I can only imagine the pain you went trough to implement this library and interact with this windows subsystem, and only have to thank you, so thank you! :-)

What do you think of instead of using:

commandElement := message.CreateElement(body, "Command", soap.DOM_NS_WIN_SHELL)
commandElement.SetContent(command)

We change this to:

message.CreateTextElement(body, "Command", soap.DOM_NS_WIN_SHELL, command)

Or:

commandElement := message.CreateElement(body, "Command", soap.DOM_NS_WIN_SHELL)
commandElement.SetContent(emitCDATA(command))

And borrow the go implementation that you've mention to implement it in the MessageBuilder class?

Or even better, just use the regular/public xml.EscapeText?

Because the dom library does not do any escaping, this responsibility is supposed to be handled by winrm library? the soap library?

[masterzen]

@rgl,

Indeed we can consider the responsibility of escaping to be in the winrm library. It's simpler.

xml.EscapeText would definitely work, it would be interesting to test.

The problem with emitCDATA is that we need to check if the license is compatible with the project and keep the copyright attribution (or reimplement).

[rgl]

Go uses a 3 clause BSD-style license and both are compatible. But I will try with the public xml.EscapeText first.