Opened 11 years ago

Closed 5 years ago

#1902 enhancement closed fixed (fixed)

compatibility work-around for commercial SSH 2.0.12 misbehaviours

Reported by: matthubb Owned by: z3p
Priority: normal Milestone:
Component: conch Keywords:
Cc: Jean-Paul Calderone, matthubb, lvh@…, jesstess Branch: branches/ssh-commerical-bugs-1902
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

Description

Commercial SSH version 2.0.12 can't be bothered to format DSA signatures according to the IETF. There are work arounds for this in OpenSSH and PuTTY sources.

Additionally the MESSAGE_SERVICE_ACCEPT for this version is an otherwise empty packet.

Here's a horrible hack of a patch to allow conch/ssh to work with said version.

diff -uNr ssh-orig/keys.py ssh/keys.py
--- ssh-orig/keys.py    2004-10-09 04:35:47.000000000 +0100
+++ ssh/keys.py 2006-07-06 13:58:02.000000000 +0100
@@ -429,6 +429,8 @@
         'ssh-dss': verifySignature_dsa,
      }
     objType = objectType(obj)
+    if len(sig) == 40: # SSH.com bug, no header
+        return mapping[objType](obj, sig, data)
     sigType, sigData = common.getNS(sig)
     if objType != sigType: # object and signature are not of same type
         return 0
@@ -439,7 +441,8 @@
     return obj.verify(pkcs1Digest(data, lenSig(obj)), sigTuple)

 def verifySignature_dsa(obj, sig, data):
-    sig = common.getNS(sig)[0]
+    if len(sig) != 40: # SSH.com bug, no header
+        sig = common.getNS(sig)[0]
     assert(len(sig) == 40)
     l = len(sig)/2
     sigTuple = map(Util.number.bytes_to_long, [sig[: l], sig[l:]])
diff -uNr ssh-orig/transport.py ssh/transport.py
--- ssh-orig/transport.py       2006-03-25 23:37:52.000000000 +0000
+++ ssh/transport.py    2006-07-06 13:58:47.000000000 +0100
@@ -591,6 +591,9 @@
         self.connectionSecure()

     def ssh_SERVICE_ACCEPT(self, packet):
+        if len(packet) == 0: # SSH.com bug, empty SERVICE ACCEPT packet
+            self.setService(self.instance)
+            return
         name = getNS(packet)[0]
         if name != self.instance.name:
             self.sendDisconnect(DISCONNECT_PROTOCOL_ERROR, "received accept for service we did not request")

Attachments (1)

conch-sshbugs.patch (9.6 KB) - added by philmayers 11 years ago.
infrastructure for SSH bug workaround test cases; attempt at a test case and the relevant client-side fix

Download all attachments as: .zip

Change History (30)

comment:1 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone matthubb added

What format does it use? Can you add a test case for this behavior?

comment:2 Changed 11 years ago by matthubb

In the immortal works of Mr S G Tatham:

/*
 * Commercial SSH (2.0.13) and OpenSSH disagree over the format
 * of a DSA signature. OpenSSH is in line with the IETF drafts:
 * it uses a string "ssh-dss", followed by a 40-byte string
 * containing two 160-bit integers end-to-end. Commercial SSH
 * can't be bothered with the header bit, and considers a DSA
 * signature blob to be _just_ the 40-byte string containing
 * the two 160-bit integers. We tell them apart by measuring
 * the length: length 40 means the commercial-SSH bug, anything
 * else is assumed to be IETF-compliant.
 */

comment:3 Changed 11 years ago by Glyph

Thanks for discovering this.

