- Rework pubkey options to be more careful about buffer lengths. Needs review.

--HG--
branch : pubkey-options
extra : convert_revision : 537a6ebebb46424b967ffe787f0f8560e5f447e8

auth.h

@@ -46,7 +46,7 @@ int svr_pubkey_allows_x11fwd();
 int svr_pubkey_allows_pty();
 void svr_pubkey_set_forced_command(struct ChanSess *chansess);
 void svr_pubkey_options_cleanup();
-int svr_add_pubkey_options(const char* opts);
+int svr_add_pubkey_options(buffer *options_buf, int line_num, const char* filename);
 #else
 /* no option : success */
 #define svr_pubkey_allows_agentfwd() 1
@@ -55,7 +55,7 @@ int svr_add_pubkey_options(const char* opts);
 #define svr_pubkey_allows_pty() 1
 static inline void svr_pubkey_set_forced_command(struct ChanSess *chansess) { }
 static inline void svr_pubkey_options_cleanup() { }
-#define svr_add_pubkey_options(x) DROPBEAR_SUCCESS
+#define svr_add_pubkey_options(x,y,z) DROPBEAR_SUCCESS
 #endif
 
 /* Client functions */

debug.h

@@ -67,6 +67,11 @@
 #define TRACE(X)
 #endif /*DEBUG_TRACE*/
 
