ctrl: tighten CTRL input parsing

Validate that incoming CTRL commands...
- have decimal IDs,
- return error on trailing characters,
- have invalid characters in variable identifiers,
- send detailed error messages as reply to the requestor.

Adjust ctrl_test.{c,ok}, which best show the change in behavior.

Message handling causes log messages on stderr; previously, stderr was empty.
Add '[ignore]' in testsuite.at so that the nonempty stderr doesn't cause test
failures.

Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1

4 files changed, 144 insertions, 121 deletions

diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index c2ce2be4..f32a200c 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -281,6 +281,15 @@ struct ctrl_cmd *ctrl_cmd_parse(void *ctx, struct msgb *msg)
 	return res;
 }
 
+static bool id_str_valid(const char *str)
+{
+	for (;*str;str++) {
+		if (!isdigit(*str))
+			return false;
+	}
+	return true;
+}
+
 /*! Parse CTRL command struct from msgb, return ctrl->type == CTRL_TYPE_ERROR and an error message in
  * ctrl->reply on any error.
  * The caller is responsible to talloc_free() the returned struct pointer. */
@@ -306,6 +315,7 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 		cmd->type = CTRL_TYPE_ERROR;
 		cmd->id = "err";
 		cmd->reply = "Request malformed";
+		LOGP(DLCTRL, LOGL_NOTICE, "Malformed request: \"%s\"\n", osmo_escape_str(str, -1));
 		goto err;
 	}
 
@@ -314,6 +324,7 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 		cmd->type = CTRL_TYPE_ERROR;
 		cmd->id = "err";
 		cmd->reply = "Request type unknown";
+		LOGP(DLCTRL, LOGL_NOTICE, "Request type unknown: \"%s\"\n", osmo_escape_str(str, -1));
 		goto err;
 	}
 
@@ -323,6 +334,15 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 		cmd->type = CTRL_TYPE_ERROR;
 		cmd->id = "err";
 		cmd->reply = "Missing ID";
+		LOGP(DLCTRL, LOGL_NOTICE, "Missing ID: \"%s\"\n", osmo_escape_str(str, -1));
+		goto err;
+	}
+
+	if (!id_str_valid(tmp)) {
+		cmd->type = CTRL_TYPE_ERROR;
+		cmd->id = "err";
+		cmd->reply = "Invalid message ID number";
+		LOGP(DLCTRL, LOGL_NOTICE, "Invalid message ID number: \"%s\"\n", osmo_escape_str(tmp, -1));
 		goto err;
 	}
 	cmd->id = talloc_strdup(cmd, tmp);
@@ -331,14 +351,30 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 
 	switch (cmd->type) {
 		case CTRL_TYPE_GET:
-			var = strtok_r(NULL, " ", &saveptr);
+			var = strtok_r(NULL, " \n", &saveptr);
 			if (!var) {
 				cmd->type = CTRL_TYPE_ERROR;
 				cmd->reply = "GET incomplete";
-				LOGP(DLCTRL, LOGL_NOTICE, "GET Command incomplete\n");
+				LOGP(DLCTRL, LOGL_NOTICE, "GET Command incomplete: \"%s\"\n",
+				     osmo_escape_str(str, -1));
+				goto err;
+			}
+			if (!osmo_separated_identifiers_valid(var, ".")) {
+				cmd->type = CTRL_TYPE_ERROR;
+				cmd->reply = "GET variable contains invalid characters";
+				LOGP(DLCTRL, LOGL_NOTICE, "GET variable contains invalid characters: \"%s\"\n",
+				     osmo_escape_str(var, -1));
 				goto err;
 			}
 			cmd->variable = talloc_strdup(cmd, var);
+			var = strtok_r(NULL, "", &saveptr);
+			if (var) {
+				cmd->type = CTRL_TYPE_ERROR;
+				cmd->reply = "GET with trailing characters";
+				LOGP(DLCTRL, LOGL_NOTICE, "GET with trailing characters: \"%s\"\n",
+				     osmo_escape_str(var, -1));
+				goto err;
+			}
 			LOGP(DLCTRL, LOGL_DEBUG, "Command: GET %s\n", cmd->variable);
 			break;
 		case CTRL_TYPE_SET:
@@ -350,31 +386,57 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 				LOGP(DLCTRL, LOGL_NOTICE, "SET Command incomplete\n");
 				goto err;
 			}
+			if (!osmo_separated_identifiers_valid(var, ".")) {
+				cmd->type = CTRL_TYPE_ERROR;
+				cmd->reply = "SET variable contains invalid characters";
+				LOGP(DLCTRL, LOGL_NOTICE, "SET variable contains invalid characters: \"%s\"\n",
+				     osmo_escape_str(var, -1));
+				goto err;
+			}
 			cmd->variable = talloc_strdup(cmd, var);
 			cmd->value = talloc_strdup(cmd, val);
 			if (!cmd->variable || !cmd->value)
 				goto oom;
