[PATCH] ssh: reject "all" combined with other Match criteria

[jelly]

To: openssh-unix-dev at mindrot.org
Subject: [PATCH] ssh: reject "all" combined with other Match criteria


[Resend: my previous copy of this message was reformatted in transit -- the
inline diff arrived with HTML entity escapes in place of ">" and "&", so it
could not be applied.  Same content, sent as plain text this time; the patch
is attached as 0001-ssh-reject-all-combined-with-other-Match-criteria.patch.]


Hi,


the "all" keyword in a Match block must appear alone.  sshd enforces
this, but the ssh client does not, so the same configuration file is
accepted by one half of the toolchain and rejected by the other.


In readconf.c:match_cfg_line(), the check that "all" is not combined
with any other criteria is made with "attributes > 1", but the counter
is incremented only after that check:


      if ((negate = (attrib[0] == '!')))
            attrib++;
      /* Criterion "all" has no argument and must appear alone */
      if (strcasecmp(attrib, "all") == 0) {
            if (attributes > 1 || ...) {
                  error("'all' cannot be combined with other "
                      "Match attributes");
                  result = -1;
                  goto out;
            }
            ...
            goto out;
      }
      attributes++;


So when "all" is processed it observes only the number of preceding
criteria.  For "Match user foo all" attributes == 1 at that point, the
check is skipped, and the line is silently accepted.  servconf.c
increments before the check (servconf.c:864) and rejects the same
configuration, which is the behaviour the manual describes:


      $ cat cfg
      Match user foo all
            Compression yes
      $ ssh -G -F cfg example.com
      exit=0                              # accepted (wrong)
      $ sshd -T -f cfg
      'all' cannot be combined with other Match attributes
      exit=255                      # rejected (correct)


The practical effect is that a user can write a Match block intending to
scope options to a subset of connections, and the client will silently
treat it as broader than written (Match x all is equivalent to Match x).
It is a validation inconsistency rather than a security bug, but since
the client and server disagree about which configurations are valid, it
seems worth making consistent.


The fix is to move the increment ahead of the check, matching what
servconf.c already does.


Tested on current master (4001f0a), patched build:
  - "Match user foo all" and "Match !user foo all" -> rejected (exit 255)
  - "Match all" -> still accepted (exit 0)
  - "Match all foo" -> still rejected by the existing argument check
  - "Match host x user y" -> accepted (multiple criteria without all:
    unaffected)
  - "make unit" passes
  - regress/sshcfgparse.sh gains coverage for both the standalone and
    combined cases; the new case fails if readconf.c alone is reverted
    ("Match combined with all accepted"), so it really exercises the bug


The same change is pushed to the immortal-jelly/openssh-portable branch
"match-all-alone".


Patch attached: 0001-ssh-reject-all-combined-with-other-Match-criteria.patch