Review at https://gerrit.osmocom.org/5438
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.
Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1
---
M src/ctrl/control_cmd.c
M tests/ctrl/ctrl_test.c
M tests/ctrl/ctrl_test.ok
M tests/testsuite.at
4 files changed, 144 insertions(+), 121 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/38/5438/1
diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index c2ce2be..f32a200 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -281,6 +281,15 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 7181db3..2a42a2e 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -79,14 +79,19 @@
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);
@@ -141,75 +146,66 @@
{
.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",
{
@@ -219,7 +215,6 @@
.value = "value",
},
"ERROR 1 Command not found",
-
},
{ "SET 1 variable value\n",
{
@@ -229,27 +224,22 @@
.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",
{
@@ -279,7 +269,6 @@
.value = "value_with_trailing_spaces ",
},
"ERROR 1 Command not found",
-
},
{ "SET 1 variable value_with_trailing_spaces \n",
{
@@ -289,27 +278,22 @@
.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",
{
@@ -318,7 +302,6 @@
.variable = "variable",
.reply = "OK",
},
- .reply_str = NULL,
},
{ "SET_REPLY 1 variable OK",
{
@@ -327,7 +310,6 @@
.variable = "variable",
.reply = "OK",
},
- .reply_str = NULL,
},
};
diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok
index 4a3a169..087ebbc 100644
--- a/tests/ctrl/ctrl_test.ok
+++ b/tests/ctrl/ctrl_test.ok
@@ -19,7 +19,7 @@
test: 'GET 1 variable\n'
parsing:
id = '1'
-variable = 'variable\n'
+variable = 'variable'
value = '(null)'
reply = '(null)'
handling:
@@ -28,65 +28,51 @@
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 @@
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 @@
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 4a59b22..81730ee 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -18,7 +18,7 @@
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])
--
To view, visit https://gerrit.osmocom.org/5438
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <[email protected]>
