config: Do not access negative indexes of "seen" array

[Jakub Jelen <jjelen@xxxxxxxxxx>]

Hello,
running an application with latest libssh processing my configuration
files, I noticed valgrind warning that libssh is accessing
uninitialized value, something like this:

+==31722== Conditional jump or move depends on uninitialised value(s)
+==31722==    at 0x730707D: ssh_config_parse_line (config.c:387)
+==31722==    by 0x7307315: local_parse_file (config.c:312)
+==31722==    by 0x7306900: local_parse_glob (config.c:346)
+==31722==    by 0x7306900: ssh_config_parse_line (config.c:400)
+==31722==    by 0x7307315: local_parse_file (config.c:312)
+==31722==    by 0x7306900: local_parse_glob (config.c:346)
+==31722==    by 0x7306900: ssh_config_parse_line (config.c:400)
+==31722==    by 0x7307445: ssh_config_parse_file (config.c:666)
+==31722==    by 0x7314167: ssh_options_parse_config (options.c:1285)
+==31722==    by 0x1E7489: connect_to_ssh (ssh.c:679)
+==31722==    by 0x1E7879: ssh_file_open (ssh.c:803)
+==31722==    by 0x12F884: bdrv_open_driver (block.c:1191)
+==31722==    by 0x1302B6: bdrv_open_common (block.c:1457)
+==31722==    by 0x133183: bdrv_open_inherit (block.c:2752)

I was not able to reproduce it reliably with libssh testsuite (probably
different complication flags or memory alignment), but it clearly
pointed out to some of the unsupported configuration options that lead
to accessing negative indexes in the "seen" array.

The attached patch should fix the problem. I extended also the
testsuite with some tests for unknown or unsupported configuration
options, making sure this is covered.

Regards,
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.

From b89b56beb21cc5217008e8f84209476c81d3e9c0 Mon Sep 17 00:00:00 2001
From: Jakub Jelen <jjelen@xxxxxxxxxx>
Date: Thu, 16 Aug 2018 10:25:46 +0200
Subject: [PATCH 1/2] config: Do not access negative indexes of seen array

Signed-off-by: Jakub Jelen <jjelen@xxxxxxxxxx>
---
 src/config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/config.c b/src/config.c
index 12792afd..49f221e6 100644
--- a/src/config.c
+++ b/src/config.c
@@ -383,7 +383,8 @@ static int ssh_config_parse_line(ssh_session session, const char *line,
   }
 
   opcode = ssh_config_get_opcode(keyword);
-  if (*parsing == 1 && opcode != SOC_HOST && opcode != SOC_UNSUPPORTED && opcode != SOC_INCLUDE) {
+  if (*parsing == 1 && opcode != SOC_HOST && opcode != SOC_INCLUDE &&
+      opcode > SOC_UNSUPPORTED) { /* Ignore all unknown types here */
       if (seen[opcode] != 0) {
           SAFE_FREE(x);
           return 0;
-- 
2.17.1


From 9d0a8e248962cc540a1fdd581449fd6e2eb8ff28 Mon Sep 17 00:00:00 2001
From: Jakub Jelen <jjelen@xxxxxxxxxx>
Date: Thu, 16 Aug 2018 10:32:11 +0200
Subject: [PATCH 2/2] tests: Unsupported and unknown configuration options do
 not crash

Signed-off-by: Jakub Jelen <jjelen@xxxxxxxxxx>
---
 tests/unittests/torture_config.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/unittests/torture_config.c b/tests/unittests/torture_config.c
index f6cf5377..b21efad0 100644
--- a/tests/unittests/torture_config.c
+++ b/tests/unittests/torture_config.c
@@ -16,6 +16,7 @@ extern LIBSSH_THREAD int ssh_log_level;
 #define LIBSSH_TESTCONFIG6 "libssh_testconfig6.tmp"
 #define LIBSSH_TESTCONFIG7 "libssh_testconfig7.tmp"
 #define LIBSSH_TESTCONFIG8 "libssh_testconfig8.tmp"
+#define LIBSSH_TESTCONFIG9 "libssh_testconfig9.tmp"
 #define LIBSSH_TESTCONFIGGLOB "libssh_testc*[36].tmp"
 
 #define USERNAME "testuser"
@@ -90,6 +91,17 @@ static int setup_config_files(void **state)
                         "Host nopubkey\n"
                         "\tPubkeyAuthentication no\n");
 
+    /* unsupported options and corner cases */
+    torture_write_file(LIBSSH_TESTCONFIG9,
+                        "\n" /* empty line */
+                        "# comment line\n"
+                        "  # comment line not starting with hash\n"
+                        "UnknownConfigurationOption yes\n"
+                        "GSSAPIKexAlgorithms yes\n"
+                        "ControlMaster auto\n" /* SOC_NA */
+                        "VisualHostkey yes\n" /* SOC_UNSUPPORTED */
+                        "");
+
     session = ssh_new();
     *state = session;
 
@@ -106,6 +118,7 @@ static int teardown(void **state)
     unlink(LIBSSH_TESTCONFIG6);
     unlink(LIBSSH_TESTCONFIG7);
     unlink(LIBSSH_TESTCONFIG8);
+    unlink(LIBSSH_TESTCONFIG9);
 
     ssh_free(*state);
 
@@ -269,6 +282,21 @@ static void torture_config_auth_methods(void **state) {
     assert_true(session->opts.flags & SSH_OPT_FLAG_PUBKEY_AUTH);
 }
 
+/**
+ * @brief Verify the configuration parser does not choke on unknown
+ * or unsupported configuration options
+ */
+static void torture_config_unknown(void **state) {
+    ssh_session session = *state;
+    int ret = 0;
+
+    /* test corner cases */
+    ret = ssh_config_parse_file(session, LIBSSH_TESTCONFIG9);
+    assert_true(ret == 0);
+    ret = ssh_config_parse_file(session, "/etc/ssh/ssh_config");
+    assert_true(ret == 0);
+}
+
 int torture_run_tests(void) {
     int rc;
     struct CMUnitTest tests[] = {
@@ -287,6 +315,9 @@ int torture_run_tests(void) {
         cmocka_unit_test_setup_teardown(torture_config_auth_methods,
                                         setup_config_files,
                                         teardown),
+        cmocka_unit_test_setup_teardown(torture_config_unknown,
+                                        setup_config_files,
+                                        teardown),
     };
 
 
-- 
2.17.1