Issue #1529: Properly handle SFTP keys from SQL databases where key h…

…eaders may have unexpectedly missing/empty values. (#1796)

NEWS

@@ -21,6 +21,8 @@
   memory error.
 - Issue 1780 - Using mod_facl and mod_vroot at same time causes unexpected
   permissions issues.
+- Issue 1529 - mod_sftp_sql logs "header value too long" due to unexpected key
+  header text.
 
 1.3.9rc2 - Released 19-Dec-2023
 --------------------------------

contrib/mod_sftp_sql.c

@@ -1,6 +1,6 @@
 /*
  * ProFTPD: mod_sftp_sql -- SQL backend module for retrieving authorized keys
- * Copyright (c) 2008-2023 TJ Saunders
+ * Copyright (c) 2008-2024 TJ Saunders
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -96,7 +96,8 @@ static char *sqlstore_getline(pool *p, char **blob, size_t *bloblen) {
     return NULL;
   }
 
-  while (data != NULL && datalen > 0) {
+  while (data != NULL &&
+         datalen > 0) {
     char *ptr;
     size_t delimlen, linelen;
     int have_line_continuation = FALSE;
@@ -190,13 +191,28 @@ static char *sqlstore_getline(pool *p, char **blob, size_t *bloblen) {
       }
 
       /* Header value starts at 2 after the ':' (one for the mandatory
-       * space character.
+       * space character.  Make sure to check that we actually have text
+       * after the ':' character (see Issue #1529).
        */
-      header_valuelen = linelen - (header_taglen + 2);
-      if (header_valuelen > 1024) {
+      if (header_taglen + 2 < linelen) {
+        header_valuelen = linelen - (header_taglen + 2);
+        if (header_valuelen > 1024) {
+          (void) pr_log_writefile(sftp_logfd, MOD_SFTP_SQL_VERSION,
+            "header value too long (%u) in retrieved SQL data for '%s'",
+            header_valuelen, sqlstore_user);
+          errno = EINVAL;
+          return NULL;
+        }
+
+      } else {
         (void) pr_log_writefile(sftp_logfd, MOD_SFTP_SQL_VERSION,
-          "header value too long (%u) in retrieved SQL data for '%s'",
-          header_valuelen, sqlstore_user);
+          "empty/missing '%.*s' header value, ignoring", (int) header_taglen,
+          line);
+
+        /* Make sure we advance past this line. */
+        *blob = data;
+        *bloblen = datalen;
+
         errno = EINVAL;
         return NULL;
       }

tests/t/lib/ProFTPD/Tests/Modules/mod_sftp_sql.pm

@@ -103,6 +103,11 @@ my $TESTS = {
     test_class => [qw(bug forking ssh2)],
   },
 
+  ssh2_auth_publickey_empty_comment_issue1529 => {
+    order => ++$order,
+    test_class => [qw(bug forking ssh2)],
+  },
+
 };
 
 sub new {
@@ -3264,4 +3269,150 @@ EOS
   test_cleanup($setup->{log_file}, $ex);
 }
 
