Asyncssh not connectiong due to "malformed" MSG_USERAUTH_BANNER

[mmayomoar]

Hello,
First of all, Thank you for this wonderful library. I am using it on a daily basis and it is helping me a lot.

I am trying to connect to some APC devices and I the tool is not connecting due to, what I think, a malformed, MSG_USERAUTH_BANNER. The package received is as follows:

DEBUG:asyncssh:[conn=0, pktid=5] Received MSG_USERAUTH_BANNER (53), 80 bytes
  00000000: 35 00 00 00 41 42 61 74 74 65 72 79 20 49 6e 73  5...ABattery Ins
  00000010: 74 61 6c 6c 65 64 3a 20 32 38 2f 30 39 2f 32 30  talled: 28/09/20
  00000020: 32 33 20 42 61 74 74 65 72 79 3a 20 53 52 54 4c  23 Battery: SRTL
  00000030: 42 4d 20 73 65 72 69 61 6c 3a 35 41 32 33 31 39  BM serial:5A2xx9
  00000040: 54 41 36 37 34 31 00 00 00 05 65 6e 2d 55 53 00  TA6741....en-US.

and the responese is as follows:

INFO:asyncssh:[conn=0] Sending disconnect: Unexpected data at end of packet (2)
DEBUG:asyncssh:[conn=0, pktid=7] Sent MSG_IGNORE (2), 5 bytes
  00000000: 02 00 00 00 00                                   .....
DEBUG:asyncssh:[conn=0, pktid=8] Sent MSG_DISCONNECT (1), 50 bytes
  00000000: 01 00 00 00 02 00 00 00 20 55 6e 65 78 70 65 63  ........ Unexpec
  00000010: 74 65 64 20 64 61 74 61 20 61 74 20 65 6e 64 20  ted data at end
  00000020: 6f 66 20 70 61 63 6b 65 74 00 00 00 05 65 6e 2d  of packet....en-
  00000030: 55 53                                            US
INFO:asyncssh:[conn=0] Connection failure: Unexpected data at end of packet
INFO:asyncssh:[conn=0] Aborting connection
SSH connection failed: Unexpected data at end of packet

The problem is coming from the last . MSG_USERAUTH_BANNER that somehow is not being taken as part of the string.

---

I have made some changes in the code and I was able to make it work. Now, it looks like this when connecting:

DEBUG:asyncssh:[conn=0, pktid=5] Received MSG_USERAUTH_BANNER (53), 80 bytes
  00000000: 35 00 00 00 41 42 61 74 74 65 72 79 20 49 6e 73  5...ABattery Ins
  00000010: 74 61 6c 6c 65 64 3a 20 32 38 2f 30 39 2f 32 30  talled: 28/xx/20
  00000020: 32 33 20 42 61 74 74 65 72 79 3a 20 53 52 54 4c  23 Battery: SRTL
  00000030: 42 4d 20 73 65 72 69 61 6c 3a 35 41 32 33 31 39  BM serial:5Axx19
  00000040: 54 41 36 37 34 31 00 00 00 05 65 6e 2d 55 53 00  TA6741....en-US.
DEBUG:asyncssh:[conn=0] Received and extra byte in the packet. Removing it
DEBUG:asyncssh:[conn=0] Received authentication banner
DEBUG:asyncssh:[conn=0] Authentication banner: Battery Installed: 28/09/2023 Battery: SRTLBM serial:5A2319TA6741
DEBUG:asyncssh:[conn=0] Detected Language: en-US
DEBUG:asyncssh:[conn=0, pktid=6] Received MSG_USERAUTH_FAILURE (51), 14 bytes

---

I was wondering if I could create a pull request with the changes and if so, to which branch development or master.
This is my first time trying working in Github. I apologize if I did any mistake.

[ronf]

You're right - the MSG_USERAUTH_BANNER message is definitely malformed. There's an extra NUL byte that doesn't belong after the banner text and language fields, and AsyncSSH doesn't allow for unexpected data at the end of a packet except in very specific circumstances.

What version string does the server send when you first connect to it? Is it something unique to this type of device?

[mmayomoar]

Hello,
Thank you for your reply. Please find below the answer to your questions:

What version string does the server send when you first connect to it?

DEBUG:asyncssh:[conn=0] Sending version SSH-2.0-AsyncSSH_2.14.2
DEBUG:asyncssh:[conn=0] Received version SSH-2.0-cryptlib

