Net::Telnet::Chunk is insecure

[Kaiepi]

Apart from Net::Telnet::Chunk not being very optimized, say a server sends a message like so: IAC WILL NAWS IAC SB 0 80 0 60 IAC IAC IAC IAC IAC IAC IAC IAC IAC IAC IAC IAC. Net::Telnet::Chunk will parse the IAC WILL NAWS and add the rest of the message to the parser buffer. If the server keeps sending messages, two things can happen:

• the connection stays open forever
• the server can keep sending messages until the client runs out of memory

Grammars probably aren't suitable for parsing Telnet messages for this reason. Net::Connection.parse should be refactored to use a more traditional method of parsing Telnet messages to prevent this from happening.

[Kaiepi]

Alternatively, refactor the Net::Telnet::Chunk grammar to throw an error if it encounters a scenario like this. Catch x number of times in the parse method and close the connection once the number of errors thrown in a row exceeds x. The method I mentioned in the OP requires much more refactoring, but if you wish to follow that instead, follow how curl handles parsing telnet messages.

[JJ]

I'd really love to help here, but I don't know the first thing about Telnet. I have no idea what the acronyms above mean, why that's insecure, if there's a test for that already, if someone would have to write the test before...
Could you maybe please clarify, with pointers to the code if that's convenient for you? Catching repeated elements in a list shouldn't be difficult, and writing a test for that, but I'm not sure if that's what you're needing.

[Kaiepi]

It's insecure because if a telnet server is badly written or malicious, it could try to send messages like the example I gave in the OP, which this currently doesn't handle properly. The acronyms represent different telnet commands, which can be found in RFC854. The grammar used to parse telnet messages is here. At the moment, it trusts that all negotiations/subnegotations will be sent exactly as specified in the spec, which may not always be the case. Like I mentioned earlier, adding an error method to the grammar to throw something like X::Net::Telnet::InvalidMessage if any of the tokens in the grammar fail to parse would help mitigate this.

[JJ]

Thanks, that helps. I'll see if I can do something along the weekend.

[Kaiepi]

@JJ any progress?

[JJ]

Not really. I'll see if I can do something here. El lun., 5 nov. 2018 a las 0:27, Ben Davies (<notifications@github.com>) escribió:

…

@JJ <https://github.com/JJ> any progress? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAB9OIIKg0eYuAzpr6uGNUCpCWQ2lW7ks5ur3fvgaJpZM4VyxKa> .

-- JJ