Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#5352 defect closed fixed (fixed)

do not register overlapping old DH key exchange messages

Reported by: Antoine Pitrou Owned by:
Priority: normal Milestone: Python-3.x
Component: conch Keywords:
Cc: z3p, Antoine Pitrou Branch: branches/conch-legacy-dh-messages-5352
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

Description

This is tricky. Some legacy SSH messages (MSG_KEXDH_*) have overlapping values with newere ones (MSG_KEX_DH_GEX_*). t.conch.ssh.transport maps all of them by iterating over its own globals. Since the globals are a dict, iteration order is undefined; SSH transports can break nastily if the legacy messages happen to override the newer ones in the mapping, since the former don't have public handlers. Patch attached.

Attachments (2)

ssh_legacy_dh_msgs.patch (458 bytes) - added by Antoine Pitrou 6 years ago.
ssh_legacy_dh_msgs2.patch (641 bytes) - added by Antoine Pitrou 6 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: z3p added

Changed 6 years ago by Antoine Pitrou

Attachment: ssh_legacy_dh_msgs.patch added

comment:2 Changed 6 years ago by Antoine Pitrou

Cc: Antoine Pitrou added
Keywords: review added

comment:3 Changed 6 years ago by Antoine Pitrou

New patch adds a runtime check against regressions, in order to detect them easily. (this was Jean-Paul's suggestion on IRC)

Changed 6 years ago by Antoine Pitrou

Attachment: ssh_legacy_dh_msgs2.patch added

comment:4 Changed 6 years ago by z3p

Author: z3p
Branch: branches/conch-legacy-dh-messages-5352

(In [33124]) Branching to 'conch-legacy-dh-messages-5352'

comment:5 Changed 6 years ago by z3p

Resolution: fixed
Status: newclosed

(In [33132]) Merge conch-legacy-dh-messages-5352: Do not register overlapping old DH key exchange messages

Author: antoine Reviewer: z3p Fixes: #5352

twisted.conch.ssh.transport.messages is created by iterating over globals(). Since globals is a dictionary, the ordering is undefined, so we can end up with old message IDs in messages. This patch checks for those old IDs and doesn't insert them into the dictionary. While not ideal, until exarkun writes some code for iterating over named constants, we can live with it.

comment:6 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Note: See TracTickets for help on using tickets.