Opened 2 years ago

Closed 2 years ago

#5776 defect closed fixed (fixed)

twisted.conch.ssh.userauth depends on hash ordering

Reported by: antoine Owned by:
Priority: high Milestone:
Component: conch Keywords: review
Cc: z3p Branch:
Author: Launchpad Bug:

Description

Running the cftp script can fail with hash randomization:

$ python2.6 -R bin/conch/cftp -v pitrou.net
2012-07-13 18:58:43+0200 [-] Log opened.
2012-07-13 18:58:43+0200 [-] Starting factory <twisted.conch.client.direct.SSHClientFactory instance at 0x2118f80>
2012-07-13 18:58:43+0200 [SSHClientTransport,client] kex alg, key alg: diffie-hellman-group-exchange-sha1 ssh-rsa
2012-07-13 18:58:43+0200 [SSHClientTransport,client] outgoing: aes256-ctr hmac-sha1 none
2012-07-13 18:58:43+0200 [SSHClientTransport,client] incoming: aes256-ctr hmac-sha1 none
2012-07-13 18:58:43+0200 [SSHClientTransport,client] REVERSE
2012-07-13 18:58:43+0200 [SSHClientTransport,client] NEW KEYS
2012-07-13 18:58:43+0200 [SSHClientTransport,client] setting client server to <twisted.conch.client.default.SSHUserAuthClient instance at 0x2118ef0>
2012-07-13 18:58:43+0200 [SSHClientTransport,client] starting service ssh-userauth
2012-07-13 18:58:43+0200 [SSHClientTransport,client] using agent
2012-07-13 18:58:43+0200 [SSHAgentClient,client] got 3 public keys
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] can continue with: ['publickey', 'password']
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] trying to auth with publickey
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] using key of type RSA
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] can continue with: ['publickey', 'password']
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] trying to auth with publickey
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] using key of type DSA
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] can continue with: ['publickey', 'password']
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] trying to auth with publickey
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] using key of type DSA
2012-07-13 18:58:43+0200 [SSHService ssh-userauth on SSHClientTransport,client] couldn't handle 60
[snip]

At this point, the script blocks instead of connecting successfully.
That's because in userauth.py, the messages table is initialized from the globals() dictionary, and there are duplicate message values. Message number 60 can be mapped to MSG_USERAUTH_PASSWD_CHANGEREQ, MSG_USERAUTH_INFO_REQUEST or MSG_USERAUTH_PK_OK depending on ordering. Without hash randomization, it is always mapped to MSG_USERAUTH_PK_OK.

Attachments (2)

randomhash_userauth.patch (1.3 KB) - added by antoine 2 years ago.
randomhash_userauth2.patch (1.9 KB) - added by antoine 2 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc z3p added

Changed 2 years ago by antoine

comment:2 Changed 2 years ago by antoine

  • Keywords review added

Patch attached.

comment:3 follow-up: Changed 2 years ago by mwh

  • Keywords review removed
  • Owner set to antoine

Hi, thanks for fixing the problem and for adding the testcase. The behaviour around here is pretty horrible :/

Can I ask you to, instead of sorting the names, simply assign MSG_USERAUTH_PASSWD_CHANGEREQ and
MSG_USERAUTH_INFO_REQUEST after the loop?

Also can you add a news file per http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles ? I'll leave it up to you as to whether this is 'misc' or 'bugfix' :-)

comment:4 in reply to: ↑ 3 Changed 2 years ago by antoine

Replying to mwh:

Can I ask you to, instead of sorting the names, simply assign MSG_USERAUTH_PASSWD_CHANGEREQ and
MSG_USERAUTH_INFO_REQUEST after the loop?

That's a better idea indeed. I'm attaching a new patch.

Changed 2 years ago by antoine

comment:5 Changed 2 years ago by antoine

  • Keywords review added

comment:6 Changed 2 years ago by antoine

  • Owner antoine deleted

comment:7 Changed 2 years ago by mwh

  • Resolution set to fixed
  • Status changed from new to closed

(In [34749]) Merge randomhash_userauth2.patch: twisted.conch.ssh.userauth now works correctly with hash randomization enabled.

Author: antoine
Reviewer: mwh
Fixes: #5776

The userauth code depended on mapping message type 60 to MSG_USERAUTH_PK_OK but
that only happened because of luck with dictionary ordering.

Note: See TracTickets for help on using tickets.