Created attachment 2525 [details] Make config parser more strict to ip:port values According to manual pages above mentioned options in sshd_config accept only values in format ip:port, but parser used in code also accept ip/port which can lead to unexpected results when someone doesn't understand what he is doing. Great example is our bugzilla [1]. Shortly problem was using ListenAddress 192.168.1.0/24 which ended in converting number 24 into port and in SELinux denial. This behaviour can be prevented by appended patch, which is accepting only valid values according to manual pages. This is done in function hpdelim, which is used only for parsing above mentioned ListenAddress and PermitOpen (same syntax according to man pages). [1] https://bugzilla.redhat.com/show_bug.cgi?id=1130733
Hello, can we fix also this, since we are changing the configuration parsers? 192.168.1.0/24 is certainly not a valid syntax for IP and port pair.
Created attachment 3137 [details] New version of patch for OpenSSH 7.7p1 (prerelease) The old patch does not apply anymore since it is now used also for other things so I put together a new version with hpdelim2(). See attached patch. Any chance getting this finally fixed. Simple test cases, that fail without this patch: ./sshd -f /dev/null -T -oPermitOpen=localhost/222 -oHostKey=regress/rsa| grep 222 ./sshd -f /dev/null -T -oListenAddress=localhost/222 -oHostKey=regress/rsa| grep 222
Some git archaeology reveals this was added in 2001 to support IPv6 addresses with ports: https://github.com/openssh/openssh-portable/commit/d71ba5771b5c67b4efd3294ecb85dc4d10d03265 at the time it was documented in sshd(8). These days it's not in line with the relevant standards (eg https://tools.ietf.org/html/rfc5952#section-6) so we should probably remove it.
Created attachment 3231 [details] remove support for obsolete host/port notation reworked the patch a little to apply to openbsd current.
Patch applied and will be in the 8.0 release. Thanks.
BTW: all of the diffs using hpdelim2 had a bug, fortunately not critical: in the common case where there's no delimiter hpdelim2 does not populate the 2nd arg so you would see spurious failures if the uninitialised memory happened to contain "/". Fixed in a follow up commit.
Comment on attachment 3231 [details] remove support for obsolete host/port notation ok, but two nits: >+char *hpdelim2(char **, char *); Maybe hpdelim_char() would be a little more descriptive? >--- servconf.c 19 Jan 2019 21:37:48 -0000 1.346 >+++ servconf.c 23 Jan 2019 10:44:04 -0000 ... >@@ -1251,8 +1251,10 @@ process_server_config_line(ServerOptions > port = 0; > p = arg; > } else { >- p = hpdelim(&arg); >- if (p == NULL) >+ char ch; >+ arg2 = NULL; Either newline after "char ch" or move it to the declarations of the start of the function
(In reply to Damien Miller from comment #7) > Maybe hpdelim_char() would be a little more descriptive? Maybe, but this only (re) adds the prototype, the function has existed under this name for a while: https://github.com/openssh/openssh-portable/commit/887669ef032d63cf07f53cada216fa8a0c9a7d72 > Either newline after "char ch" or move it to the declarations of the > start of the function Already fixed in https://github.com/openssh/openssh-portable/commit/281ce042579b834cdc1e74314f1fb2eeb75d2612
*** Bug 3010 has been marked as a duplicate of this bug. ***
closing resolved bugs as of 8.6p1 release
[spam removed]