Bug 1918 - match_pattern_list fails for negated failure
Summary: match_pattern_list fails for negated failure
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 5.8p2
Hardware: All All
: P2 normal
Assignee: Assigned to nobody
URL:
Keywords:
: 1680 2397 (view as bug list)
Depends on:
Blocks: 2397 V_7_7
  Show dependency treegraph
 
Reported: 2011-07-06 02:10 AEST by Robin Becker
Modified: 2018-04-06 12:26 AEST (History)
3 users (show)

See Also:


Attachments
patch to fix the 'bug' (404 bytes, patch)
2011-07-06 02:10 AEST, Robin Becker
no flags Details | Diff
improved negated match heuristic (8.36 KB, patch)
2017-08-25 18:57 AEST, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Becker 2011-07-06 02:10:13 AEST
Created attachment 2061 [details]
patch to fix the 'bug'

The code in match_pattern_list will never return 1 for a pattern with all negated entries. In particular this match line can never succeed

Match User !adminguy Group sftponly

The problem is that the code at match.c line 157 only tests for negation in the case of successful matching. In this case we want the User test to succeed if the user is not adminguy. That can happen if the code is patched to set get_positive if a failed match is negated. The attached trivial patch does that.

Of course I am making the rather feeble assumption that

negated failure == true
Comment 1 Damien Miller 2012-02-24 10:34:29 AEDT
Retarget from 6.0 to 6.1
Comment 2 Damien Miller 2012-02-24 10:38:08 AEDT
Retarget 6.0 => 6.1
Comment 3 Damien Miller 2012-09-07 11:38:18 AEST
Retarget uncompleted bugs from 6.1 => 6.2
Comment 4 Damien Miller 2012-09-07 11:40:43 AEST
Retarget bugs from 6.1 => 6.2
Comment 5 Damien Miller 2013-03-08 10:24:08 AEDT
retarget to openssh-6.3
Comment 6 Damien Miller 2013-07-25 12:18:03 AEST
Retarget to openssh-6.4
Comment 7 Damien Miller 2013-07-25 12:21:01 AEST
Retarget 6.3 -> 6.4
Comment 8 Damien Miller 2014-02-06 10:18:10 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 9 Damien Miller 2014-02-06 10:20:20 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 10 Damien Miller 2014-04-12 14:50:11 AEST
Retarget to 6.7 release, since 6.6 was mostly bugfixing.
Comment 11 Damien Miller 2014-04-12 14:55:18 AEST
Remove from 6.6 tracking bug
Comment 12 Damien Miller 2014-08-30 04:38:19 AEST
Retarget incomplete bugs to 6.8 release.
Comment 13 Damien Miller 2014-08-30 04:40:11 AEST
These bugs are no longer targeted at the imminent 6.7 release
Comment 14 Damien Miller 2015-03-03 07:59:27 AEDT
OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release.
Comment 15 Damien Miller 2015-03-03 08:01:43 AEDT
Retarget to 6.9
Comment 16 Damien Miller 2015-04-17 14:57:41 AEST
*** Bug 1680 has been marked as a duplicate of this bug. ***
Comment 17 Damien Miller 2015-05-22 13:34:21 AEST
This has some chance at breaking existing configurations, so retarget to the 7.0 release where we are a little more tolerant of breakage.
Comment 18 Damien Miller 2015-08-11 22:59:07 AEST
Retarget pending bugs to openssh-7.1
Comment 19 Damien Miller 2016-02-26 14:44:27 AEDT
Retarget to openssh-7.3
Comment 20 Damien Miller 2016-02-26 14:47:23 AEDT
Retarget to openssh-7.3
Comment 21 Damien Miller 2016-07-22 14:10:54 AEST
retarget unfinished bugs to next release
Comment 22 Damien Miller 2016-07-22 14:14:52 AEST
retarget unfinished bugs to next release
Comment 23 Damien Miller 2016-07-22 14:15:49 AEST
retarget unfinished bugs to next release
Comment 24 Damien Miller 2016-07-22 14:17:11 AEST
retarget unfinished bugs to next release
Comment 25 Damien Miller 2016-08-23 14:34:27 AEST
This is fixed and will be in the OpenSSH 7.4 release. Thanks!

commit 4067ec8a4c64ccf16250c35ff577b4422767da64
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Tue Aug 23 03:22:49 2016 +0000

    upstream commit
    
    fix matching for pattern lists that contain a single
    negated match, e.g. "Host !example"
    
    report and patch from Robin Becker. bz#1918 ok dtucker@
    
    Upstream-ID: 05a0cb323ea4bc20e98db099b42c067bfb9ea1ea
Comment 26 Robin Becker 2016-08-23 18:03:28 AEST
I guess all's well that ends well :)
Comment 27 Damien Miller 2016-09-22 02:59:21 AEST
I've reverted this fix because it yields surprising behaviour, e.g.

Match address 2002::/16,!::1

will also match 10.0.0.1. I'll look at a better fix, probably a combination of special-casing match strings that consist solely of negated matches and better documentation.
Comment 28 Damien Miller 2016-12-16 14:31:24 AEDT
OpenSSH 7.4 release is closing; punt the bugs to 7.5
Comment 29 Damien Miller 2017-06-30 13:43:09 AEST
Move incomplete bugs to openssh-7.6 target since 7.5 shipped a while back.

To calibrate expectations, there's little chance all of these are going to make 7.6.
Comment 30 Damien Miller 2017-06-30 13:44:31 AEST
remove 7.5 target
Comment 31 Damien Miller 2017-08-25 18:55:17 AEST
*** Bug 2397 has been marked as a duplicate of this bug. ***
Comment 32 Damien Miller 2017-08-25 18:57:33 AEST
Created attachment 3039 [details]
improved negated match heuristic

Revisiting this, I think I've come up with a heuristic that doesn't
yield horrible surprises:

If a pattern-list contains a mixture of negated and non-negated patterns,
then it will only return success if the comparison string matches one of
the non-negated patterns. E.g.

match("foo.example.com", "!bar.example.com,*.example.com") => success
match("bar.example.com", "!bar.example.com,*.example.com") => failure
match("notexample.com", "!bar.example.com,*.example.com") => failure

If the pattern-list contains only negated matches. the it will return
success if none of them match. E.g.

match("a", "!a,!b") => failure
match("a", "!b,!c") => success

These examples use test strings, but the same logic applies for address matching too.

This patch implements this heuristic and adds unit tests for it.
Comment 33 Damien Miller 2017-10-18 13:51:14 AEDT
I couldn't reach consensus with the other developers on applying this patch, so have clarified the documentation instead:

> Note that a negated match will never produce a positive result
> by itself. For example, attempting to match "host3" against the 
> following pattern-list will fail:
>
>    from="!host1,!host2"
>
> The solution here is to include a term that will yield a 
> positive match, such as a wildcard:
>
>    from="!host1,!host2,*"
Comment 34 Damien Miller 2018-04-06 12:26:45 AEST
Close all resolved bugs after release of OpenSSH 7.7.