+/* To debug with GDB it is easier to run with no forking of child processes.
+   You will need to pass "-F" as well. */
+/* #define DEBUG_NOFORK */
+
+
 /* For testing as non-root on shadowed systems, include the crypt of a password
  * here. You can then log in as any user with this password. Ensure that you
  * make your own password, and are careful about using this. This will also

svr-authpubkey.c

@@ -189,8 +189,9 @@ static int checkpubkey(unsigned char* algo, unsigned int algolen,
 	char * filename = NULL;
 	int ret = DROPBEAR_FAILURE;
 	buffer * line = NULL;
-	unsigned int len, pos, quoted;
-	const char *options = NULL;
+	unsigned int len, pos;
+	buffer * options_buf = NULL;
+	int line_num;
 
 	TRACE(("enter checkpubkey"))
 
@@ -225,17 +226,22 @@ static int checkpubkey(unsigned char* algo, unsigned int algolen,
 	TRACE(("checkpubkey: opened authorized_keys OK"))
 
 	line = buf_new(MAX_AUTHKEYS_LINE);
+	line_num = 0;
 
 	/* iterate through the lines */
 	do {
 		/* new line : potentially new options */
-		options = NULL;
+		if (options_buf) {
+			buf_free(options_buf);
+			options_buf = NULL;
+		}
 
 		if (buf_getline(line, authfile) == DROPBEAR_FAILURE) {
 			/* EOF reached */
 			TRACE(("checkpubkey: authorized_keys EOF reached"))
 			break;
 		}
+		line_num++;
 
 		if (line->len < MIN_AUTHKEYS_LINE) {
 			TRACE(("checkpubkey: line too short"))
@@ -243,38 +249,59 @@ static int checkpubkey(unsigned char* algo, unsigned int algolen,
 		}
 
 		/* check the key type - will fail if there are options */
-		if (strncmp(buf_getptr(line, algolen), algo, algolen) != 0) {
-			/* there may be options or a commented line */
-			if ('#' == line->data[line->pos]) continue;
-			/* no comment, skip to next space character */
-			len = 0;
-			pos = line->pos;
-			options = buf_getptr(line, 1);
-			quoted = 0;
-			while (line->data[pos] 
-				&& (quoted || (line->data[pos] != ' '
-						&& line->data[pos] != '\t'
-						&& line->data[pos] != '\n'
-						&& line->data[pos] != '\r'))) { 
-				pos++;
-				if (line->data[pos] == '\\' 
-					&& line->data[pos+1] == '"') { 
-					pos++;	/* skip both */ 
-				} else if (line->data[pos] == '"') 
-					quoted = !quoted; 
-			} /* line->data[pos] == ['\0'|' '|'\t'] */
+		TRACE(("a line!"))
 
-			/* skip line if there is nothing left */
-			if (pos >= line->len) continue;
-			/* skip line if it begins with a space or tab character */
-			if (pos == line->pos) continue;
-			/* set the position of the line after what we have read */
-			buf_setpos(line, pos+1);
-			/* give a second chance to the algo */
-			if (line->pos + algolen > line->len) continue;
+		if (strncmp(buf_getptr(line, algolen), algo, algolen) != 0) {
+			int is_comment = 0;
+			char *options_start = NULL;
+			int options_len = 0;
+			int escape, quoted;
+			
+			/* skip over any comments or leading whitespace */
+			while (line->pos < line->len) {
+				const char c = buf_getbyte(line);
+				if (c == ' ' || c == '\t') {
+					continue;
+				} else if (c == '#') {
+					is_comment = 1;
+					break;
+				}
+				buf_incrpos(line, -1);
+				break;
+			}
+			if (is_comment) {
+				/* next line */
+				continue;
+			}
+
+			/* remember start of options */
+			options_start = buf_getptr(line, 1);
+			quoted = 0;
+			escape = 0;
+			options_len = 0;
+			
+			/* figure out where the options are */
+			while (line->pos < line->len) {
+				const char c = buf_getbyte(line);
+				if (!quoted && (c == ' ' || c == '\t')) {
+					break;
+				}
+				escape = (!escape && c == '\\');
+				if (!escape && c == '"') {
+					quoted = !quoted;
+				}
+				options_len++;
+			}
+			options_buf = buf_new(options_len);
+			buf_putbytes(options_buf, options_start, options_len);
+
+			/* compare the algorithm */
+			if (line->pos + algolen > line->len) {
+				continue;
+			}
 			if (strncmp(buf_getptr(line, algolen), algo, algolen) != 0) { 
 				continue;
-			 }
+			}
 		}
 		buf_incrpos(line, algolen);
 		
@@ -296,8 +323,8 @@ static int checkpubkey(unsigned char* algo, unsigned int algolen,
 
 		ret = cmp_base64_key(keyblob, keybloblen, algo, algolen, line, NULL);
 
-		if (ret == DROPBEAR_SUCCESS) {
-			ret = svr_add_pubkey_options(options);
+		if (ret == DROPBEAR_SUCCESS && options_buf) {
+			ret = svr_add_pubkey_options(options_buf, line_num, filename);
 		}
 
 		if (ret == DROPBEAR_SUCCESS) {
@@ -316,6 +343,9 @@ out:
 		buf_free(line);
 	}
 	m_free(filename);
+	if (options_buf) {
+		buf_free(options_buf);
+	}
 	TRACE(("leave checkpubkey: ret=%d", ret))
 	return ret;
 }

svr-authpubkeyoptions.c

@@ -102,106 +102,86 @@ void svr_pubkey_options_cleanup() {
 	}
 }
 
+/* helper for svr_add_pubkey_options. returns DROPBEAR_SUCCESS if the option is matched,
+   and increments the options_buf */
+static int match_option(buffer *options_buf, const char *opt_name) {
+	const int len = strlen(opt_name);
+	if (options_buf->len - options_buf->pos < len) {
+		return DROPBEAR_FAILURE;
+	}
+	if (strncasecmp(buf_getptr(options_buf, len), opt_name, len) == 0) {
+		buf_incrpos(options_buf, len);
+		return DROPBEAR_SUCCESS;
+	}
+	return DROPBEAR_FAILURE;
+}
+
 /* Parse pubkey options and set ses.authstate.pubkey_options accordingly.
  * Returns DROPBEAR_SUCCESS if key is ok for auth, DROPBEAR_FAILURE otherwise */
-int svr_add_pubkey_options(const char* opts) {
-	const char *cp;
-	int i;
+int svr_add_pubkey_options(buffer *options_buf, int line_num, const char* filename) {
 	int ret = DROPBEAR_FAILURE;
 
 	TRACE(("enter addpubkeyoptions"))
 
-	if (!opts || *opts == ' ') {
-		/* no option, success */
-		ret = DROPBEAR_SUCCESS;
-		goto end;
-	}
-	
 	ses.authstate.pubkey_options = (struct PubKeyOptions*)m_malloc(sizeof( struct PubKeyOptions ));
+	memset(ses.authstate.pubkey_options, '\0', sizeof(*ses.authstate.pubkey_options));
 
-	while (*opts && *opts != ' ' && *opts != '\t') {
-		cp = "no-port-forwarding";
-		if (strncasecmp(opts, cp, strlen(cp)) == 0) {
+	buf_setpos(options_buf, 0);
+	while (options_buf->pos < options_buf->len) {
+		if (match_option(options_buf, "no-port-forwarding") == DROPBEAR_SUCCESS) {
 			dropbear_log(LOG_WARNING, "Port forwarding disabled.");
 			ses.authstate.pubkey_options->no_port_forwarding_flag = 1;
-			opts += strlen(cp);
 			goto next_option;
 		}
 #ifdef ENABLE_AGENTFWD
-		cp = "no-agent-forwarding";
-		if (strncasecmp(opts, cp, strlen(cp)) == 0) {
+		if (match_option(options_buf, "no-agent-forwarding") == DROPBEAR_SUCCESS) {
 			dropbear_log(LOG_WARNING, "Agent forwarding disabled.");
 			ses.authstate.pubkey_options->no_agent_forwarding_flag = 1;
-			opts += strlen(cp);
 			goto next_option;
 		}
 #endif
 #ifdef ENABLE_X11FWD
-		cp = "no-X11-forwarding";
-		if (strncasecmp(opts, cp, strlen(cp)) == 0) {
+		if (match_option(options_buf, "no-X11-forwarding") == DROPBEAR_SUCCESS) {
 			dropbear_log(LOG_WARNING, "X11 forwarding disabled.");
 			ses.authstate.pubkey_options->no_x11_forwarding_flag = 1;
-			opts += strlen(cp);
 			goto next_option;
 		}
 #endif
-		cp = "no-pty";
-		if (strncasecmp(opts, cp, strlen(cp)) == 0) {
+		if (match_option(options_buf, "no-pty") == DROPBEAR_SUCCESS) {
 			dropbear_log(LOG_WARNING, "Pty allocation disabled.");
 			ses.authstate.pubkey_options->no_pty_flag = 1;
-			opts += strlen(cp);
 			goto next_option;
 		}
-		cp = "command=\"";
-		if (strncasecmp(opts, cp, strlen(cp)) == 0) {
-			opts += strlen(cp);
-			ses.authstate.pubkey_options->forced_command = (char*)m_malloc(strlen(opts) + 1);
-			i = 0;
-			while (*opts) {
-				if (*opts == '"')
-					break;
-				if (*opts == '\\' && opts[1] == '"') {
-					opts += 2;
-					ses.authstate.pubkey_options->forced_command[i++] = '"';
-					continue;
+		if (match_option(options_buf, "command=\"") == DROPBEAR_SUCCESS) {
+			int escaped = 0;
+			const unsigned char* command_start = buf_getptr(options_buf, 0);
+			while (options_buf->pos < options_buf->len) {
+				const char c = buf_getbyte(options_buf);
+				if (!escaped && c == '"') {
+					const int command_len = buf_getptr(options_buf, 0) - command_start;
+					ses.authstate.pubkey_options->forced_command = m_malloc(command_len);
+					memcpy(ses.authstate.pubkey_options->forced_command,
+							command_start, command_len-1);
+					ses.authstate.pubkey_options->forced_command[command_len-1] = '\0';
+					dropbear_log(LOG_WARNING, "Forced command '%s'", 
+						ses.authstate.pubkey_options->forced_command);
+					goto next_option;
 				}
-				ses.authstate.pubkey_options->forced_command[i++] = *opts++;
+				escaped = (!escaped && c == '\\');
 			}
-			if (!*opts) {
-				dropbear_log(LOG_WARNING, 
-						"Missing end quote in public key command option");
-				m_free(ses.authstate.pubkey_options->forced_command);
-				ses.authstate.pubkey_options->forced_command = NULL;
-				goto bad_option;
-			}
-			ses.authstate.pubkey_options->forced_command[i] = '\0';
-			if (strlen(ses.authstate.pubkey_options->forced_command) > MAX_CMD_LEN) {
-				dropbear_log(LOG_WARNING, 
-						"Public key option command too long (>MAX_CMD_LEN).");
-				m_free(ses.authstate.pubkey_options->forced_command);
-				ses.authstate.pubkey_options->forced_command = NULL;
-				goto bad_option;
-			}
-			dropbear_log(LOG_WARNING, "Forced command '%s'", 
-				ses.authstate.pubkey_options->forced_command);
-			opts++;
-			goto next_option;
+			dropbear_log(LOG_WARNING, "Badly formatted command= authorized_keys option");
+			goto bad_option;
 		}
-		next_option:
+
+next_option:
 		/*
 		 * Skip the comma, and move to the next option
 		 * (or break out if there are no more).
 		 */
-		if (!*opts) {
-			TRACE(("Bugs in svr-chansession.c pubkey option processing."))
-		}
-		if (*opts == ' ' || *opts == '\t') {
-			break;		/* End of options. */
-		}
-		if (*opts != ',') {
+		if (options_buf->pos < options_buf->len 
+				&& buf_getbyte(options_buf) != ',') {
 			goto bad_option;
 		}
-		opts++;
 		/* Process the next option. */
 	}
 	/* parsed all options with no problem */
@@ -212,12 +192,11 @@ bad_option:
 	ret = DROPBEAR_FAILURE;
 	m_free(ses.authstate.pubkey_options);
 	ses.authstate.pubkey_options = NULL;
-	dropbear_log(LOG_WARNING, "Bad public key options : '%.50s'", opts);
+	dropbear_log(LOG_WARNING, "Bad public key options at %s:%d", filename, line_num);
 
 end:
 	TRACE(("leave addpubkeyoptions"))
 	return ret;
-
 }
 
 #endif

svr-main.c

@@ -266,7 +266,11 @@ void main_noinetd() {
 				goto out;
 			}
 
+#ifdef DEBUG_NOFORK
+			fork_ret = 0;
+#else
 			fork_ret = fork();
+#endif
 			if (fork_ret < 0) {
 				dropbear_log(LOG_WARNING, "error forking: %s", strerror(errno));
 				goto out;
@@ -292,9 +296,11 @@ void main_noinetd() {
 				addrstring = getaddrstring(&remoteaddr, 1);
 				dropbear_log(LOG_INFO, "Child connection from %s", addrstring);
 
+#ifndef DEBUG_NOFORK
 				if (setsid() < 0) {
 					dropbear_exit("setsid: %s", strerror(errno));
 				}
+#endif
 
 				/* make sure we close sockets */
 				for (i = 0; i < listensockcount; i++) {