conntrack: fix coupled-options sanity checkings

This patch extends the generic_opt_check() function to add
extra information on the possible option combinations. Under
some specific situations, like the creation and getting of
a conntrack, you may specify the original or the reply tuple
but at least one MUST be present. This handling has been
always tricky, it still remains but we're more user friendly
at least.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

6 files changed, 119 insertions, 79 deletions

diff --git a/extensions/libct_proto_icmp.c b/extensions/libct_proto_icmp.c
index 51366f1..3a346ed 100644
--- a/extensions/libct_proto_icmp.c
+++ b/extensions/libct_proto_icmp.c
@@ -103,7 +103,7 @@ static void final_check(unsigned int flags,
 	generic_opt_check(flags,
 			  ICMP_NUMBER_OF_OPT,
 			  icmp_commands_v_options[cmd],
-			  icmp_optflags);
+			  icmp_optflags, NULL, 0, NULL);
 }
 
 static struct ctproto_handler icmp = {
diff --git a/extensions/libct_proto_icmpv6.c b/extensions/libct_proto_icmpv6.c
index cfc5979..070eb7f 100644
--- a/extensions/libct_proto_icmpv6.c
+++ b/extensions/libct_proto_icmpv6.c
@@ -103,10 +103,9 @@ static void final_check(unsigned int flags,
 		        unsigned int cmd,
 		        struct nf_conntrack *ct)
 {
-	generic_opt_check(flags,
-			  ICMPV6_NUMBER_OF_OPT,
-			  icmpv6_commands_v_options[cmd],
-			  icmpv6_optflags);
+	generic_opt_check(flags, ICMPV6_NUMBER_OF_OPT,
+			  icmpv6_commands_v_options[cmd], icmpv6_optflags,
+			  NULL, 0, NULL);
 }
 
 static struct ctproto_handler icmpv6 = {
diff --git a/extensions/libct_proto_tcp.c b/extensions/libct_proto_tcp.c
index 30d7229..ac54ac7 100644
--- a/extensions/libct_proto_tcp.c
+++ b/extensions/libct_proto_tcp.c
@@ -56,10 +56,10 @@ static char tcp_commands_v_options[NUMBER_OF_CMD][TCP_NUMBER_OF_OPT] =
 {
 	    	/* 1 2 3 4 5 6 7 8 9 */
 /*CT_LIST*/   	  {2,2,2,2,0,0,2,0,0},
-/*CT_CREATE*/	  {2,2,2,2,0,0,1,0,0},
+/*CT_CREATE*/	  {3,3,3,3,0,0,1,0,0},
 /*CT_UPDATE*/	  {2,2,2,2,0,0,2,0,0},
 /*CT_DELETE*/	  {2,2,2,2,0,0,2,0,0},
-/*CT_GET*/	  {2,2,2,2,0,0,2,0,0},
+/*CT_GET*/	  {3,3,3,3,0,0,2,0,0},
 /*CT_FLUSH*/	  {0,0,0,0,0,0,0,0,0},
 /*CT_EVENT*/	  {2,2,2,2,0,0,2,0,0},
 /*CT_VERSION*/	  {0,0,0,0,0,0,0,0,0},
@@ -172,36 +172,36 @@ static int parse_options(char c,
 	return 1;
 }
 
+#define TCP_VALID_FLAGS_MAX   2
+static unsigned int tcp_valid_flags[TCP_VALID_FLAGS_MAX] = {
+       CT_TCP_ORIG_SPORT | CT_TCP_ORIG_DPORT,
+       CT_TCP_REPL_SPORT | CT_TCP_REPL_DPORT,
+};
+
 static void final_check(unsigned int flags,
 			unsigned int cmd,
 			struct nf_conntrack *ct)
 {
-	if ((1 << cmd) & (CT_CREATE|CT_GET)) {
-		if (!(flags & CT_TCP_ORIG_SPORT) &&
-		     (flags & CT_TCP_ORIG_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--sport'");
-		}
-		if ((flags & CT_TCP_ORIG_SPORT) &&
-		    !(flags & CT_TCP_ORIG_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--dport'");
-		}
-		if (!(flags & CT_TCP_REPL_SPORT) &&
-		    (flags & CT_TCP_REPL_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--reply-port-src'");
-		}
-		if ((flags & CT_TCP_REPL_SPORT) &&
-		    !(flags & CT_TCP_REPL_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--reply-port-dst'");
+	int ret, partial;
+
+	ret = generic_opt_check(flags, TCP_NUMBER_OF_OPT,
+				tcp_commands_v_options[cmd], tcp_optflags,
+				tcp_valid_flags, TCP_VALID_FLAGS_MAX, &partial);
+	if (!ret) {
+		switch(partial) {
+		case -1:
+		case 0:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--sport' and "
+						      "`--dport'");
+			break;
+		case 1:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--reply-src-port' and "
+						      "`--reply-dst-port'");
+			break;
 		}
 	}
-	generic_opt_check(flags, 
-			  TCP_NUMBER_OF_OPT,
-			  tcp_commands_v_options[cmd],
-			  tcp_optflags);
 }
 
 static struct ctproto_handler tcp = {
diff --git a/extensions/libct_proto_udp.c b/extensions/libct_proto_udp.c
index 4f34e3b..d7c4da1 100644
--- a/extensions/libct_proto_udp.c
+++ b/extensions/libct_proto_udp.c
@@ -64,10 +64,10 @@ static char udp_commands_v_options[NUMBER_OF_CMD][UDP_NUMBER_OF_OPT] =
 {
 		/* 1 2 3 4 5 6 7 8 */
 /*CT_LIST*/	  {2,2,2,2,0,0,0,0},
-/*CT_CREATE*/     {2,2,2,2,0,0,0,0},
+/*CT_CREATE*/     {3,3,3,3,0,0,0,0},
 /*CT_UPDATE*/     {2,2,2,2,0,0,0,0},
 /*CT_DELETE*/     {2,2,2,2,0,0,0,0},
-/*CT_GET*/        {2,2,2,2,0,0,0,0},
+/*CT_GET*/        {3,3,3,3,0,0,0,0},
 /*CT_FLUSH*/      {0,0,0,0,0,0,0,0},
 /*CT_EVENT*/      {2,2,2,2,0,0,0,0},
 /*CT_VERSION*/    {0,0,0,0,0,0,0,0},
@@ -144,36 +144,36 @@ static int parse_options(char c,
 	return 1;
 }
 
+#define UDP_VALID_FLAGS_MAX   2
+static unsigned int udp_valid_flags[UDP_VALID_FLAGS_MAX] = {
+       CT_UDP_ORIG_SPORT | CT_UDP_ORIG_DPORT,
+       CT_UDP_REPL_SPORT | CT_UDP_REPL_DPORT,
+};
+
 static void final_check(unsigned int flags,
 		        unsigned int cmd,
 		        struct nf_conntrack *ct)
 {
-	if ((1 << cmd) & (CT_CREATE|CT_GET)) {
-		if (!(flags & CT_UDP_ORIG_SPORT) &&
-		     (flags & CT_UDP_ORIG_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--sport'");
-		}
-		if ((flags & CT_UDP_ORIG_SPORT) &&
-		    !(flags & CT_UDP_ORIG_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--dport'");
-		}
-		if (!(flags & CT_UDP_REPL_SPORT) &&
-		    (flags & CT_UDP_REPL_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--reply-port-src'");
-		}
-		if ((flags & CT_UDP_REPL_SPORT) &&
-		    !(flags & CT_UDP_REPL_DPORT)) {
-			exit_error(PARAMETER_PROBLEM,
-				   "missing `--reply-port-dst'");
+	int ret, partial;
+
+	ret = generic_opt_check(flags, UDP_NUMBER_OF_OPT,
+				udp_commands_v_options[cmd], udp_optflags,
+				udp_valid_flags, UDP_VALID_FLAGS_MAX, &partial);
+	if (!ret) {
+		switch(partial) {
+		case -1:
+		case 0:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--sport' and "
+						      "`--dport'");
+			break;
+		case 1:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--reply-src-port' and "
+						      "`--reply-dst-port'");
+			break;
 		}
 	}
-	generic_opt_check(flags, 
-			  UDP_NUMBER_OF_OPT,
-			  udp_commands_v_options[cmd],
-			  udp_optflags);
 }
 
 static struct ctproto_handler udp = {
diff --git a/include/conntrack.h b/include/conntrack.h
index e1f8d0a..17c0121 100644
--- a/include/conntrack.h
+++ b/include/conntrack.h
@@ -188,10 +188,10 @@ enum exittype {
 	VERSION_PROBLEM
 };
 
-void generic_opt_check(int options, 
-		       int nops,
-		       char *optset,
-		       const char *optflg[]);
+int generic_opt_check(int options, int nops,
+		      char *optset, const char *optflg[],
+		      unsigned int *coupled_flags, int coupled_flags_size,
+		      int *partial);
 void exit_error(enum exittype status, const char *msg, ...);
 
 extern void register_proto(struct ctproto_handler *h);
diff --git a/src/conntrack.c b/src/conntrack.c
index 0305408..e1c57e5 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -115,6 +115,7 @@ static unsigned int global_option_offset = 0;
  *  0  illegal
  *  1  compulsory
  *  2  optional
+ *  3  undecided, see flag combination checkings in generic_opt_check()
  */
 
 static char commands_v_options[NUMBER_OF_CMD][NUMBER_OF_OPT] =
@@ -122,10 +123,10 @@ static char commands_v_options[NUMBER_OF_CMD][NUMBER_OF_OPT] =
 {
           /*   s d r q p t u z e [ ] { } a m i f n g o c b*/
 /*CT_LIST*/   {2,2,2,2,2,0,2,2,0,0,0,0,0,0,2,0,2,2,2,2,2,0},
-/*CT_CREATE*/ {2,2,2,2,1,1,2,0,0,0,0,0,0,2,2,0,0,2,2,0,0,0},
+/*CT_CREATE*/ {3,3,3,3,1,1,2,0,0,0,0,0,0,2,2,0,0,2,2,0,0,0},
 /*CT_UPDATE*/ {2,2,2,2,2,2,2,0,0,0,0,0,0,0,2,2,2,2,2,2,0,0},
 /*CT_DELETE*/ {2,2,2,2,2,2,2,0,0,0,0,0,0,0,2,2,2,2,2,2,0,0},
-/*CT_GET*/    {2,2,2,2,1,0,0,0,0,0,0,0,0,0,0,2,0,0,0,2,0,0},
+/*CT_GET*/    {3,3,3,3,1,0,0,0,0,0,0,0,0,0,0,2,0,0,0,2,0,0},
 /*CT_FLUSH*/  {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0},
 /*CT_EVENT*/  {2,2,2,2,2,0,0,0,2,0,0,0,0,0,2,0,0,2,2,2,2,2},
 /*VERSION*/   {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0},
@@ -141,6 +142,12 @@ static char commands_v_options[NUMBER_OF_CMD][NUMBER_OF_OPT] =
 /*X_STATS*/   {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0},
 };
 
+#define ADDR_VALID_FLAGS_MAX   2
+static unsigned int addr_valid_flags[ADDR_VALID_FLAGS_MAX] = {
+	CT_OPT_ORIG_SRC | CT_OPT_ORIG_DST,
+	CT_OPT_REPL_SRC | CT_OPT_REPL_DST,
+};
+
 static LIST_HEAD(proto_list);
 
 static unsigned int options;
@@ -260,12 +267,12 @@ static int bit2cmd(int command)
 	return i;
 }
 
-void generic_opt_check(int local_options, 
-		       int num_opts,
-		       char *optset, 
-		       const char *optflg[])
+int generic_opt_check(int local_options, int num_opts,
+		      char *optset, const char *optflg[],
+		      unsigned int *coupled_flags, int coupled_flags_size,
+		      int *partial)
 {
-	int i;
+	int i, matching = -1, special_case = 0;
 
 	for (i = 0; i < num_opts; i++) {
 		if (!(local_options & (1<<i))) {
@@ -280,7 +287,33 @@ void generic_opt_check(int local_options,
 					   "option `--%s' with this "
 					   "command", optflg[i]);
 		}
+		if (optset[i] == 3)
+			special_case = 1;
+	}
+
+	/* no weird flags combinations, leave */
+	if (!special_case || coupled_flags == NULL)
+		return 1;
+
+	*partial = -1;
+	for (i=0; i<coupled_flags_size; i++) {
+		/* we look for an exact matching to ensure this is correct */
+		if ((local_options & coupled_flags[i]) == coupled_flags[i]) {
+			matching = i;
+			break;
+		}
+		/* ... otherwise look for the first partial matching */
+		if ((local_options & coupled_flags[i]) && *partial < 0) {
+			*partial = i;
+		}
 	}
+
+	/* we found an exact matching, game over */
+	if (matching >= 0)
+		return 1;
+
+	/* report a partial matching to suggest something */
+	return 0;
 }
 
 static struct option *
@@ -995,7 +1028,7 @@ int main(int argc, char *argv[])
 {
 	int c, cmd;
 	unsigned int type = 0, event_mask = 0, l4flags = 0, status = 0;
-	int res = 0;
+	int res = 0, partial;
 	size_t socketbuffersize = 0;
 	int family = AF_UNSPEC;
 	char __obj[nfct_maxsize()];
@@ -1197,16 +1230,24 @@ int main(int argc, char *argv[])
 		family = AF_INET;
 
 	cmd = bit2cmd(command);
-	generic_opt_check(options,
-			  NUMBER_OF_OPT,
-			  commands_v_options[cmd],
-			  optflags);
-
-	if (command & (CT_CREATE|CT_GET) &&
-	    !((options & CT_OPT_ORIG_SRC && options & CT_OPT_ORIG_DST) ||
-	      (options & CT_OPT_REPL_SRC && options & CT_OPT_REPL_DST)))
-	      exit_error(PARAMETER_PROBLEM, "missing IP address");
-
+	res = generic_opt_check(options, NUMBER_OF_OPT,
+				commands_v_options[cmd], optflags,
+				addr_valid_flags, ADDR_VALID_FLAGS_MAX,
+				&partial);
+	if (!res) {
+		switch(partial) {
+		case -1:
+		case 0:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--src' and `--dst'");
+			break;
+		case 1:
+			exit_error(PARAMETER_PROBLEM, "you have to specify "
+						      "`--reply-src' and "
+						      "`--reply-dst'");
+			break;
+		}
+	}
 	if (!(command & CT_HELP) && h && h->final_check)
 		h->final_check(l4flags, cmd, obj);