diff mbox

[v2] iptables: allow service names in [DS]NAT targets

Message ID 20130708164606.GA10203@linuxace.com
State Superseded
Headers show

Commit Message

Phil Oester July 8, 2013, 4:46 p.m. UTC 
As reported by Alexander Hoogerhuis, the [DS]NAT targets do not allow use of
service names in the --to argument.  The same problem was fixed in the REDIRECT
target in commit 84d758b3 ("extensions: REDIRECT: fix --to-ports parser").
Use a similar fix here.

This closes bugzilla #514.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>

---
v2: also handle IPv6 [DS]NAT targets

Comments

Pablo Neira Ayuso July 24, 2013, 7 p.m. UTC | #1 
On Mon, Jul 08, 2013 at 09:46:06AM -0700, Phil Oester wrote:
> As reported by Alexander Hoogerhuis, the [DS]NAT targets do not allow use of
> service names in the --to argument.  The same problem was fixed in the REDIRECT
> target in commit 84d758b3 ("extensions: REDIRECT: fix --to-ports parser").
> Use a similar fix here.

While testing this I noticed that this works:

--to-source 1.1.1.1:telnet
--to-source 1.1.1.1-1.1.1.10:1025-3000
--to-source 1.1.1.1-1.1.1.10:telnet

But this does not:

--to-source 1.1.1.1-1.1.1.10:telnet-http
iptables v1.4.19.1: SNAT: Bad value for "--to" option:
"1.1.1.1-1.1.1.10:telnet-ssh"

I think it should, for consistency (even if I have to confess that it
looks a bit ugly to me).

If you decide to address this and send me a new version to support
this, then it would be also good to update the manpage to say that we
support services starting 1.4.20.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik July 24, 2013, 9:11 p.m. UTC | #2 
On Wed, 24 Jul 2013, Pablo Neira Ayuso wrote:

> On Mon, Jul 08, 2013 at 09:46:06AM -0700, Phil Oester wrote:
> > As reported by Alexander Hoogerhuis, the [DS]NAT targets do not allow use of
> > service names in the --to argument.  The same problem was fixed in the REDIRECT
> > target in commit 84d758b3 ("extensions: REDIRECT: fix --to-ports parser").
> > Use a similar fix here.
> 
> While testing this I noticed that this works:
> 
> --to-source 1.1.1.1:telnet
> --to-source 1.1.1.1-1.1.1.10:1025-3000
> --to-source 1.1.1.1-1.1.1.10:telnet
> 
> But this does not:
> 
> --to-source 1.1.1.1-1.1.1.10:telnet-http
> iptables v1.4.19.1: SNAT: Bad value for "--to" option:
> "1.1.1.1-1.1.1.10:telnet-ssh"
> 
> I think it should, for consistency (even if I have to confess that it
> looks a bit ugly to me).
> 
> If you decide to address this and send me a new version to support
> this, then it would be also good to update the manpage to say that we
> support services starting 1.4.20.

That is still ambiguous - there are service names with dash. So I suggest 
to support the notation '[name-with-dash]' in order to explicitly express 
and handle such cases.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Oester July 25, 2013, 12:17 a.m. UTC | #3 
On Wed, Jul 24, 2013 at 11:11:48PM +0200, Jozsef Kadlecsik wrote:
> On Wed, 24 Jul 2013, Pablo Neira Ayuso wrote:
> > But this does not:
> > 
> > --to-source 1.1.1.1-1.1.1.10:telnet-http
> > iptables v1.4.19.1: SNAT: Bad value for "--to" option:
> > "1.1.1.1-1.1.1.10:telnet-ssh"
> > 
> > I think it should, for consistency (even if I have to confess that it
> > looks a bit ugly to me).
> > 
> > If you decide to address this and send me a new version to support
> > this, then it would be also good to update the manpage to say that we
> > support services starting 1.4.20.
> 
> That is still ambiguous - there are service names with dash. So I suggest 
> to support the notation '[name-with-dash]' in order to explicitly express 
> and handle such cases.

Or perhaps as an alternative, we don't allow more than one port if one
wishes to use service names?  It seems the port parser is going to get so
complicated it will lead to bugs.  Particularly since ip6tables uses [ ] for
addresses to disambiguate them from the :port section.  Now we'd have to be
able to handle multiple [] arguments.

So these would be acceptable:

    :22-23
    :ssh
    :wap-push (port 2948)

