[PATCH 5/5] misc: relax fatal errors in ssh_analyze_banner

[Jon Simons <jon@xxxxxxxxxxxxx>]

From 504c147e0ea14f69aace2e69e401df894a622964 Mon Sep 17 00:00:00 2001
From: Jon Simons <jon@xxxxxxxxxxxxx>
Date: Tue, 11 Jul 2017 19:34:18 -0400
Subject: [PATCH 5/5] misc: relax fatal errors in ssh_analyze_banner

Relax the cases where `ssh_analyze_banner` fails to extract a
major and minor version from banners which appear like OpenSSH
banners.

Update the tests to demonstrate that now a banner as might be
sent by `ssh-keyscan(1)` ("SSH-2.0-OpenSSH-keyscan") no longer
returns failure.

Signed-off-by: Jon Simons <jon@xxxxxxxxxxxxx>
---
 src/misc.c                     | 17 +++++++----------
 tests/unittests/torture_misc.c | 40 +++++++++++++++++++---------------------
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/src/misc.c b/src/misc.c
index 21276c68..5b260b15 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -844,6 +844,7 @@ int ssh_analyze_banner(ssh_session session, int server, int *ssh1, int *ssh2) {
             return -1;
     }
 
+    /* Make a best-effort to extract OpenSSH version numbers. */
     openssh = strstr(banner, "OpenSSH");
     if (openssh != NULL) {
         char *tmp = NULL;
@@ -861,11 +862,8 @@ int ssh_analyze_banner(ssh_session session, int server, int *ssh1, int *ssh2) {
                 ((errno == ERANGE) && (major == ULONG_MAX)) ||
                 ((errno != 0) && (major == 0)) ||
                 ((major < 1) || (major > 100))) {
-                ssh_set_error(session,
-                              SSH_FATAL,
-                              "Invalid major version number: %s",
-                              banner);
-                return -1;
+                /* invalid major */
+                goto done;
             }
 
             minor = strtoul(openssh + 10, &tmp, 10);
@@ -873,12 +871,10 @@ int ssh_analyze_banner(ssh_session session, int server, int *ssh1, int *ssh2) {
                 ((errno == ERANGE) && (major == ULONG_MAX)) ||
                 ((errno != 0) && (major == 0)) ||
                 (minor > 100)) {
-                ssh_set_error(session,
-                              SSH_FATAL,
-                              "Invalid minor version number: %s",
-                              banner);
-                return -1;
+                /* invalid minor */
+                goto done;
             }
+
             session->openssh = SSH_VERSION_INT(((int) major), ((int) minor), 0);
 
             SSH_LOG(SSH_LOG_RARE,
@@ -887,6 +883,7 @@ int ssh_analyze_banner(ssh_session session, int server, int *ssh1, int *ssh2) {
         }
     }
 
+done:
     return 0;
 }
 
diff --git a/tests/unittests/torture_misc.c b/tests/unittests/torture_misc.c
index f11f4488..1d0e0f5c 100644
--- a/tests/unittests/torture_misc.c
+++ b/tests/unittests/torture_misc.c
@@ -315,35 +315,33 @@ static void torture_ssh_analyze_banner(void **state) {
     assert_server_banner_accepted("SSH-2.0-OpenSSH_1.99", 0, 1);
     assert_int_equal(SSH_VERSION_INT(1, 99, 0), session->openssh);
 
-    /* OpenSSH banners: major, minor version limits */
-    reset_banner_test();
-    assert_client_banner_rejected("SSH-2.0-OpenSSH_0.99p1");
-    reset_banner_test();
-    assert_server_banner_rejected("SSH-2.0-OpenSSH_0.99p1");
-    reset_banner_test();
-    assert_client_banner_rejected("SSH-2.0-OpenSSH_1.101p1");
-    reset_banner_test();
-    assert_server_banner_rejected("SSH-2.0-OpenSSH_1.101p1");
+    /* OpenSSH banners: major, minor version limits result in zero */
+    assert_client_banner_accepted("SSH-2.0-OpenSSH_0.99p1", 0, 1);
+    assert_int_equal(0, session->openssh);
+    assert_server_banner_accepted("SSH-2.0-OpenSSH_0.99p1", 0, 1);
+    assert_int_equal(0, session->openssh);
+    assert_client_banner_accepted("SSH-2.0-OpenSSH_1.101p1", 0, 1);
+    assert_int_equal(0, session->openssh);
+    assert_server_banner_accepted("SSH-2.0-OpenSSH_1.101p1", 0, 1);
+    assert_int_equal(0, session->openssh);
 
-    /* OpenSSH banners: bogus major */
-    reset_banner_test();
-    assert_client_banner_rejected("SSH-2.0-OpenSSH_X.9p1");
-    reset_banner_test();
-    assert_server_banner_rejected("SSH-2.0-OpenSSH_X.9p1");
+    /* OpenSSH banners: bogus major results in zero */
+    assert_client_banner_accepted("SSH-2.0-OpenSSH_X.9p1", 0, 1);
+    assert_int_equal(0, session->openssh);
+    assert_server_banner_accepted("SSH-2.0-OpenSSH_X.9p1", 0, 1);
+    assert_int_equal(0, session->openssh);
 
-    /* OpenSSH banners: bogus minor */
-    reset_banner_test();
-    assert_server_banner_rejected("SSH-2.0-OpenSSH_5.Yp1");
-    reset_banner_test();
-    assert_client_banner_rejected("SSH-2.0-OpenSSH_5.Yp1");
+    /* OpenSSH banners: bogus minor results in zero */
+    assert_server_banner_accepted("SSH-2.0-OpenSSH_5.Yp1", 0, 1);
+    assert_int_equal(0, session->openssh);
+    assert_client_banner_accepted("SSH-2.0-OpenSSH_5.Yp1", 0, 1);
+    assert_int_equal(0, session->openssh);
 
     /* OpenSSH banners: ssh-keyscan(1) */
-    #if 0 /* these don't pass */
     assert_client_banner_accepted("SSH-2.0-OpenSSH-keyscan", 0, 1);
     assert_int_equal(0, session->openssh);
     assert_server_banner_accepted("SSH-2.0-OpenSSH-keyscan", 0, 1);
     assert_int_equal(0, session->openssh);
-    #endif /* these don't pass */
 
     ssh_free(session);
 }
-- 
2.13.2