Considering that no Twisted developers are likely to have a version of commercial SSH available, a unit test is even more important than usual here. (And usually, it's still a hard requirement of getting patches merged...)

comment:4 Changed 11 years ago by philmayers

Given the need to have a buggy server side to test this behaviour, what is the most appropriate way to implement a unit test? The empty packet will be easy to simulate, but the signing bug much harder - the SSHTransport class uses keys.signData where "keys" is a module, and swapping that out on a sub-class (in the 25 and 40 line functions that use it) would be tricky to say the least.

Given the relatively few places that SSHTransport uses the "keys" module, would a patch be accepted that moved the "keys.doAThing" function calls to be instance variables, then a followup patch implementing a fake buggy SSHServerTransport by swapping them out?

comment:5 Changed 11 years ago by philmayers

I'm going to attach a first try of a patch against SVN trunk. The patch puts the "virtualisation" of the keys module in place, and does the basic infrastructure work in the test module for the test case. However, the test fails - I'm not sure why yet. Will look at it on monday.

Changed 11 years ago by philmayers

Attachment: conch-sshbugs.patch added

infrastructure for SSH bug workaround test cases; attempt at a test case and the relevant client-side fix

comment:6 Changed 11 years ago by philmayers

Note the attached patch also fixes some non-DSA-clean bits of the conch tests.

The conch tests should really have a DSS-DSS test in them too (aside from against the simulated buggy server that is).

comment:7 Changed 10 years ago by philmayers

I can't figure out the problem with the test I wrote failing. Is anyone else able to offer some advice?

comment:8 Changed 7 years ago by lvh

Cc: lvh@… added

This version is four years old. Does the bug still exist?

I've tried to access ssh.com and it appears this software doesn't really exist anymore -- there's Tectia Client/Server which may or may not be the same thing.

comment:9 in reply to:  8 ; Changed 7 years ago by philmayers

Replying to lvh:

This version is four years old. Does the bug still exist?

As I recall, we were actually targetting an embedded system (router/switch) so it may well still exist. I can't remember the specific model. Matt?

comment:10 Changed 7 years ago by lvh

I have contacted the people at SSH.com (now Tectia) about this.

It seems you can not contact support at all without being a customer (so where would I post security problems I've found?), so I contacted their media/PR address.

comment:11 in reply to:  9 Changed 7 years ago by matthubb

Replying to philmayers:

Replying to lvh:

This version is four years old. Does the bug still exist?

As I recall, we were actually targetting an embedded system (router/switch) so it may well still exist. I can't remember the specific model. Matt?

Yes it was for an embedded system, but I honestly can't remember which either. Embedded systems tend to linger.

comment:12 Changed 6 years ago by <automation>

Owner: z3p deleted

comment:13 Changed 6 years ago by Andrés Gasson

Keywords: review added

well I propose we just close the bug as no one seems to care or have any knowledge of anyone requiring the patch- sound sensible - reduces bug count? It will still be searchable if someone wishes to reopen.

comment:14 Changed 6 years ago by philmayers

Close it if you want. Conch has so many bugs in the space of interop with commercial servers that Matt and I have given up using it.

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

reduces bug count

This is not a goal. Providing useful functionality is the goal. Closing tickets without providing useful functionality is just a numbers game and a waste of time.

Conch has so many bugs in the space of interop with commercial servers that Matt and I have given up using it.

I'm really sorry to hear that. Perhaps we will be able to fix this one, since you seem to have put together at least a really good start of a test case. I wish the ticket had gotten the review keyword five years ago when the patch was first attached. :/

comment:16 Changed 6 years ago by z3p

Author: z3p
Branch: branches/ssh-commerical-bugs-1902

(In [33119]) Branching to 'ssh-commerical-bugs-1902'

comment:17 Changed 6 years ago by z3p

Keywords: review removed

The patch doesn't apply on trunk.

comment:18 Changed 6 years ago by z3p

Keywords: review added

But, the branch implements those fixes against trunk, so someone else should review it!

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

Keywords: review removed
Owner: set to z3p

Thanks for picking this one up z3p! This looks good (so far as I can tell without being able to test against the offending proprietary software). Here's some comments, all of which are pretty minor, exclusively about coding standard things:

  1. in twisted/conch/test/test_keys.py
    1. test_verify_DSA_no_prefix needs to be named in compliance with the coding standard
    2. Test method docstrings should not say "should". Just say what the behavior is, eg "these keys are loaded as if they had an ssh-dss prefix."
    3. Test methods should be separated from each other by two blank lines, not just one.
    4. We don't typically put ticket references into the code. If you think this reference is particularly merited, that's fine, but please put a full URL instead of just #1902.
  2. in twisted/conch/test/test_transport.py
    1. test_SERVICE_ACCEPT_no_payload needs to be named in compliance with the coding standard
    2. Same comment about should
    3. Same comment about vertical whitespace
    4. Same comment about ticket reference
  3. in twisted/conch/ssh/keys.py
    1. Same comment about ticket reference. Also, give the comment its own line, if you're going to leave it in.

comment:20 Changed 5 years ago by z3p

(In [34449]) test_keys coding standard updates

  • test case name
  • whitespace
  • ticket reference
  • no "should"

Refs #1902

comment:21 Changed 5 years ago by z3p

(In [34450]) test_transport coding standard updates

  • test case name
  • whitespace
  • ticket reference
  • no "should"

Refs #1902

comment:22 Changed 5 years ago by z3p

(In [34451]) keys coding standard updates

  • ticket reference
  • comment on its own line

Refs #1902

comment:23 Changed 5 years ago by z3p

(In [34452]) newsfile

Refs #1902

comment:24 Changed 5 years ago by z3p

Keywords: review added
Owner: z3p deleted

I believe I've addressed all the review comments. I also added a newsfile. Back up for review.

comment:25 Changed 5 years ago by jesstess

Owner: set to jesstess

comment:26 Changed 5 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to z3p

Thanks for sticking with this, z3p. A couple of comments:

  • You lost a second newline between test_SERVICE_ACCEPT and test_requestService in twisted/conch/test/test_transport.py.
  • I'd tweak the news fragment to read "twisted.conch now supports commercial SSH implementations which don't comply with the IETF standard", to fit the format in ReviewProcess#Newsfiles.

Other than that, build results look good, and I think this is ready to merge!

comment:27 Changed 5 years ago by z3p

(In [34692]) add missing newline

Refs #1902

comment:28 Changed 5 years ago by z3p

(In [34693]) tweak newsfile

Refs #1902

comment:29 Changed 5 years ago by z3p

Resolution: fixed
Status: newclosed

(In [34694]) Merge branch ssh-commerical-bugs-1902: fixes some interop issues with commercial SSH servers

Author: matthubb, philmayers, z3p Reviewer: exarkun, jesstess Fixes: #1902

Commercial SSH servers don't follow the IETF format for DSS keys. They also send SERVICE_ACCEPT messages which don't include the name of the service to start. This branch includes fixes which allow Twisted Conch to communicate with those servers.

Note: See TracTickets for help on using tickets.