twisted

twisted copied to clipboard

Scope and purpose

Fix a crash that occurs when using Dropbear to connect to a Twisted SSH server.

Contributor Checklist:

• [x] The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10138
• [x] I ran tox -e lint to format my patch to meet the Twisted Coding Standard
• [x] I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
• [x] The title of the PR starts with the associated Trac ticket number (without the # character).
• [ ] I have updated the automated tests and checked that all checks for the PR are green.
• [x] I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
• [x] I have added twisted/twisted-contributors teams to the PR Reviewers.
• [x] The merge commit will use the below format The first line is automatically generated by GitHub based on PR ID and branch name. The other lines generated by GitHub should be replaced.

Merge pull request #10138 from nielsavonds/10138-nielsavonds-dropbearfix

Author: nielsavonds
Reviewer: 
Fixes: ticket:10138

Assign list instead of dict_keys to transport supportedPublicKeys in t.c.ssh.Factory

Thanks Niels for the PR. I am trying to run some manual and automated tests for this PR.

Is this an issue only with dropbear client? Does it required a specific dropbear version? Do you know why this is not an issue with OpenSSH client?

I am doing a root cause analysis trying to find answer myself.. but maybe you already have the answers of some questions and could speed up the review :)

Thanks!

Hi and thanks for your response,

This is an issue that only occurs when connecting with the Dropbear client. We're setting up port forwards, so I'm not sure if it would work for any other connection. The crash occurs in ssh_KEXINIT, here:

        if ord(rest[0:1]):  # Flag first_kex_packet_follows?
            if (
                kexAlgs[0] != self.supportedKeyExchanges[0]
                or keyAlgs[0] != self.gi[0]
            ):
                self.ignoreNextPacket = True  # Guess was wrong

I traced this a bit and with OpenSSH the first if-test is false, so we never get to the part where the supportedKeyExchanges is indexed. However, when using OpenSSH, the supportedKeyExchanges also contains a dict_keys object, it's just never indexed.

The dropbear client command I used to test was:

dbclient -p 2225 -N -R 8000:localhost:8000  user@localhost

For the moment, I managed to work around this in our code, by doing this:

class NobiFactory(factory.SSHFactory):

    def buildProtocol(self, addr):
        result = super().buildProtocol(addr)
        result.supportedPublicKeys = list(result.supportedPublicKeys)
        return result

Let me know if you have any additional questions.

I think it's worth adding a clarifying comment by the change to ssh/factory.py, because that ivar is explicitly documented as being an ordered list, and here we are not imposing any order. I think it's okay, but only because a bunch of things are true:

• This is only used by the server side, and my reading of RFC4253 §7.1 is that only the client's preference order matters for the host-key selection algorithm;
• Conch never sends a speculative (guessed) KEXINIT, so the fact that we're not being consistent about which key alg is our preferred one never matters.

An even better approach might be something along the lines of (untested):

         t.supportedPublicKeys = [ 
             keyAlg 
             for keyAlg in t.supportedPublicKeys
             if keyAlg in self.privateKeys
         ]

This would preserve the existing order in SSHTransportBase.supportedPublicKeys as well as allowing someone to remove key algorithms from that list without having them re-added by buildProtocol().

This is still on my todo list, but for now I am busing with fixing some generic Twisted CI issues

I have executed a small manual test:

$ mkdir ssh-keys
$ ckeygen -t rsa -f ssh-keys/ssh_host_rsa_key
$ ckeygen -t rsa -f ssh-keys/client_rsa
$ python docs/conch/examples/sshsimpleserver.py

From another terminal then do $ telnet localhost 5022

I can see that ssh-rsa is advertised ... so all fine here.

Then I do $ dbclient -p 5022 user@localhost and here fails.

On Ubuntu you can install dropbear client via on sudo apt-get install dropbear-bin

---

The next step is to write an automated unit test and integration test for this.

---

The SSHTransportBase.supportedPublicKeys is documented as a list, but maybe type hinting can be added as part of this PR.

Documentation for first_kex_packet_follows boolean from SSH_MSG_KEXINIT https://datatracker.ietf.org/doc/html/rfc4253#section-7.1

When a new connection is made both client and server will send its SSH_MSG_KEXINIT without waiting for any other message from the other peer.

Is not like other protocols in which the server need to send the first message... in with HTTP in which the client needs to send the first message.

My understanding is that dropbear (and any other ssh-client) can send its list of supported host keys and kex algorithm with SSH_MSG_KEXINIT and and informs the server that the client has already decided what algorithms to use... so the client will send a KEX message like MSG_KEXDH_INIT righ away, without waiting for the server's SSH_MSG_KEXINIT

correct me if I am wrong :)

---

Twisted SSH client will send its client side SSH_MSG_KEXINIT but then will wait to receive the SSH_MSG_KEXINIT from the server before proceeding with SSH_MSG_KEXINIT or other KEX method.

---

FWIW
I see that paramiko is also affected by this dropbear client behaviour https://github.com/paramiko/paramiko/issues/418

And also golang https://github.com/golang/go/issues/16962

My understanding is that the client doesn't force a choice, but that it's a sort of speculative key exchange. If the client correctly guesses the negotiated cipher, then it can reduce the number of roundtrips needed to set up the connection. TLS1.3 does something similar.

We don't really do kex negotiation correctly anyway (I'm sure it's already in the trac backlog) but implementing that is for another PR, I think.

but that it's a sort of speculative key exchange.

Yes. speculative on the client side :)

We don't really do kex negotiation correctly anyway (I'm sure it's already in the trac backlog) but implementing that is for another PR, I think.

Why? Do you have a link or a ticket number?

---

For this PR to land I think that we need at least one unit test to get code coverage for the affected code. @wiml do you plan to create the test or do you want me to create the test?

Thanks!

We don't really do kex negotiation correctly anyway (I'm sure it's already in the trac backlog) but implementing that is for another PR, I think.

Why? Do you have a link or a ticket number?

Just comparing RFC4253 §7.1 against SSHTransportBase.ssh_KEXINIT() — I think conch's algorithm would have been correct in an RSA-only world, but when there are multiple host key algorithms the negotiation algorithm has to take that into account.

A quick look through trac doesn't turn up an entry for this particular bug; I could be wrong about it being an error.

For this PR to land I think that we need at least one unit test to get code coverage for the affected code. @wiml do you plan to create the test or do you want me to create the test?

I've gotten really busy lately; I could take a stab at it in a couple of weeks, but probably not before then.

Thanks for the cleanup Tom.

This was on my todo for a long time.

I have assigned it to myself as a reminder...but if anyone wants to continue working on this, feel free to do so and I can help with a review.