-			LOGP(DLCTRL, LOGL_DEBUG, "Command: SET %s = %s\n", cmd->variable, cmd->value);
+
+			var = strtok_r(NULL, "", &saveptr);
+			if (var) {
+				cmd->type = CTRL_TYPE_ERROR;
+				cmd->reply = "SET with trailing characters";
+				LOGP(DLCTRL, LOGL_NOTICE, "SET with trailing characters: \"%s\"\n",
+				     osmo_escape_str(var, -1));
+				goto err;
+			}
+
+			LOGP(DLCTRL, LOGL_DEBUG, "Command: SET %s = \"%s\"\n", cmd->variable,
+			     osmo_escape_str(cmd->value, -1));
 			break;
 		case CTRL_TYPE_GET_REPLY:
 		case CTRL_TYPE_SET_REPLY:
 		case CTRL_TYPE_TRAP:
 			var = strtok_r(NULL, " ", &saveptr);
-			val = strtok_r(NULL, " ", &saveptr);
+			val = strtok_r(NULL, "", &saveptr);
 			if (!var || !val) {
 				cmd->type = CTRL_TYPE_ERROR;
 				cmd->reply = "Trap/Reply incomplete";
 				LOGP(DLCTRL, LOGL_NOTICE, "Trap/Reply incomplete\n");
 				goto err;
 			}
+			if (!osmo_separated_identifiers_valid(var, ".")) {
+				cmd->type = CTRL_TYPE_ERROR;
+				cmd->reply = "Trap/Reply variable contains invalid characters";
+				LOGP(DLCTRL, LOGL_NOTICE, "Trap/Reply variable contains invalid characters: \"%s\"\n",
+				     osmo_escape_str(var, -1));
+				goto err;
+			}
 			cmd->variable = talloc_strdup(cmd, var);
 			cmd->reply = talloc_strdup(cmd, val);
 			if (!cmd->variable || !cmd->reply)
 				goto oom;
-			LOGP(DLCTRL, LOGL_DEBUG, "Command: TRAP/REPLY %s: %s\n", cmd->variable, cmd->reply);
+			LOGP(DLCTRL, LOGL_DEBUG, "Command: TRAP/REPLY %s: \"%s\"\n", cmd->variable,
+			     osmo_escape_str(cmd->reply, -1));
 			break;
 		case CTRL_TYPE_ERROR:
-			var = strtok_r(NULL, "\0", &saveptr);
+			var = strtok_r(NULL, "", &saveptr);
 			if (!var) {
 				cmd->reply = "";
 				goto err;
@@ -382,7 +444,8 @@ struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg)
 			cmd->reply = talloc_strdup(cmd, var);
 			if (!cmd->reply)
 				goto oom;
-			LOGP(DLCTRL, LOGL_DEBUG, "Command: ERROR %s\n", cmd->reply);
+			LOGP(DLCTRL, LOGL_DEBUG, "Command: ERROR \"%s\"\n",
+			     osmo_escape_str(cmd->reply, -1));
 			break;
 		case CTRL_TYPE_UNKNOWN:
 		default:
diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c
index 39ec61a8..a38591f3 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -79,14 +79,19 @@ static void assert_test(struct ctrl_handle *ctrl, struct ctrl_connection *ccon,
 	cmd = ctrl_cmd_parse2(ctx, msg);
 	OSMO_ASSERT(cmd);
 
-	OSMO_ASSERT(t->expect_parsed.type == cmd->type);
+	if (t->expect_parsed.type != cmd->type) {
+		printf("type mismatch: got %s\n", get_value_string(ctrl_type_vals, cmd->type));
+		OSMO_ASSERT(t->expect_parsed.type == cmd->type);
+	}
 
 #define ASSERT_SAME_STR(field) \
 	assert_same_str(#field, t->expect_parsed.field, cmd->field)
 
 	ASSERT_SAME_STR(id);
-	ASSERT_SAME_STR(variable);
-	ASSERT_SAME_STR(value);
+	if (t->expect_parsed.type != CTRL_TYPE_ERROR) {
+		ASSERT_SAME_STR(variable);
+		ASSERT_SAME_STR(value);
+	}
 	ASSERT_SAME_STR(reply);
 
 	talloc_free(cmd);
@@ -140,75 +145,66 @@ static const struct one_test test_messages_list[] = {
 		{
 			.type = CTRL_TYPE_GET,
 			.id = "1",
-			.variable = "variable\n", /* current bug */
+			.variable = "variable",
 		},
 		"ERROR 1 Command not found",
 
 	},
 	{ "GET 1 var\ni\nable",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "var\ni\nable", /* current bug */
+			.reply = "GET with trailing characters",
 		},
-		"ERROR 1 Command not found",
-
+		"ERROR 1 GET with trailing characters",
 	},
 	{ "GET 1 var\ti\table",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "var\ti\table", /* current bug */
+			.reply = "GET variable contains invalid characters",
 		},
-		"ERROR 1 Command not found",
+		"ERROR 1 GET variable contains invalid characters",
 	},
 	{ "GET 1 var\ri\rable",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "var\ri\rable", /* current bug */
+			.reply = "GET variable contains invalid characters",
 		},
