From: Martin Willi <martin@strongswan.org>
Date: Mon, 14 May 2018 11:42:53 +0000 (+0200)
Subject: ipsec-types: Restrict the use of %unique and other keywords when parsing marks
X-Git-Tag: 5.7.0rc1~28^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ebd2d3877e146ad128a06dcec9e8aa3ea85adffd;p=thirdparty%2Fstrongswan.git

ipsec-types: Restrict the use of %unique and other keywords when parsing marks

%unique (and the upcoming %same key) are usable in specific contexts only.
To restrict the user from using it in other places where it does not get the
expected results, reject such keywords unless explicitly allowed.
---

diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c
index 3ef3dc712f..c9a76ba01a 100644
--- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c
+++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c
@@ -2925,7 +2925,7 @@ static status_t manage_rule(private_kernel_netlink_net_t *this, int nlmsg_type,
 			msg->rtm_flags |= FIB_RULE_INVERT;
 			fwmark++;
 		}
-		if (mark_from_string(fwmark, &mark))
+		if (mark_from_string(fwmark, MARK_OP_NONE, &mark))
 		{
 			chunk = chunk_from_thing(mark.value);
 			netlink_add_attribute(hdr, FRA_FWMARK, chunk, sizeof(request));
diff --git a/src/libcharon/plugins/socket_default/socket_default_socket.c b/src/libcharon/plugins/socket_default/socket_default_socket.c
index 57e092968b..68e5a7a0e9 100644
--- a/src/libcharon/plugins/socket_default/socket_default_socket.c
+++ b/src/libcharon/plugins/socket_default/socket_default_socket.c
@@ -745,7 +745,7 @@ static int open_socket(private_socket_default_socket_t *this,
 
 		fwmark = lib->settings->get_str(lib->settings,
 							"%s.plugins.socket-default.fwmark", NULL, lib->ns);
-		if (fwmark && mark_from_string(fwmark, &mark))
+		if (fwmark && mark_from_string(fwmark, MARK_OP_NONE, &mark))
 		{
 			if (setsockopt(skt, SOL_SOCKET, SO_MARK, &mark.value,
 						   sizeof(mark.value)) < 0)
diff --git a/src/libcharon/plugins/vici/vici_config.c b/src/libcharon/plugins/vici/vici_config.c
index 05fa8c5ca9..98f68264c2 100644
--- a/src/libcharon/plugins/vici/vici_config.c
+++ b/src/libcharon/plugins/vici/vici_config.c
@@ -1181,7 +1181,7 @@ CALLBACK(parse_mark, bool,
 	{
 		return FALSE;
 	}
-	return mark_from_string(buf, out);
+	return mark_from_string(buf, MARK_OP_UNIQUE, out);
 }
 
 /**
diff --git a/src/libstrongswan/ipsec/ipsec_types.c b/src/libstrongswan/ipsec/ipsec_types.c
index d231bb3a45..7eed1566a8 100644
--- a/src/libstrongswan/ipsec/ipsec_types.c
+++ b/src/libstrongswan/ipsec/ipsec_types.c
@@ -69,7 +69,7 @@ bool ipsec_sa_cfg_equals(ipsec_sa_cfg_t *a, ipsec_sa_cfg_t *b)
 /*
  * See header
  */
-bool mark_from_string(const char *value, mark_t *mark)
+bool mark_from_string(const char *value, mark_op_t ops, mark_t *mark)
 {
 	char *endptr;
 
@@ -79,6 +79,11 @@ bool mark_from_string(const char *value, mark_t *mark)
 	}
 	if (strcasepfx(value, "%unique"))
 	{
+		if (!(ops & MARK_OP_UNIQUE))
+		{
+			DBG1(DBG_APP, "unexpected use of %%unique mark", value);
+			return FALSE;
+		}
 		endptr = (char*)value + strlen("%unique");
 		if (strcasepfx(endptr, "-dir"))
 		{
diff --git a/src/libstrongswan/ipsec/ipsec_types.h b/src/libstrongswan/ipsec/ipsec_types.h
index bd5545e221..5b1735178a 100644
--- a/src/libstrongswan/ipsec/ipsec_types.h
+++ b/src/libstrongswan/ipsec/ipsec_types.h
@@ -28,6 +28,7 @@ typedef enum policy_priority_t policy_priority_t;
 typedef enum ipcomp_transform_t ipcomp_transform_t;
 typedef enum hw_offload_t hw_offload_t;
 typedef enum dscp_copy_t dscp_copy_t;
+typedef enum mark_op_t mark_op_t;
 typedef struct ipsec_sa_cfg_t ipsec_sa_cfg_t;
 typedef struct lifetime_cfg_t lifetime_cfg_t;
 typedef struct mark_t mark_t;
@@ -216,13 +217,24 @@ struct mark_t {
 #define MARK_UNIQUE_DIR (0xFFFFFFFE)
 #define MARK_IS_UNIQUE(m) ((m) == MARK_UNIQUE || (m) == MARK_UNIQUE_DIR)
 
+/**
+ * Special mark operations to accept when parsing marks.
+ */
+enum mark_op_t {
+	/** none of the following */
+	MARK_OP_NONE = 0,
+	/** %unique and %unique-dir */
+	MARK_OP_UNIQUE = (1<<0),
+};
+
 /**
  * Try to parse a mark_t from the given string of the form mark[/mask].
  *
  * @param value		string to parse
+ * @param ops		operations to accept
  * @param mark		mark to fill
  * @return			TRUE if parsing was successful
  */
-bool mark_from_string(const char *value, mark_t *mark);
+bool mark_from_string(const char *value, mark_op_t ops, mark_t *mark);
 
 #endif /** IPSEC_TYPES_H_ @}*/
diff --git a/src/libstrongswan/tests/suites/test_utils.c b/src/libstrongswan/tests/suites/test_utils.c
index 00f000a6a0..5a854f3f67 100644
--- a/src/libstrongswan/tests/suites/test_utils.c
+++ b/src/libstrongswan/tests/suites/test_utils.c
@@ -860,47 +860,69 @@ END_TEST
 static struct {
 	char *s;
 	bool ok;
+	mark_op_t ops;
 	mark_t m;
 } mark_data[] = {
-	{NULL,			FALSE, { 0 }},
-	{"",			TRUE,  { 0, 0xffffffff }},
-	{"/",			TRUE,  { 0, 0 }},
-	{"42",			TRUE,  { 42, 0xffffffff }},
-	{"0x42",		TRUE,  { 0x42, 0xffffffff }},
-	{"x",			FALSE, { 0 }},
-	{"42/",			TRUE,  { 0, 0 }},
-	{"42/0",		TRUE,  { 0, 0 }},
-	{"42/x",		FALSE, { 0 }},
-	{"42/42",		TRUE,  { 42, 42 }},
-	{"42/0xff",		TRUE,  { 42, 0xff }},
-	{"0x42/0xff",	TRUE,  { 0x42, 0xff }},
-	{"/0xff",		TRUE,  { 0, 0xff }},
-	{"/x",			FALSE, { 0 }},
-	{"x/x",			FALSE, { 0 }},
-	{"0xfffffff0/0x0000ffff",	TRUE,  { 0x0000fff0, 0x0000ffff }},
-	{"%unique",					TRUE,  { MARK_UNIQUE, 0xffffffff }},
-	{"%unique/",				TRUE,  { MARK_UNIQUE, 0 }},
-	{"%unique/0x0000ffff",		TRUE,  { MARK_UNIQUE, 0x0000ffff }},
-	{"%unique/0xffffffff",		TRUE,  { MARK_UNIQUE, 0xffffffff }},
-	{"%unique0xffffffffff",		FALSE, { 0, 0 }},
-	{"0xffffffff/0x0000ffff",	TRUE,  { MARK_UNIQUE, 0x0000ffff }},
-	{"0xffffffff/0xffffffff",	TRUE,  { MARK_UNIQUE, 0xffffffff }},
-	{"%unique-dir",				TRUE,  { MARK_UNIQUE_DIR, 0xffffffff }},
-	{"%unique-dir/",			TRUE,  { MARK_UNIQUE_DIR, 0 }},
-	{"%unique-dir/0x0000ffff",	TRUE,  { MARK_UNIQUE_DIR, 0x0000ffff }},
-	{"%unique-dir/0xffffffff",	TRUE,  { MARK_UNIQUE_DIR, 0xffffffff }},
-	{"%unique-dir0xffffffff",	FALSE, { 0, 0 }},
-	{"0xfffffffe/0x0000ffff",	TRUE,  { MARK_UNIQUE_DIR, 0x0000ffff }},
-	{"0xfffffffe/0xffffffff",	TRUE,  { MARK_UNIQUE_DIR, 0xffffffff }},
-	{"%unique-/0xffffffff",		FALSE, { 0, 0 }},
-	{"%unique-foo/0xffffffff",	FALSE, { 0, 0 }},
+	{NULL,			FALSE,	MARK_OP_NONE, { 0 }},
+	{"",			TRUE,	MARK_OP_NONE, { 0, 0xffffffff }},
+	{"/",			TRUE,	MARK_OP_NONE, { 0, 0 }},
+	{"42",			TRUE,	MARK_OP_NONE, { 42, 0xffffffff }},
+	{"0x42",		TRUE,	MARK_OP_NONE, { 0x42, 0xffffffff }},
+	{"x",			FALSE,	MARK_OP_NONE, { 0 }},
+	{"42/",			TRUE,	MARK_OP_NONE, { 0, 0 }},
+	{"42/0",		TRUE,	MARK_OP_NONE, { 0, 0 }},
+	{"42/x",		FALSE,	MARK_OP_NONE, { 0 }},
+	{"42/42",		TRUE,	MARK_OP_NONE, { 42, 42 }},
+	{"42/0xff",		TRUE,	MARK_OP_NONE, { 42, 0xff }},
+	{"0x42/0xff",	TRUE,	MARK_OP_NONE, { 0x42, 0xff }},
+	{"/0xff",		TRUE,	MARK_OP_NONE, { 0, 0xff }},
+	{"/x",			FALSE,	MARK_OP_NONE, { 0 }},
+	{"x/x",			FALSE,	MARK_OP_NONE, { 0 }},
+	{"0xfffffff0/0x0000ffff",	TRUE,	MARK_OP_UNIQUE,
+		{ 0x0000fff0, 0x0000ffff }},
+	{"%unique",					TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0xffffffff }},
+	{"%unique/",				TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0 }},
+	{"%unique",					FALSE,	MARK_OP_NONE,
+		{ 0, 0 }},
+	{"%unique/0x0000ffff",		TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0x0000ffff }},
+	{"%unique/0xffffffff",		TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0xffffffff }},
+	{"%unique0xffffffffff",		FALSE,	MARK_OP_UNIQUE,
+		{ 0, 0 }},
+	{"0xffffffff/0x0000ffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0x0000ffff }},
+	{"0xffffffff/0xffffffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE, 0xffffffff }},
+	{"%unique-dir",				TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0xffffffff }},
+	{"%unique-dir/",			TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0 }},
+	{"%unique-dir",				FALSE,	MARK_OP_NONE,
+		{ 0, 0 }},
+	{"%unique-dir/0x0000ffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0x0000ffff }},
+	{"%unique-dir/0xffffffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0xffffffff }},
+	{"%unique-dir0xffffffff",	FALSE,	MARK_OP_UNIQUE,
+		{ 0, 0 }},
+	{"0xfffffffe/0x0000ffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0x0000ffff }},
+	{"0xfffffffe/0xffffffff",	TRUE,	MARK_OP_UNIQUE,
+		{ MARK_UNIQUE_DIR, 0xffffffff }},
+	{"%unique-/0xffffffff",		FALSE,	MARK_OP_UNIQUE,
+		{ 0, 0 }},
+	{"%unique-foo/0xffffffff",	FALSE,	MARK_OP_UNIQUE,
+		{ 0, 0 }},
 };
 
 START_TEST(test_mark_from_string)
 {
 	mark_t mark;
 
-	if (mark_from_string(mark_data[_i].s, &mark))
+	if (mark_from_string(mark_data[_i].s, mark_data[_i].ops, &mark))
 	{
 		ck_assert_int_eq(mark.value, mark_data[_i].m.value);
 		ck_assert_int_eq(mark.mask, mark_data[_i].m.mask);
diff --git a/src/starter/confread.c b/src/starter/confread.c
index 345d0b60bf..407ef5e13b 100644
--- a/src/starter/confread.c
+++ b/src/starter/confread.c
@@ -444,7 +444,7 @@ static void handle_keyword(kw_token_t token, starter_conn_t *conn, char *key,
 			KW_SA_OPTION_FLAG("yes", "no", SA_OPTION_COMPRESS)
 			break;
 		case KW_MARK:
-			if (!mark_from_string(value, &conn->mark_in))
+			if (!mark_from_string(value, MARK_OP_UNIQUE, &conn->mark_in))
 			{
 				cfg->err++;
 				break;
@@ -452,13 +452,13 @@ static void handle_keyword(kw_token_t token, starter_conn_t *conn, char *key,
 			conn->mark_out = conn->mark_in;
 			break;
 		case KW_MARK_IN:
-			if (!mark_from_string(value, &conn->mark_in))
+			if (!mark_from_string(value, MARK_OP_UNIQUE, &conn->mark_in))
 			{
 				cfg->err++;
 			}
 			break;
 		case KW_MARK_OUT:
-			if (!mark_from_string(value, &conn->mark_out))
+			if (!mark_from_string(value, MARK_OP_UNIQUE, &conn->mark_out))
 			{
 				cfg->err++;
 			}