openssh bugs - Oct 2016 - [Bug 2625] New: Support Capabilities for ssh client port forwarding

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

            Bug ID: 2625
           Summary: Support Capabilities for ssh client port forwarding
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: alukardd+openssh_mindrot at alukardd.org

Created attachment 2880
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2880&action=edit
Don't require a root if there is CAP_NET_BIND_SERVICE

Hello.

I think openssh-client should allow use port forwarding not only for 
root user.
CAP_NET_BIND_SERVICE enought to use privileged ports.

I did patch for myself, but I think, that you could improve it and
apply 
to master.

This patch requires one more build dependency: libcap-dev.

Regards,
Alexey Mochkin.

p.s. Forward from
http://marc.info/?l=openssh-unix-dev&m=147666133031142&w=2

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2880
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2880
Don't require a root if there is CAP_NET_BIND_SERVICE

I'm ok with doing something like this but the patch currently needs
some work:
 - unconditionally linking against libcap will break every other
platform that doesn't have it.  Ditto the actual function calls.
 - putting the libcap interface code inline in readconf.c will make
maintenance of that file harder as future changes need to be pulled in,
and that file changes a lot.
 - there is the equivalent check in sshd, which this code does not
address.

I've started by factoring this check out into its own function:
https://anongit.mindrot.org/openssh.git/commit/?id=1c4ef0b808d3d38232aeeb1cebb7e9a43def42c5
>From there we needs to add the appropriate configure time
--with-capabilities flag and add the code inside #ifdef
USE_CAPABILITIES inside misc.c:bind_permitted().

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Darren Tucker from comment #1)
> the patch currently needs some work:
also:

 - the return values of cap_get_proc cap_get_flag are not checked but
the values are still used.  The values are used anyway, so the
behaviour is undefined at best and segfault at worst.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2880|0                           |1
        is obsolete|                            |

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2883
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2883&action=edit
Add configure bits, move Linux specific code to port-linux.c

Something like this.  Compile tested only.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #4 from Alexey Mochkin <alukardd+openssh_mindrot at
alukardd.org> ---
Your bind_permitted() function should have fallback to check for uid =0 if no
capabilities were presented.


+#ifdef LINUX_CAPABILITIES
+       if (linux_capability_bind_permitted()) {
+               return 1;
+       } else {
+#else
        if (port < IPPORT_RESERVED && uid != 0)
                return 0;
        return 1;
+#endif
+#ifdef LINUX_CAPABILITIES
+       }
+#endif

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #5 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Alexey Mochkin from comment #4)
> Your bind_permitted() function should have fallback to check for uid
> == 0 if no capabilities were presented.
fair enough, but it'd be cleaner if it just fell through, eg:

int
bind_permitted(int port, uid_t uid)
{
#ifdef LINUX_CAPABILITIES
        if (linux_capability_bind_permitted())
                return 1;
#endif
        if (port < IPPORT_RESERVED && uid != 0)
                return 0;
        return 1;

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2647

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
OpenSSH 7.4 release is closing; punt the bugs to 7.5


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2594                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Richard E. Silverman <res at qoxp.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |res at qoxp.net

--- Comment #7 from Richard E. Silverman <res at qoxp.net> ---
Hello,

This should be addressed, but I disagree with the proposed solution
here. The real problem is not that ssh checks its euid -- it is that
ssh tries to guess whether the kernel will allow it to bind a low port,
but cannot in principle know what is required for that; that's the
kernel's job, and will change depending on the security facilities in
use on a particular system. It's like refusing to try to open a file if
the mode bits don't seem to allow you to: maybe an ACL would allow it.
Or deciding that you must be able to open the file, but then finding
that you can't because SELinux is enabled, and the policy blocks it.
Programs should not second-guess the kernel: ssh should just try to
bind the port, and report the result.

Pleasantly, this also gets rid of all the issues discussed here around
the usage of libcap etc.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #8 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Richard E. Silverman from comment #7)
> This should be addressed, but I disagree with the proposed solution
> here. The real problem is not that ssh checks its euid
Well it checks the uid of the user logging in, which may or may not be
the euid of the process.

In the case where sshd is running with UsePrivilegeSeparation=no the
process making the bind() calls is running as root even when handling
non-root logins.  Similarly ssh can be installed setuid, although it's
not common any more.  If you don't have some kind of check (or do
temporarily_use_uid()), well, things like
https://bugs.chromium.org/p/project-zero/issues/detail?id=1010 happen.

Currently these errors are currently caught at config parse time.  Your
proposal wouldn't detect them until later when the connection was
already up.

These are solvable, eg by temporarily_use_uid() and/or by testing binds
during config parsing, but it's not a simple case of "delete those
checks and YOLO".

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #9 from Richard E. Silverman <res at qoxp.net> ---
Ah, I was only thinking about the client-side case, since that is how
this bug report started:
> I think openssh-client should allow use port forwarding not only for
> root user. CAP_NET_BIND_SERVICE enough to use privileged ports.
... and in fact I came across this bugzilla entry because I was about
to file one for the same problem with the client-side UsePrivilegedPort
option, which is silently turned off if the euid is not 0:

[ssh.c]
        if (original_effective_uid != 0)
                options.use_privileged_port = 0;

... which is similarly inaccurate.
> These are solvable, eg by temporarily_use_uid() and/or by testing
> binds during config parsing, but it's not a simple case of 
> "delete those checks and YOLO".
Agreed, on the server side where privilege management is involved; I
was advocating a different approach to the problem rather than giving a
detailed, finished solution. On the client just that should be almost
enough. If we want to preserve the current behavior on the client --
that the connection succeeds anyway -- then it would try to bind the
low port, and if it gets EPERM (or any error?), retry without the
low-port restriction before giving up.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #10 from Alexey Mochkin <alukardd+openssh_mindrot at
alukardd.org> ---
Hi Richard,

I think it's not bad idea for openssh-client check nothing, but tries
to do what user ask it.
And if there is no permission for such action(OS return any kind of
error) then return error to user.
Agree.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2883|0                           |1
        is obsolete|                            |

--- Comment #11 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2941
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2941&action=edit
remove uid checks for binds (openbsd, probably won't apply cleanly to
portable)

I've been looking at this.  Attached is a patch that removes the
explicit uid checks and use temporarily_use_uid when calling bind and
related functions.

I still need to test this properly (probably by writing a regression
test), and it does not address UsePrivilegedPort (that can be looked at
separately).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2698

--- Comment #12 from Damien Miller <djm at mindrot.org> ---
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.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2698
[Bug 2698] Tracking bug for OpenSSH 7.6 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
remove 7.5 target

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2647                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2782
             Blocks|2698                        |
             Blocks|                            |2852


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2698
[Bug 2698] Tracking bug for OpenSSH 7.6 release
https://bugzilla.mindrot.org/show_bug.cgi?id=2782
[Bug 2782] Tracking bug for OpenSSH 7.7 release
https://bugzilla.mindrot.org/show_bug.cgi?id=2852
[Bug 2852] Tracking bug for OpenSSH 7.8 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2782                        |

--- Comment #14 from Damien Miller <djm at mindrot.org> ---
Move to OpenSSH 7.8 tracking bug


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2782
[Bug 2782] Tracking bug for OpenSSH 7.7 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |djm at mindrot.org
         Resolution|---                         |FIXED

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Darren removed all the preemptive checks for low-numbered ports, etc as
part of his purge of setuid support in ssh(1). OpenSSH 7.8 will just
attempt the bind and either succeed or fail, using whatever
capabilities/rules apply.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2625

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.