this would not:

    :ssh-telnet

Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/extensions/libip6t_DNAT.c b/extensions/libip6t_DNAT.c
index eaa6bf1..f842e40 100644
--- a/extensions/libip6t_DNAT.c
+++ b/extensions/libip6t_DNAT.c
@@ -46,7 +46,7 @@  static const struct xt_option_entry DNAT_opts[] = {
 static void
 parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 {
-	char *arg, *start, *end = NULL, *colon = NULL, *dash, *error;
+	char *arg, *start, *end = "", *colon = NULL, *dash;
 	const struct in6_addr *ip;
 
 	arg = strdup(orig_arg);
@@ -60,8 +60,7 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 		colon = strchr(arg, ':');
 		if (colon && strchr(colon+1, ':'))
 			colon = NULL;
-	}
-	else {
+	} else {
 		start++;
 		end = strchr(start, ']');
 		if (end == NULL)
@@ -73,7 +72,7 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 	}
 
 	if (colon) {
-		int port;
+		unsigned int port, maxport;
 
 		if (!portok)
 			xtables_error(PARAMETER_PROBLEM,
@@ -81,34 +80,29 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 
 		range->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 
-		port = atoi(colon+1);
-		if (port <= 0 || port > 65535)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Port `%s' not valid\n", colon+1);
-
-		error = strchr(colon+1, ':');
-		if (error)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid port:port syntax - use dash\n");
+		if (!xtables_strtoui(colon + 1, &end, &port, 0, UINT16_MAX) &&
+		    (port = xtables_service_to_port(colon + 1, NULL)) == (unsigned)-1)
+			xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", colon);
 
-		dash = strchr(colon, '-');
-		if (!dash) {
+		switch (*end) {
+		case '\0':
 			range->min_proto.tcp.port
 				= range->max_proto.tcp.port
 				= htons(port);
-		} else {
-			int maxport;
+			break;
+		case '-':
+			if (!xtables_strtoui(end + 1, NULL, &maxport, 0, UINT16_MAX) &&
+			    (maxport = xtables_service_to_port(end + 1, NULL)) == (unsigned)-1)
+				xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", colon);
 
-			maxport = atoi(dash + 1);
-			if (maxport <= 0 || maxport > 65535)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port `%s' not valid\n", dash+1);
 			if (maxport < port)
-				/* People are stupid. */
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port range `%s' funky\n", colon+1);
+				xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", colon);
+
 			range->min_proto.tcp.port = htons(port);
 			range->max_proto.tcp.port = htons(maxport);
+			break;
+		default:
+			xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", colon);
 		}
 		/* Starts with colon or [] colon? No IP info...*/
 		if (colon == arg || colon == arg+2) {
diff --git a/extensions/libip6t_SNAT.c b/extensions/libip6t_SNAT.c
index 7382ad0..ea46f1a 100644
--- a/extensions/libip6t_SNAT.c
+++ b/extensions/libip6t_SNAT.c
@@ -46,7 +46,7 @@  static const struct xt_option_entry SNAT_opts[] = {
 static void
 parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 {
-	char *arg, *start, *end = NULL, *colon = NULL, *dash, *error;
+	char *arg, *start, *end = "", *colon = NULL, *dash;
 	const struct in6_addr *ip;
 
 	arg = strdup(orig_arg);
@@ -60,8 +60,7 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 		colon = strchr(arg, ':');
 		if (colon && strchr(colon+1, ':'))
 			colon = NULL;
-	}
-	else {
+	} else {
 		start++;
 		end = strchr(start, ']');
 		if (end == NULL)
@@ -73,7 +72,7 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 	}
 
 	if (colon) {
-		int port;
+		unsigned int port, maxport;
 
 		if (!portok)
 			xtables_error(PARAMETER_PROBLEM,
@@ -81,34 +80,29 @@  parse_to(const char *orig_arg, int portok, struct nf_nat_range *range)
 
 		range->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 
-		port = atoi(colon+1);
-		if (port <= 0 || port > 65535)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Port `%s' not valid\n", colon+1);
-
-		error = strchr(colon+1, ':');
-		if (error)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid port:port syntax - use dash\n");
+		if (!xtables_strtoui(colon + 1, &end, &port, 0, UINT16_MAX) &&
+		    (port = xtables_service_to_port(colon + 1, NULL)) == (unsigned)-1)
+			xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", colon);
 
-		dash = strchr(colon, '-');
-		if (!dash) {
+		switch (*end) {
+		case '\0':
 			range->min_proto.tcp.port
 				= range->max_proto.tcp.port
 				= htons(port);
-		} else {
-			int maxport;
+			break;
+		case '-':
+			if (!xtables_strtoui(end + 1, NULL, &maxport, 0, UINT16_MAX) &&
+			    (maxport = xtables_service_to_port(end + 1, NULL)) == (unsigned)-1)
+				xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", colon);
 
-			maxport = atoi(dash + 1);
-			if (maxport <= 0 || maxport > 65535)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port `%s' not valid\n", dash+1);
 			if (maxport < port)
-				/* People are stupid. */
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port range `%s' funky\n", colon+1);
+				xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", colon);
+
 			range->min_proto.tcp.port = htons(port);
 			range->max_proto.tcp.port = htons(maxport);
+			break;
+		default:
+			xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", colon);
 		}
 		/* Starts with colon or [] colon? No IP info...*/
 		if (colon == arg || colon == arg+2) {
diff --git a/extensions/libipt_DNAT.c b/extensions/libipt_DNAT.c
index ff18799..e452cfc 100644
--- a/extensions/libipt_DNAT.c
+++ b/extensions/libipt_DNAT.c
@@ -67,7 +67,7 @@  static struct xt_entry_target *
 parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 {
 	struct nf_nat_ipv4_range range;
-	char *arg, *colon, *dash, *error;
+	char *arg, *colon, *dash;
 	const struct in_addr *ip;
 
 	arg = strdup(orig_arg);
@@ -77,7 +77,8 @@  parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 	colon = strchr(arg, ':');
 
 	if (colon) {
-		int port;
+		char *end = "";
+		unsigned int port, maxport;
 
 		if (!portok)
 			xtables_error(PARAMETER_PROBLEM,
@@ -85,34 +86,29 @@  parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 
 		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 
-		port = atoi(colon+1);
-		if (port <= 0 || port > 65535)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Port `%s' not valid\n", colon+1);
-
-		error = strchr(colon+1, ':');
-		if (error)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid port:port syntax - use dash\n");
+		if (!xtables_strtoui(colon + 1, &end, &port, 0, UINT16_MAX) &&
+		    (port = xtables_service_to_port(colon + 1, NULL)) == (unsigned)-1)
+			xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", arg);
 
-		dash = strchr(colon, '-');
-		if (!dash) {
+		switch (*end) {
+		case '\0':
 			range.min.tcp.port
 				= range.max.tcp.port
 				= htons(port);
-		} else {
-			int maxport;
+			break;
+		case '-':
+			if (!xtables_strtoui(end + 1, NULL, &maxport, 0, UINT16_MAX) &&
+			    (maxport = xtables_service_to_port(end + 1, NULL)) == (unsigned)-1)
+				xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", arg);
 
-			maxport = atoi(dash + 1);
-			if (maxport <= 0 || maxport > 65535)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port `%s' not valid\n", dash+1);
 			if (maxport < port)
-				/* People are stupid. */
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port range `%s' funky\n", colon+1);
+				xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", arg);
+
 			range.min.tcp.port = htons(port);
 			range.max.tcp.port = htons(maxport);
+			break;
+		default:
+			xtables_param_act(XTF_BAD_VALUE, "DNAT", "--to", arg);
 		}
 		/* Starts with a colon? No IP info...*/
 		if (colon == arg) {
diff --git a/extensions/libipt_SNAT.c b/extensions/libipt_SNAT.c
index 1a24f3d..01be677 100644
--- a/extensions/libipt_SNAT.c
+++ b/extensions/libipt_SNAT.c
@@ -67,7 +67,7 @@  static struct xt_entry_target *
 parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 {
 	struct nf_nat_ipv4_range range;
-	char *arg, *colon, *dash, *error;
+	char *arg, *colon, *dash;
 	const struct in_addr *ip;
 
 	arg = strdup(orig_arg);
@@ -77,7 +77,8 @@  parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 	colon = strchr(arg, ':');
 
 	if (colon) {
-		int port;
+		char *end = "";
+		unsigned int port, maxport;
 
 		if (!portok)
 			xtables_error(PARAMETER_PROBLEM,
@@ -85,34 +86,29 @@  parse_to(const char *orig_arg, int portok, struct ipt_natinfo *info)
 
 		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 
-		port = atoi(colon+1);
-		if (port <= 0 || port > 65535)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Port `%s' not valid\n", colon+1);
-
-		error = strchr(colon+1, ':');
-		if (error)
-			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid port:port syntax - use dash\n");
+		if (!xtables_strtoui(colon + 1, &end, &port, 0, UINT16_MAX) &&
+		    (port = xtables_service_to_port(colon + 1, NULL)) == (unsigned)-1)
+			xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", arg);
 
-		dash = strchr(colon, '-');
-		if (!dash) {
+		switch (*end) {
+		case '\0':
 			range.min.tcp.port
 				= range.max.tcp.port
 				= htons(port);
-		} else {
-			int maxport;
+			break;
+		case '-':
+			if (!xtables_strtoui(end + 1, NULL, &maxport, 0, UINT16_MAX) &&
+			    (maxport = xtables_service_to_port(end + 1, NULL)) == (unsigned)-1)
+				xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", arg);
 
-			maxport = atoi(dash + 1);
-			if (maxport <= 0 || maxport > 65535)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port `%s' not valid\n", dash+1);
 			if (maxport < port)
-				/* People are stupid. */
-				xtables_error(PARAMETER_PROBLEM,
-					   "Port range `%s' funky\n", colon+1);
+				xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", arg);
+
 			range.min.tcp.port = htons(port);
 			range.max.tcp.port = htons(maxport);
+			break;
+		default:
+			xtables_param_act(XTF_BAD_VALUE, "SNAT", "--to", arg);
 		}
 		/* Starts with a colon? No IP info...*/
 		if (colon == arg) {