From dd8481cc1c92af32ec69cded42b985b7bad40b26 Mon Sep 17 00:00:00 2001
From: "Ondrej Zajicek (work)" <santiago@crfreenet.org>
Date: Tue, 18 May 2021 19:54:18 +0200
Subject: [PATCH] Flowspec: Do not use comma for bitmask operators

For numeric operators, comma is used for disjunction in expressions like
"10, 20, 30..40". But for bitmask operators, comma is used for
conjunction in a way that does not really make much sense. Use always
explicit logical operators (&& and ||) to connect bitmask operators.

Thanks to Matt Corallo for the bugreport.
---
 doc/bird.sgml       | 15 ++++++++-------
 filter/test.conf    |  2 +-
 lib/flowspec.c      | 13 ++-----------
 lib/flowspec_test.c |  2 +-
 4 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index 51a92ce9..ff599216 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -5080,20 +5080,21 @@ options (<cf/bfd/ and <cf/weight 1/), the second nexthop has just <cf/weight 2/.
 
 <p>The flow specification are rules for routers and firewalls for filtering
 purpose. It is described by <rfc id="5575">. There are 3 types of arguments:
-<m/inet4/ or <m/inet6/ prefixes, bitmasks matching expressions and numbers
+<m/inet4/ or <m/inet6/ prefixes, numeric matching expressions and bitmask
 matching expressions.
 
-Bitmasks matching is written using <m/value/<cf>/</cf><m/mask/ or
-<cf/!/<m/value/<cf>/</cf><m/mask/ pairs. It means that <cf/(/<m/data/ <cf/&/
-<m/mask/<cf/)/ is or is not equal to <m/value/.
-
-Numbers matching is a matching sequence of numbers and ranges separeted by a
+Numeric matching is a matching sequence of numbers and ranges separeted by a
 commas (<cf/,/) (e.g. <cf/10,20,30/). Ranges can be written using double dots
 <cf/../ notation (e.g. <cf/80..90,120..124/). An alternative notation are
 sequence of one or more pairs of relational operators and values separated by
 logical operators <cf/&&/ or <cf/||/. Allowed relational operators are <cf/=/,
 <cf/!=/, <cf/</, <cf/<=/, <cf/>/, <cf/>=/, <cf/true/ and <cf/false/.
 
+Bitmask matching is written using <m/value/<cf>/</cf><m/mask/ or
+<cf/!/<m/value/<cf>/</cf><m/mask/ pairs. It means that <cf/(/<m/data/ <cf/&/
+<m/mask/<cf/)/ is or is not equal to <m/value/. It is also possible to use
+multiple value/mask pairs connected by logical operators <cf/&&/ or <cf/||/.
+
 <sect2>IPv4 Flowspec
 
 <p><descrip>
@@ -5199,7 +5200,7 @@ protocol static {
 		next header = 23;
 		sport > 24 && < 30 || = 40 || 50,60,70..80;
 		dport = 50;
-		tcp flags 0x03/0x0f, !0/0xff || 0x33/0x33;
+		tcp flags 0x03/0x0f && !0/0xff || 0x33/0x33;
 		fragment !is_fragment || !first_fragment;
 		label 0xaaaa/0xaaaa && 0x33/0x33;
 	};
diff --git a/filter/test.conf b/filter/test.conf
index 63af25bb..653bb82f 100644
--- a/filter/test.conf
+++ b/filter/test.conf
@@ -579,7 +579,7 @@ prefix p;
 	bt_assert(format(flow6 { sport 0..0x400; }) = "flow6 { sport 0..1024; }");
 	bt_assert(format(flow6 { icmp type 80; }) = "flow6 { icmp type 80; }");
 	bt_assert(format(flow6 { icmp code 90; }) = "flow6 { icmp code 90; }");
-	bt_assert(format(flow6 { tcp flags 0x03/0x0f; }) = "flow6 { tcp flags 0x3/0x3,0x0/0xc; }");
+	bt_assert(format(flow6 { tcp flags 0x03/0x0f; }) = "flow6 { tcp flags 0x3/0x3 && 0x0/0xc; }");
 	bt_assert(format(flow6 { length 0..65535; }) = "flow6 { length 0..65535; }");
 	bt_assert(format(flow6 { dscp = 63; }) = "flow6 { dscp 63; }");
 	bt_assert(format(flow6 { fragment is_fragment || !first_fragment; }) = "flow6 { fragment is_fragment || !first_fragment; }");
diff --git a/lib/flowspec.c b/lib/flowspec.c
index e47fb7e2..1445435a 100644
--- a/lib/flowspec.c
+++ b/lib/flowspec.c
@@ -1278,17 +1278,8 @@ net_format_flow_bitmask(buffer *b, const byte *part)
   while (1)
   {
     if (!first)
-    {
-      if (isset_and(op))
-      {
-	b->pos--; /* Remove last char (it is a space) */
-	buffer_puts(b, ",");
-      }
-      else
-      {
-	buffer_puts(b, "|| ");
-      }
-    }
+      buffer_puts(b, isset_and(op) ? "&& " : "|| ");
+
     first = 0;
 
     len = get_value_length(op);
diff --git a/lib/flowspec_test.c b/lib/flowspec_test.c
index b6b4d7b8..2285075c 100644
--- a/lib/flowspec_test.c
+++ b/lib/flowspec_test.c
@@ -630,7 +630,7 @@ t_formatting4(void)
   net_addr_flow4 *input;
   NET_ADDR_FLOW4_(input, ip4_build(5, 6, 7, 0), 24, nlri);
 
-  const char *expect = "flow4 { dst 10.0.0.0/8; proto 23; dport > 24 && < 30 || 40..50,60..70,80 && >= 90; sport > 24 && < 30 || 40,50,60..70,80; icmp type 80; icmp code 90; tcp flags 0x3/0x3,0x0/0xc; length 0..65535; dscp 63; fragment dont_fragment || !is_fragment; }";
+  const char *expect = "flow4 { dst 10.0.0.0/8; proto 23; dport > 24 && < 30 || 40..50,60..70,80 && >= 90; sport > 24 && < 30 || 40,50,60..70,80; icmp type 80; icmp code 90; tcp flags 0x3/0x3 && 0x0/0xc; length 0..65535; dscp 63; fragment dont_fragment || !is_fragment; }";
 
   bt_assert(flow4_net_format(b, sizeof(b), input) == strlen(expect));
   bt_debug(" expect: '%s',\n output: '%s'\n", expect, b);