Fix SSH_PUBKEYINFO, limit characters, add tests

We fix a bad_bufptr() failure from a previous commit. We now limit
the allowed characters to those that will definitely be safe
in a shell. Some scripts/programs may use arbitrary environment
variables without escaping correctly - that could be a problem
in a restricted environment.

The current allowed set is a-z A-Z 0-9 .,_-+@

This also adds a test for SSH_PUBKEYINFO, by default it only runs
under github actions (or "act -j build").

.github/workflows/build.yml

@@ -86,6 +86,8 @@ jobs:
       # for fuzzing
       CXX: clang++
       RANLIB: ${{ matrix.ranlib || 'ranlib' }}
+      # some pytests depend on special setup from this file. see authorized_keys below.
+      DBTEST_IN_ACTION: true
 
     steps:
       - name: deps
@@ -128,7 +130,14 @@ jobs:
       - name: keys
         run: |
           mkdir -p ~/.ssh
+          # remove old files so we can rerun in-place with "act -r" during test development
+          rm -vf ~/.ssh/id_dropbear*
           ~/inst/bin/dropbearkey -t ecdsa -f ~/.ssh/id_dropbear | grep ^ecdsa > ~/.ssh/authorized_keys
+
+          # to test setting SSH_PUBKEYINFO, replace the trailing comment
+          ~/inst/bin/dropbearkey -t ecdsa -f ~/.ssh/id_dropbear_key2 | grep ^ecdsa | sed 's/[^ ]*$/key2 extra/' >> ~/.ssh/authorized_keys
+          ~/inst/bin/dropbearkey -t ecdsa -f ~/.ssh/id_dropbear_key3 | grep ^ecdsa | sed 's/[^ ]*$/key3%char/' >> ~/.ssh/authorized_keys
+          ~/inst/bin/dropbearkey -t ecdsa -f ~/.ssh/id_dropbear_key4 | grep ^ecdsa | sed 's/[^ ]*$/key4,char/' >> ~/.ssh/authorized_keys
           chmod 700 ~ ~/.ssh ~/.ssh/authorized_keys
           ls -ld ~ ~/.ssh ~/.ssh/authorized_keys
 

svr-authpubkey.c

@@ -261,6 +261,7 @@ static int checkpubkey_line(buffer* line, int line_num, const char* filename,
 		const char* algo, unsigned int algolen,
 		const unsigned char* keyblob, unsigned int keybloblen) {
 	buffer *options_buf = NULL;
+	char *info_str = NULL;
 	unsigned int pos, len, infopos, infolen;
 	int ret = DROPBEAR_FAILURE;
 
@@ -339,16 +340,36 @@ static int checkpubkey_line(buffer* line, int line_num, const char* filename,
 		goto out;
 	}
 
-	/* truncate the line at the space after the base64 data */
+	/* find the length of base64 data */
 	pos = line->pos;
 	for (len = 0; line->pos < line->len; len++) {
-		if (buf_getbyte(line) == ' ') break;
-	}	
-	/* findout the length of the public key info */
+		if (buf_getbyte(line) == ' ') {
+			break;
+		}
+	}
+
+	/* find out the length of the public key info, stop at the first space */
 	infopos = line->pos;
 	for (infolen = 0; line->pos < line->len; infolen++) {
-		if (buf_getbyte(line) == ' ') break;
+		const char c = buf_getbyte(line);
+		if (c == ' ') {
+			break;
+		}
+		/* We have an allowlist - authorized_keys lines can't be fully trusted,
+		some shell scripts may do unsafe things with env var values */
+		if (!(isalnum(c) || strchr(".,_-+@", c))) {
+			TRACE(("Not setting SSH_PUBKEYINFO, special characters"))
+			infolen = 0;
+			break;
+		}
 	}
+	if (infolen > 0) {
+		info_str = m_malloc(infolen + 1);
+		buf_setpos(line, infopos);
+        strncpy(info_str, buf_getptr(line, infolen), infolen);
+	}
+
+	/* truncate to base64 data length */
 	buf_setpos(line, pos);
 	buf_setlen(line, line->pos + len);
 
@@ -359,26 +380,24 @@ static int checkpubkey_line(buffer* line, int line_num, const char* filename,
 	/* free pubkey_info if it is filled */
 	if (ses.authstate.pubkey_info) {
 		m_free(ses.authstate.pubkey_info);
-		ses.authstate.pubkey_info = NULL;
 	}
+
 	if (ret == DROPBEAR_SUCCESS) {
 		if (options_buf) {
 			ret = svr_add_pubkey_options(options_buf, line_num, filename);
 		}
-		/* save the (optional) public key information */
-		if (infolen) {
-			ses.authstate.pubkey_info = m_malloc(infolen + 1);
-			if (ses.authstate.pubkey_info) {
-		                strncpy(ses.authstate.pubkey_info,(const char *) buf_getptr(line, infopos), infolen);
-				ses.authstate.pubkey_info[infolen]='\0';
-			}
-		}
+		/* take the (optional) public key information */
+		ses.authstate.pubkey_info = info_str;
+		info_str = NULL;
 	}
 
 out:
 	if (options_buf) {
 		buf_free(options_buf);
 	}
+	if (info_str) {
+		m_free(info_str);
+	}
 	return ret;
 }
 

test/test_svrauth.py

@@ -0,0 +1,30 @@
+from test_dropbear import *
+import signal
+import queue
+import socket
+import os
+from pathlib import Path
+
+# Tests for server side authentication
+
+# Requires keyfile and authorized_keys set up in github action build.yml
+@pytest.mark.skipif('DBTEST_IN_ACTION' not in os.environ, reason="DBTEST_PUBKEYINFO not set")
+def test_pubkeyinfo(request, dropbear):
+	kf = str(Path.home() / ".ssh/id_dropbear_key2")
+	r = dbclient(request, "-i", kf, "echo -n $SSH_PUBKEYINFO", capture_output=True)
+	# stop at first space
+	assert r.stdout.decode() == "key2"
+
+@pytest.mark.skipif('DBTEST_IN_ACTION' not in os.environ, reason="DBTEST_PUBKEYINFO not set")
+def test_pubkeyinfo_special(request, dropbear):
+	kf = str(Path.home() / ".ssh/id_dropbear_key3")
+	r = dbclient(request, "-i", kf, "echo -n $SSH_PUBKEYINFO", capture_output=True)
+	# comment contains special characters so the SSH_PUBKEYINFO should not be set
+	assert r.stdout.decode() == ""
+
+@pytest.mark.skipif('DBTEST_IN_ACTION' not in os.environ, reason="DBTEST_PUBKEYINFO not set")
+def test_pubkeyinfo_okchar(request, dropbear):
+	kf = str(Path.home() / ".ssh/id_dropbear_key4")
+	r = dbclient(request, "-i", kf, "echo -n $SSH_PUBKEYINFO", capture_output=True)
+	# comment contains special characters so the SSH_PUBKEYINFO should not be set
+	assert r.stdout.decode() == "key4,char"