Merge pull request #103 from marlam/validate-code-input

Validate verification code input before sending it

src/pam_weblogin.c

@@ -49,11 +49,21 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, UNUSED int flags, int arg
 		log_message(LOG_ERR, "Error getting user");
 		return PAM_SYSTEM_ERR;
 	}
-	/* Get RHOST. This should always be valid since this PAM module is used with SSH. */
+	if (!username || !input_is_safe(username, MAX_INPUT_LENGTH))
+	{
+		log_message(LOG_ERR, "Invalid user");
+		return PAM_SYSTEM_ERR;
+	}
+	/* Get RHOST. We should always get a valid one in the sshd service context, but maybe not in other contexts. */
 	const char *rhost;
 	if (pam_get_item(pamh, PAM_RHOST, (const void **)(&rhost)) != PAM_SUCCESS || !rhost)
 	{
-		log_message(LOG_ERR, "Error getting rhost");
+		/* Fall back to an empty string. Does not happen in the sshd service context. */
+		rhost = "";
+	}
+	if (!input_is_safe(rhost, MAX_INPUT_LENGTH))
+	{
+		log_message(LOG_ERR, "Invalid rhost");
 		return PAM_SYSTEM_ERR;
 	}
 
@@ -165,6 +175,13 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, UNUSED int flags, int arg
 		 ++retry)
 	{
 		char *code = tty_input(pamh, PROMPT_CODE, PAM_PROMPT_ECHO_OFF);
+		if (!code || !input_is_safe(code, MAX_INPUT_LENGTH))
+		{
+			tty_output(pamh, "invalid code");
+			log_message(LOG_ERR, "invalid code");
+			free(code);
+			continue;
+		}
 
 		/* Prepare URL... */
 		url = str_printf("%s/%s", cfg->url, API_CHECK_CODE_PATH);

src/pam_weblogin.h

@@ -16,6 +16,8 @@
 #define PROMPT_GROUP "\nSelect group: "
 #define PROMPT_WRONG_NUMBER "Wrong number"
 
+#define MAX_INPUT_LENGTH 256
+
 #ifndef GIT_COMMIT
 #define GIT_COMMIT 0000
 #endif

src/tty.c

@@ -77,6 +77,35 @@ char *tty_input(pam_handle_t *pamh, const char *text, int echo_code)
 	return ret;
 }
 
+bool input_is_safe(const char *input, size_t max_length)
+{
+	size_t length = strnlen(input, max_length);
+	if (input[length] != '\0')
+	{
+		return false;
+	}
+	/* Allow strings that are valid usernames according to POSIX.
+	 * Valid characters are a-z A-Z 0-9 '.' '_' '-' ; the first character must not be '-'.
+	 * See POSIX spec:
+	 * https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_437
+	 * https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_282
+         * Additionally allow ':' so that IPv6 addresses are also considered safe. */
+	for (size_t i = 0; i < length; i++)
+	{
+		/* Don't use isalnum() here because it is locale-dependent,
+		 * and don't use isalnum_l() because it is not portable. */
+		if (!(     (input[i] >= 'a' && input[i] <= 'z')
+			|| (input[i] >= 'A' && input[i] <= 'Z')
+			|| (input[i] >= '0' && input[i] <= '9')
+			|| (input[i] == '.' || input[i] == '_' || input[i] == ':')
+			|| (i > 0 && input[i] == '-')))
+		{
+			return false;
+		}
+	}
+	return true;
+}
+
 void tty_output(pam_handle_t *pamh, const char *text)
 {
 	/* note: on MacOS, pam_message.msg is a non-const char*, so we need to copy it */

src/tty.h

@@ -2,6 +2,8 @@
 #define TTY_H
 
 #include <syslog.h>
+#include <stddef.h>
+#include <stdbool.h>
 
 #include <security/pam_appl.h>
 #include <security/pam_modules.h>
@@ -10,6 +12,7 @@ void log_message(int priority, const char *fmt, ...)
 	__attribute__ ((format (printf, 2, 3)));
 
 char *tty_input(pam_handle_t *pamh, const char *text, int echo_code);
+bool input_is_safe(const char *input, size_t max_length);
 void tty_output(pam_handle_t *pamh, const char *text);
 
 #endif /* TTY_H */

tests/test_utils.c

@@ -8,6 +8,7 @@
 #include <errno.h>
 
 #include "../src/utils.h"
+#include "../src/tty.h"
 
 
 /* helper function to lower RLIMIT_DATA to test out of memory conditions
@@ -273,6 +274,28 @@ START_TEST (test_json_utils_oom)
 }
 END_TEST
 
+START_TEST (test_input_is_safe)
+{
+	ck_assert( input_is_safe("", 0));
+	ck_assert(!input_is_safe("A", 0));
+	ck_assert( input_is_safe("ABC", 8));
+	ck_assert(!input_is_safe("123456789", 8));
+	ck_assert( input_is_safe("AB.C", 8));
+	ck_assert(!input_is_safe("\"ABC\"", 8));
+	ck_assert(!input_is_safe("A\"}BBB", 8));
+	ck_assert(!input_is_safe("1234", 3));
+	ck_assert( input_is_safe("1234", 4));
+	ck_assert( input_is_safe("user-name", 16));
+	ck_assert( input_is_safe("user_name", 16));
+	ck_assert(!input_is_safe("-user-name", 16));
+	ck_assert( input_is_safe("_user_name", 16));
+	ck_assert( input_is_safe("127.0.0.1", 64));
+	ck_assert( input_is_safe("192.168.2.23", 64));
+	ck_assert( input_is_safe("::1", 64));
+	ck_assert( input_is_safe("3fff:fff:ffff:ffff:ffff:ffff:ffff:ffff", 64));
+}
+END_TEST
+
 TCase * test_utils(void)
 {
     TCase *tc =tcase_create("utils");
@@ -291,5 +314,8 @@ TCase * test_utils(void)
     tcase_add_test(tc, test_json_utils);
 	tcase_add_test(tc, test_json_utils_oom);
 
+	/* input_is_safes */
+    tcase_add_test(tc, test_input_is_safe);
+
     return tc;
 }