Attachment 3039 Details for Bug 1918 – improved negated match heuristic

[patch] improved negated match heuristic

bz1918.diff (text/plain), 8.36 KB, created by Damien Miller on 2017-08-25 18:57:33 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2017-08-25 18:57:33 AEST

Size: 8.36 KB

patch

obsolete

>diff --git a/addrmatch.c b/addrmatch.c
>index 8658e105..13058fc4 100644
>--- a/addrmatch.c
>+++ b/addrmatch.c
>@@ -379,7 +379,7 @@ addr_match_list(const char *addr, const char *_list)
> 	char *list, *cp, *o;
> 	struct xaddr try_addr, match_addr;
> 	u_int masklen, neg;
>-	int ret = 0, r;
>+	int got_neg = 0, seen_pos = 0, ret = 0, r;
> 
> 	if (addr != NULL && addr_pton(addr, &try_addr) != 0) {
> 		debug2("%s: couldn't parse address %.100s", __func__, addr);
>@@ -391,6 +391,8 @@ addr_match_list(const char *addr, const char *_list)
> 		neg = *cp == '!';
> 		if (neg)
> 			cp++;
>+		else
>+			seen_pos = 1;
> 		if (*cp == '\0') {
> 			ret = -2;
> 			break;
>@@ -411,16 +413,28 @@ addr_match_list(const char *addr, const char *_list)
> 					break;
> 				}
> 				ret = 1;
>-			}
>+			} else if (neg)
>+				got_neg = 1;
> 			continue;
> 		} else {
> 			/* If CIDR parse failed, try wildcard string match */
>-			if (addr != NULL && match_pattern(addr, cp) == 1)
>+			if (addr == NULL)
>+				continue;
>+			if (match_pattern(addr, cp) == 1)
> 				goto foundit;
>+			else if (neg)
>+				got_neg = 1;
> 		}
> 	}
> 	free(o);
> 
>+	/*
>+	 * Allow a list of all-negated clauses to return a positive result
>+	 * See the explanation in match_pattern_list() for more detail.
>+	 */
>+	if (ret == 0 && got_neg && !seen_pos)
>+		return 1;
>+
> 	return ret;
> }
> 
>diff --git a/match.c b/match.c
>index 3cf40306..bb5db8f2 100644
>--- a/match.c
>+++ b/match.c
>@@ -122,17 +122,19 @@ match_pattern_list(const char *string, const char *pattern, int dolower)
> {
> 	char sub[1024];
> 	int negated;
>-	int got_positive;
>+	int got_positive, got_negative, seen_positive;
> 	u_int i, subi, len = strlen(pattern);
> 
>-	got_positive = 0;
>+	seen_positive = got_positive = got_negative = 0;
> 	for (i = 0; i < len;) {
> 		/* Check if the subpattern is negated. */
> 		if (pattern[i] == '!') {
> 			negated = 1;
> 			i++;
>-		} else
>+		} else {
> 			negated = 0;
>+			seen_positive = 1;
>+		}
> 
> 		/*
> 		 * Extract the subpattern up to a comma or end.  Convert the
>@@ -160,9 +162,24 @@ match_pattern_list(const char *string, const char *pattern, int dolower)
> 				return -1;		/* Negative */
> 			else
> 				got_positive = 1;	/* Positive */
>-		}
>+		} else if (negated)
>+			got_negative = 1;
> 	}
> 
>+	/*
>+	 * Return success if we saw a negated match but only if all other
>+	 * patterns were negated too. E.g. matching "a" against a
>+	 * pattern-list "!b,!c,!d" should return 1.
>+	 *
>+	 * This avoids surprises with mixtures of negated and non-negated
>+	 * patterns. For example, given a pattern-list of of "!foo.xx,*.xx":
>+	 *     bar.xx => true
>+	 *     foo.xx => false
>+	 *     foo.yy => false
>+	 */
>+	if (got_negative && !seen_positive)
>+		return 1;
>+
> 	/*
> 	 * Return success if got a positive match.  If there was a negative
> 	 * match, we have already returned -1 and never get here.
>diff --git a/regress/unittests/match/tests.c b/regress/unittests/match/tests.c
>index e1593367..8eeb1ba9 100644
>--- a/regress/unittests/match/tests.c
>+++ b/regress/unittests/match/tests.c
>@@ -59,7 +59,9 @@ tests(void)
> 	ASSERT_INT_EQ(match_pattern_list("a", "*", 0), 1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "!*", 0), -1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "!a", 0), -1);
>-	/* XXX negated ASSERT_INT_EQ(match_pattern_list("a", "!b", 0), 1); */
>+	ASSERT_INT_EQ(match_pattern_list("a", "!a,!b", 0), -1);
>+	ASSERT_INT_EQ(match_pattern_list("a", "!b,!a", 0), -1);
>+	ASSERT_INT_EQ(match_pattern_list("a", "!b,!c", 0), 1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "!a,*", 0), -1);
> 	ASSERT_INT_EQ(match_pattern_list("b", "!a,*", 0), 1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "*,!a", 0), -1);
>@@ -67,7 +69,10 @@ tests(void)
> 	ASSERT_INT_EQ(match_pattern_list("a", "a,!*", 0), -1);
> 	ASSERT_INT_EQ(match_pattern_list("b", "a,!*", 0), -1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "a,!a", 0), -1);
>-	/* XXX negated ASSERT_INT_EQ(match_pattern_list("b", "a,!a", 0), 1); */
>+	ASSERT_INT_EQ(match_pattern_list("a.x", "!a.x,*.x", 0), -1);
>+	ASSERT_INT_EQ(match_pattern_list("a.x", "*.x,!a.x", 0), -1);
>+	ASSERT_INT_EQ(match_pattern_list("b.x", "!a.x,*.x", 0), 1);
>+	ASSERT_INT_EQ(match_pattern_list("b.x", "*.x,!a.x", 0), 1);
> 	ASSERT_INT_EQ(match_pattern_list("a", "!*,a", 0), -1);
> 	ASSERT_INT_EQ(match_pattern_list("b", "!*,a", 0), -1);
> 	TEST_DONE();
>@@ -88,19 +93,52 @@ tests(void)
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.1"), 1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.2"), 0);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.1"), -1);
>-	/* XXX negated ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.2"), 1); */
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.2"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.2,!127.0.0.3"), 1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.255", "127.0.0.0/24"), 1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "127.0.0.0/24"), 0);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.0/24"), 1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.1.0/24"), 0);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.0/24"), -1);
>-	/* XXX negated ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.1.0/24"), 1); */
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.1.0/24"), 1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.1,!127.0.0.1"), -1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.1,10.0.0.1"), -1);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.1,127.0.0.2"), 0);
> 	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.2,10.0.0.1"), 0);
>-	/* XXX negated ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.1,!127.0.0.2"), 1); */
>-	/* XXX negated ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.2,10.0.0.1"), 1); */
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.1,!127.0.0.2"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.2,10.0.0.1"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.1,127.0.0.0/8"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.0/8,!127.0.0.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.2", "!127.0.0.1,127.0.0.0/8"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.2", "127.0.0.0/8,!127.0.0.1"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.2", "!127.0.1.0/24,!127.0.0.1"), 1);
>+	TEST_DONE();
>+
>+	TEST_START("addr_match_list wildcard fallback");
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "127.0.0.*"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.1.*"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "127.0.1.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.*"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!127.0.0.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.1.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!127.0.1.*"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.0/8,127.0.0.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "10.0.0.0/8,127.0.0.*"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "10.0.0.0/8,127.0.1.*"), 0);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "10.0.0.0/8,127.0.1.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!10.0.0.0/8,!127.0.0.*"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!10.0.0.0/8,!127.0.0.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!10.0.0.0/8,!127.0.1.*"), 1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!10.0.0.0/8,!127.0.1.*"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.0.*,!127.0.0.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "127.0.*.*,!127.0.1.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "127.0.1.*,!127.0.0.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "127.0.1.*,!127.0.1.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.0.*,!127.0.1.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.0.1", "!127.0.1.*,!127.0.0.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!127.0.1.*,!127.0.0.1"), -1);
>+	ASSERT_INT_EQ(addr_match_list("127.0.1.1", "!127.0.0.*,!127.0.0.*,!172.0.0.0/8"), 1);
> 	TEST_DONE();
> 
> #define CHECK_FILTER(string,filter,expected) \
>diff --git a/ssh_config.5 b/ssh_config.5
>index 15ca0b4f..b1527124 100644
>--- a/ssh_config.5
>+++ b/ssh_config.5
>@@ -1659,6 +1659,10 @@ pool,
> the following entry (in authorized_keys) could be used:
> .Pp
> .Dl from=\&"!*.dialup.example.com,*.example.com\&"
>+.Pp
>+If a pattern-list contains a mixture of negated and non-negated patterns,
>+at least one of the non-negated patterns must match for the result to be
>+considered successful.
> .Sh TOKENS
> Arguments to some keywords can make use of tokens,
> which are expanded at runtime:

Actions: View | Diff

Attachments on bug 1918: 2061 | 3039