+sub ssh2_auth_publickey_empty_comment_issue1529 {
+  my $self = shift;
+  my $tmpdir = $self->{tmpdir};
+  my $setup = test_setup($tmpdir, 'sftp_sql');
+
+  my $db_file = File::Spec->rel2abs("$tmpdir/sftp.db");
+
+  my $rsa_rfc4716_data = '---- BEGIN SSH2 PUBLIC KEY ----
+Comment:
+AAAAB3NzaC1yc2EAAAABIwAAAQEAzJ1CLwnVP9mUa8uyM+XBzxLxsRvGz4cS59aPTgdw7j
+Gx1jCvC9ya400x7ej5Q4ubwlAAPblXzG5GYv2ROmYQ1DIjrhmR/61tDKUvAAZIgtvLZ00y
+dqqpq5lG4ubVJ4gW6sxbPfq/X12kV1gxGsFLUJCgoYInZGyIONrnvmQjFIfIx+mQXaK84u
+O6w0CT6KhRWgonajMrlO6P8O7qr80rFmOZsBNIMooyYrGTaMyxVsQK2SY+VKbXWFC+2HMm
+ef62n+02ohAOBKtOsSOn8HE2wi7yMA0g8jRTd8kZcWBIkAhizPvl8pqG1F0DCmLn00rhPk
+Byq2pv4VBo953gK7f1AQ==
+---- END SSH2 PUBLIC KEY ----';
+
+  my $db_script = File::Spec->rel2abs("$tmpdir/sftp.sql");
+
+  my $fh;
+  if (open($fh, "> $db_script")) {
+    print $fh <<EOS;
+CREATE TABLE sftpuserkeys (
+  name TEXT NOT NULL PRIMARY KEY,
+  key BLOB NOT NULL
+);
+
+INSERT INTO sftpuserkeys (name, key) VALUES ('$setup->{user}', '$rsa_rfc4716_data');
+EOS
+    unless (close($fh)) {
+      die("Can't write $db_script: $!");
+    }
+
+  } else {
+    die("Can't open $db_script: $!");
+  }
+
+  my $cmd = "sqlite3 $db_file < $db_script";
+  if ($ENV{TEST_VERBOSE}) {
+    print STDERR "Executing sqlite3: $cmd\n";
+  }
+
+  my @output = `$cmd`;
+
+  unlink($db_script);
+
+  my $rsa_host_key = File::Spec->rel2abs('t/etc/modules/mod_sftp/ssh_host_rsa_key');
+  my $dsa_host_key = File::Spec->rel2abs('t/etc/modules/mod_sftp/ssh_host_dsa_key');
+
+  my $rsa_priv_key = File::Spec->rel2abs('t/etc/modules/mod_sftp/test_rsa_key');
+  my $rsa_pub_key = File::Spec->rel2abs('t/etc/modules/mod_sftp/test_rsa_key.pub');
+
+  my $config = {
+    PidFile => $setup->{pid_file},
+    ScoreboardFile => $setup->{scoreboard_file},
+    SystemLog => $setup->{log_file},
+    TraceLog => $setup->{log_file},
+    Trace => 'ssh2:20 sftp:20 sql:20',
+
+    AuthUserFile => $setup->{auth_user_file},
+    AuthGroupFile => $setup->{auth_group_file},
+    AuthOrder => 'mod_auth_file.c',
+
+    IfModules => {
+      'mod_delay.c' => {
+        DelayEngine => 'off',
+      },
+
+      'mod_sql_sqlite.c' => {
+        SQLAuthenticate => 'off',
+        SQLConnectInfo => $db_file,
+        SQLLogFile => $setup->{log_file},
+        SQLNamedQuery => 'get-user-authorized-keys SELECT "key FROM sftpuserkeys WHERE name = \'%{0}\'"',
+      },
+
+      'mod_sftp.c' => [
+        "SFTPEngine on",
+        "SFTPLog $setup->{log_file}",
+        "SFTPHostKey $rsa_host_key",
+        "SFTPHostKey $dsa_host_key",
+        "SFTPAuthorizedUserKeys sql:/get-user-authorized-keys",
+      ],
+    },
+  };
+
+  my ($port, $config_user, $config_group) = config_write($setup->{config_file},
+    $config);
+
+  # Open pipes, for use between the parent and child processes.  Specifically,
+  # the child will indicate when it's done with its test by writing a message
+  # to the parent.
+  my ($rfh, $wfh);
+  unless (pipe($rfh, $wfh)) {
+    die("Can't open pipe: $!");
+  }
+
+  require Net::SSH2;
+
+  my $ex;
+
+  # Fork child
+  $self->handle_sigchld();
+  defined(my $pid = fork()) or die("Can't fork: $!");
+  if ($pid) {
+    eval {
+      # Allow for server startup
+      sleep(2);
+
+      my $ssh2 = Net::SSH2->new();
+
+      unless ($ssh2->connect('127.0.0.1', $port)) {
+        my ($err_code, $err_name, $err_str) = $ssh2->error();
+        die("Can't connect to SSH2 server: [$err_name] ($err_code) $err_str");
+      }
+
+      unless ($ssh2->auth_publickey($setup->{user}, $rsa_pub_key, $rsa_priv_key)) {
+        my ($err_code, $err_name, $err_str) = $ssh2->error();
+        die("RSA publickey authentication failed: [$err_name] ($err_code) $err_str");
+      }
+
+      $ssh2->disconnect();
+    };
+    if ($@) {
+      $ex = $@;
+    }
+
+    $wfh->print("done\n");
+    $wfh->flush();
+
+  } else {
+    eval { server_wait($setup->{config_file}, $rfh) };
+    if ($@) {
+      warn($@);
+      exit 1;
+    }
+
+    exit 0;
+  }
+
+  # Stop server
+  server_stop($setup->{pid_file});
+  $self->assert_child_ok($pid);
+
+  test_cleanup($setup->{log_file}, $ex);
+}
+
 1;