-		"ERROR 1 Command not found",
+		"ERROR 1 GET variable contains invalid characters",
 	},
 	{ "GET 1 variable value",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "variable",
-			.value = NULL,
+			.reply = "GET with trailing characters",
 		},
-		"ERROR 1 Command not found",
-
+		"ERROR 1 GET with trailing characters",
 	},
 	{ "GET 1 variable value\n",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "variable",
-			.value = NULL,
+			.reply = "GET with trailing characters",
 		},
-		"ERROR 1 Command not found",
-
+		"ERROR 1 GET with trailing characters",
 	},
 	{ "GET 1 variable multiple value tokens",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "variable",
-			.value = NULL,
+			.reply = "GET with trailing characters",
 		},
-		"ERROR 1 Command not found",
-
+		"ERROR 1 GET with trailing characters",
 	},
 	{ "GET 1 variable multiple value tokens\n",
 		{
-			.type = CTRL_TYPE_GET,
+			.type = CTRL_TYPE_ERROR,
 			.id = "1",
-			.variable = "variable",
-			.value = NULL,
+			.reply = "GET with trailing characters",
 		},
-		"ERROR 1 Command not found",
-
+		"ERROR 1 GET with trailing characters",
 	},
 	{ "SET 1 variable value",
 		{
@@ -218,7 +214,6 @@ static const struct one_test test_messages_list[] = {
 			.value = "value",
 		},
 		"ERROR 1 Command not found",
-
 	},
 	{ "SET 1 variable value\n",
 		{
@@ -228,27 +223,22 @@ static const struct one_test test_messages_list[] = {
 			.value = "value",
 		},
 		"ERROR 1 Command not found",
-
 	},
 	{ "SET weird_id variable value",
 		{
-			.type = CTRL_TYPE_SET,
-			.id = "weird_id",
-			.variable = "variable",
-			.value = "value",
+			.type = CTRL_TYPE_ERROR,
+			.id = "err",
+			.reply = "Invalid message ID number",
 		},
-		"ERROR weird_id Command not found",
-
+		"ERROR err Invalid message ID number",
 	},
 	{ "SET weird_id variable value\n",
 		{
-			.type = CTRL_TYPE_SET,
-			.id = "weird_id",
-			.variable = "variable",
-			.value = "value",
+			.type = CTRL_TYPE_ERROR,
+			.id = "err",
+			.reply = "Invalid message ID number",
 		},
-		"ERROR weird_id Command not found",
-
+		"ERROR err Invalid message ID number",
 	},
 	{ "SET 1 variable multiple value tokens",
 		{
@@ -278,7 +268,6 @@ static const struct one_test test_messages_list[] = {
 			.value = "value_with_trailing_spaces  ",
 		},
 		"ERROR 1 Command not found",
-
 	},
 	{ "SET 1 variable value_with_trailing_spaces  \n",
 		{
@@ -288,27 +277,22 @@ static const struct one_test test_messages_list[] = {
 			.value = "value_with_trailing_spaces  ",
 		},
 		"ERROR 1 Command not found",
-
 	},
 	{ "SET \n special_char_id value",
 		{
-			.type = CTRL_TYPE_SET,
-			.id = "\n",
-			.variable = "special_char_id",
-			.value = "value",
+			.type = CTRL_TYPE_ERROR,
+			.id = "err",
+			.reply = "Invalid message ID number",
 		},
-		"ERROR \n Command not found",
-
+		"ERROR err Invalid message ID number",
 	},
 	{ "SET \t special_char_id value",
 		{
-			.type = CTRL_TYPE_SET,
-			.id = "\t",
-			.variable = "special_char_id",
-			.value = "value",
+			.type = CTRL_TYPE_ERROR,
+			.id = "err",
+			.reply = "Invalid message ID number",
 		},
-		"ERROR \t Command not found",
-
+		"ERROR err Invalid message ID number",
 	},
 	{ "GET_REPLY 1 variable OK",
 		{
@@ -317,7 +301,6 @@ static const struct one_test test_messages_list[] = {
 			.variable = "variable",
 			.reply = "OK",
 		},
-		.reply_str = NULL,
 	},
 	{ "SET_REPLY 1 variable OK",
 		{
@@ -326,7 +309,6 @@ static const struct one_test test_messages_list[] = {
 			.variable = "variable",
 			.reply = "OK",
 		},
-		.reply_str = NULL,
 	},
 
 };
diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok
index 4a3a1696..087ebbc2 100644
--- a/tests/ctrl/ctrl_test.ok
+++ b/tests/ctrl/ctrl_test.ok
@@ -19,7 +19,7 @@ ok
 test: 'GET 1 variable\n'
 parsing:
 id = '1'
-variable = 'variable\n'
+variable = 'variable'
 value = '(null)'
 reply = '(null)'
 handling:
@@ -28,65 +28,51 @@ ok
 test: 'GET 1 var\ni\nable'
 parsing:
 id = '1'
-variable = 'var\ni\nable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET with trailing characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET with trailing characters'
 ok
 test: 'GET 1 var\ti\table'
 parsing:
 id = '1'
-variable = 'var\ti\table'
-value = '(null)'
-reply = '(null)'
+reply = 'GET variable contains invalid characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET variable contains invalid characters'
 ok
 test: 'GET 1 var\ri\rable'
 parsing:
 id = '1'
-variable = 'var\ri\rable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET variable contains invalid characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET variable contains invalid characters'
 ok
 test: 'GET 1 variable value'
 parsing:
 id = '1'
-variable = 'variable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET with trailing characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET with trailing characters'
 ok
 test: 'GET 1 variable value\n'
 parsing:
 id = '1'
-variable = 'variable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET with trailing characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET with trailing characters'
 ok
 test: 'GET 1 variable multiple value tokens'
 parsing:
 id = '1'
-variable = 'variable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET with trailing characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET with trailing characters'
 ok
 test: 'GET 1 variable multiple value tokens\n'
 parsing:
 id = '1'
-variable = 'variable'
-value = '(null)'
-reply = '(null)'
+reply = 'GET with trailing characters'
 handling:
-replied: 'ERROR 1 Command not found'
+replied: 'ERROR 1 GET with trailing characters'
 ok
 test: 'SET 1 variable value'
 parsing:
@@ -108,21 +94,17 @@ replied: 'ERROR 1 Command not found'
 ok
 test: 'SET weird_id variable value'
 parsing:
-id = 'weird_id'
-variable = 'variable'
-value = 'value'
-reply = '(null)'
+id = 'err'
+reply = 'Invalid message ID number'
 handling:
-replied: 'ERROR weird_id Command not found'
+replied: 'ERROR err Invalid message ID number'
 ok
 test: 'SET weird_id variable value\n'
 parsing:
-id = 'weird_id'
-variable = 'variable'
-value = 'value'
-reply = '(null)'
+id = 'err'
+reply = 'Invalid message ID number'
 handling:
-replied: 'ERROR weird_id Command not found'
+replied: 'ERROR err Invalid message ID number'
 ok
 test: 'SET 1 variable multiple value tokens'
 parsing:
@@ -162,21 +144,17 @@ replied: 'ERROR 1 Command not found'
 ok
 test: 'SET \n special_char_id value'
 parsing:
-id = '\n'
-variable = 'special_char_id'
-value = 'value'
-reply = '(null)'
+id = 'err'
+reply = 'Invalid message ID number'
 handling:
-replied: 'ERROR \n Command not found'
+replied: 'ERROR err Invalid message ID number'
 ok
 test: 'SET \t special_char_id value'
 parsing:
-id = '\t'
-variable = 'special_char_id'
-value = 'value'
-reply = '(null)'
+id = 'err'
+reply = 'Invalid message ID number'
 handling:
-replied: 'ERROR \t Command not found'
+replied: 'ERROR err Invalid message ID number'
 ok
 test: 'GET_REPLY 1 variable OK'
 parsing:
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 4a59b221..81730ee2 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -18,7 +18,7 @@ AT_CLEANUP
 AT_SETUP([ctrl])
 AT_KEYWORDS([ctrl])
 cat $abs_srcdir/ctrl/ctrl_test.ok > expout
-AT_CHECK([$abs_top_builddir/tests/ctrl/ctrl_test], [0], [expout])
+AT_CHECK([$abs_top_builddir/tests/ctrl/ctrl_test], [0], [expout], [ignore])
 AT_CLEANUP
 
 AT_SETUP([kasumi])