Is it something unique to this type of device?
Schneider Electric devices have a big variety of family of products that allow shh connections (sumx, su, rpdu2g, ats4g, sucan ....). For now, I only found this problem on sucan devices, but this could be that the firmware version that the devices are using is very recent (2.5.1.3). I could assume that this problem will be appearing in other family of products in newer firmware versions.

[ronf]

Ok - I believe cryptlib is referring to https://cryptlib.com, which appears to provide SSH functionality on a number of embedded systems. There is an open-source version of the code, but based on a quick look at that source I don't see code which actually generates an auth banner message, so I can't quite tell if this bug was fixed at some point in cryptlib. I'm also not sure if all of the code is available as open source, or if some features are only available in the commercial closed-source version. You might want to report this issue to the cryptlib authors and see if it is something they're aware of.

I'm happy to work around the issue in AsyncSSH as well, but just wanted to narrow down the check to avoid impacting other platforms.

Here's an example of such a check in AsyncSSH today in SSHConnection._process_ignore:

        # Work around missing payload bytes in an ignore message
        # in some Cisco SSH servers
        if b'Cisco' not in self._server_version: # pragma: no branch
            _ = packet.get_string()     # data
            packet.check_end()

I'm thinking something similar might be possible in _process_userauth_banner for this issue, replacing:

        msg_bytes = packet.get_string()
        lang_bytes = packet.get_string()
        packet.check_end()

with:

        msg_bytes = packet.get_string()
        lang_bytes = packet.get_string()

        if b'cryptlib' in self._server_version:
            # Work around an extra NUL byte appearing in user
            # auth banner message in some cryptlib servers
            if len(packet.get_remaining_payload()) == 1: # pragma: no cover
                packet.get_byte()

        packet.check_end()

Could you give this a try and see if it solves the problem?

[mmayomoar]

The changes that you are proposing worked like a charm :)

Below, if you dont mind, I would like to show you what I came out with for _process_userauth_banner and have your opinion on it:

def _process_userauth_banner(self, _pkttype: int, _pktid: int,
                                 packet: SSHPacket) -> None:
        """Process a user authentication banner message"""

        msg_bytes = packet.get_string()
        lang_bytes = packet.get_string()
        if packet.get_remaining_payload() == b'\x00':
            # Removing extra NUL byte at the end of the package if exists because causes issues
            # when retrieving the auth banner. Removing the last NUL byte solves the problem
            self.logger.debug2('Received and extra byte in the packet. Removing it')
            packet.get_byte()
        packet.check_end()

        try:
            msg = msg_bytes.decode('utf-8')
            lang = lang_bytes.decode('ascii')
            if lang == "":
                lang = "No Language detected"
        except UnicodeDecodeError:
            raise ProtocolError('Invalid userauth banner') from None

        self.logger.debug1('Received authentication banner')
        self.logger.debug1('Authentication banner: %s', msg)
        self.logger.debug1('Detected Language: %s', lang)

        if self.is_client():
            cast(SSHClient, self._owner).auth_banner_received(msg, lang)
        else:
            raise ProtocolError('Unexpected userauth banner')

I added some extra logging to write the auth banner and language detected.

[ronf]

I like your choice to just compare the value of get_remaining_payload() instead of testing its length. As long as you only ever see NUL bytes here, that's more strict, and it's potentially slightly cheaper.

As for the logging of lang, I don't think that belongs as part of this change. If this were done, it should be done in all the places that lang is sent/received. That said, I'm not sure this part of the standard actually gets much use in practice. OpenSSH pretty much always fills in an empty string or empty list whenever it sends a lang value, and from what I can tell this is not just a default setting -- it's hardcoded to always send an empty value.

Thanks for confirming the fix worked. I'll try to get an updated version of this committed in the next few days.

[mmayomoar]

Sounds good to me :) . On my side, I will try to get in touch with Schneider to suggest them to get an updated version of the cryptlib library for future versions of the firmware on their Network cards.
Will this fix go directly to master or to the development branch?

[ronf]

This fix is now in commit 3242743 in the "develop" branch. It will get merged to master when I do the next release.

I've got a few other changes I'd like to get in before doing a release, but if all goes well I should be able get that out in the next couple of weeks.

Regarding updating to a newer version of cryptlib, I don't know for sure if this has already been fixed in cryptlib or not. You may want to report the bug to the cryptlib maintainers first. That would also let you know which version to suggest upgrading to if it has been fixed.

Thanks for reporting this, and sharing your proposed fix!

[mmayomoar]

Thanks to you Ron for the fast support provided. I will be looking from time to time if the fix was deployed

[ronf]

This change is now available in AsyncSSH 2.15.0.

[mmayomoar]

Thank you :)