rsync - Feb 2017 - [Bug 12576] popt aliases allow users to bypass sudo argument restrictions

https://bugzilla.samba.org/show_bug.cgi?id=12576

            Bug ID: 12576
           Summary: popt aliases allow users to bypass sudo argument
                    restrictions
           Product: rsync
           Version: 3.1.3
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: core
          Assignee: wayned at samba.org
          Reporter: samba-bugs at PaulSD.com
        QA Contact: rsync-qa at samba.org

My goal is to allow a specific user to read but not write a specific file as
root using rsync via SSH.

The obvious solution was to configure /etc/rsyncd.conf to allow read-only
access to the file, then add "user ALL=(root) NOPASSWD:/usr/bin/rsync
--server
--daemon ." to /etc/sudoers, then have the user run `rsync --rsh=ssh
--rsync-path='sudo rsync' host::module/file .`

However, this is not as secure as it appears.  Unfortunately, the popt alias
feature allows the user to replace the rsync arguments with almost any other
arbitrary rsync arguments, which effectively gives the user full root access to
the system.  For example, the user can add the following to /home/user/.popt
and run `sudo rsync --server --daemon .` to read the contents of /etc/shadow:
rsync alias --server -v
rsync alias --daemon /etc/shadow

This can be prevented by setting the 'always_set_home' option in sudoers
(so
that only root's popt config is read), although this setting is global and
may
not be desirable in all cases.  This can also be prevented by creating a shell
script that overrides $HOME then runs rsync, and using that shell script
instead of rsync in both sudoers and --rsync-path.  However, that is an
unintuitive solution that few users are likely to implement unless a giant
disclaimer is added to the documentation.  This really seems like a problem
that should be solved in rsync itself.

In the rsync code, popt aliases are explicitly disabled when '--server'
or
'--daemon' is used, but only after those arguments have been parsed with
popt
aliases enabled, which is why the above example is able to use popt aliases to
override those arguments.  The first attached patch checks for
'--server' or
'--daemon' before enabling popt aliases, which fixes this issue when
'--server'
or '--daemon' are used.

The second attached patch adds a new '--no-popt-aliases' argument.  This
explicitly disables popt aliases and may be used to allow rsync to be
safely run using sudo with an argument list that does not include
'--server' or
'--daemon'.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #1 from Paul Donohue <samba-bugs at PaulSD.com> ---
Created attachment 12915
  --> https://bugzilla.samba.org/attachment.cgi?id=12915&action=edit
Do not enable popt aliases if --server or --daemon is specified

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #2 from Paul Donohue <samba-bugs at PaulSD.com> ---
Created attachment 12916
  --> https://bugzilla.samba.org/attachment.cgi?id=12916&action=edit
Add a new --no-popt-aliases option to explicitly disable popt aliases

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #3 from Kevin Korb <rsync at sanitarium.net> ---
There is no reason to involve rsyncd (or even sudo).  See the rrsync script in
the support directory.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #4 from Paul Donohue <samba-bugs at PaulSD.com> ---
That's an interesting solution, but it doesn't really work well for my
use
case.  I would like my users to be able to maintain their own SSH keys (this
solution would require me to manage users' SSH keys in
/root/.ssh/authorized_keys), and I don't particularly want to set
"PermitRootLogin yes" in /etc/ssh/sshd_config.  I also already have
scripts to
manage sudo permissions, and I would have to make some significant changes to
support centrally managing authorized_keys.

I think the rsyncd+sudo solution actually works pretty well except for the
non-obvious fact that popt lets the user override the sudo restrictions.

There are are lots of rsync users out there who are running rsync through sudo,
so even if there happens to be a better way to handle my specific use case, it
seems to me that there either needs to be a giant disclaimer somewhere that
says running rsync in sudo is dangerous and suggests alternative solutions, or
rsync needs to provide some reasonably intuitive mitigations.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

On Sun, 12 Feb 2017 02:18:35 +0000
samba-bugs at samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=12576
> 
> --- Comment #4 from Paul Donohue <samba-bugs at PaulSD.com> ---
> That's an interesting solution, but it doesn't really work well for
> my use case.  I would like my users to be able to maintain their own
> SSH keys (this solution would require me to manage users' SSH keys in
> /root/.ssh/authorized_keys), and I don't particularly want to set
> "PermitRootLogin yes" in /etc/ssh/sshd_config.  I also already
have
> scripts to manage sudo permissions, and I would have to make some
> significant changes to support centrally managing authorized_keys.
You don't have to set "PermitRootLogin yes".  You can set
"PermitRootLogin forced-commands-only".  Far more secure.

Yes, you probably would need a script, or something, that takes a ssh
key and somehow modifies /root/.ssh/authorized_keys.  

One secure way to do this is to have a script (or any
other interface) that drops a file containing the key 
(and other requisite information) into a directory
that is watched with, e.g., incron.  incron can then run a script
that parses the file, pulls out the requisite information, and
has the permissions to modify /root/.ssh/authorized_keys.
(And then cleans up by deleting the file.)  This way the process
with permissions to modify /root/.ssh/authorized_keys can
verify the data it's putting in there and validate the credentials
of the user putting the data in.

You could instead leverage your sudo management scripts to
give users permissions to run scripts as root that modify
/root/.ssh/authorized_keys.  Here it's sudo, instead of
the script run by incron, controlling access. Anybody without
authorization gets a permission error.  As above, this approach
avoids the insecurity of suid root scripts.

Either way, the script the user runs must pick up the user's
username (or home directory, or whatever) itself.  This avoids
the problem of allowing one user to set permissions for another.
The trick for the script run via sudo would be to use logname(1).

So, writing a script (or scripts) to add/remove/replace lines in
/root/.ssh/authorized_keys does not sound all that much work,
although of course there are always devilish details.  flock(1) is
probably your friend if you write in shell.  You probably
want to log with logger(1), etc.

Regards,

Karl <kop at meme.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #5 from Karl O. Pinc <kop at meme.com> ---
On Sun, 12 Feb 2017 02:18:35 +0000
samba-bugs at samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=12576
> 
> --- Comment #4 from Paul Donohue <samba-bugs at PaulSD.com> ---
> That's an interesting solution, but it doesn't really work well for
> my use case.  I would like my users to be able to maintain their own
> SSH keys (this solution would require me to manage users' SSH keys in
> /root/.ssh/authorized_keys), and I don't particularly want to set
> "PermitRootLogin yes" in /etc/ssh/sshd_config.  I also already
have
> scripts to manage sudo permissions, and I would have to make some
> significant changes to support centrally managing authorized_keys.
You don't have to set "PermitRootLogin yes".  You can set
"PermitRootLogin forced-commands-only".  Far more secure.

Yes, you probably would need a script, or something, that takes a ssh
key and somehow modifies /root/.ssh/authorized_keys.  

One secure way to do this is to have a script (or any
other interface) that drops a file containing the key 
(and other requisite information) into a directory
that is watched with, e.g., incron.  incron can then run a script
that parses the file, pulls out the requisite information, and
has the permissions to modify /root/.ssh/authorized_keys.
(And then cleans up by deleting the file.)  This way the process
with permissions to modify /root/.ssh/authorized_keys can
verify the data it's putting in there and validate the credentials
of the user putting the data in.

You could instead leverage your sudo management scripts to
give users permissions to run scripts as root that modify
/root/.ssh/authorized_keys.  Here it's sudo, instead of
the script run by incron, controlling access. Anybody without
authorization gets a permission error.  As above, this approach
avoids the insecurity of suid root scripts.

Either way, the script the user runs must pick up the user's
username (or home directory, or whatever) itself.  This avoids
the problem of allowing one user to set permissions for another.
The trick for the script run via sudo would be to use logname(1).

So, writing a script (or scripts) to add/remove/replace lines in
/root/.ssh/authorized_keys does not sound all that much work,
although of course there are always devilish details.  flock(1) is
probably your friend if you write in shell.  You probably
want to log with logger(1), etc.

Regards,

Karl <kop at meme.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #6 from Paul Donohue <samba-bugs at PaulSD.com> ---
This all makes sense, I appreciate the suggestions, and I may actually
implement some of this.

However, the existence of this rrsync solution doesn't change the fact that
there exists another simple and obvious solution using sudo which has a giant
undocumented security hole related to an unusual, undocumented, and not widely
used feature of rsync (popt aliases).

My goal for this bug report is to either get a disclaimer added to the rsync
man page (which documents popt aliases feature, explains the security
implications, and suggests mitigations and/or alternative solutions to avoid
security issues, including this rrsync solution), or to get the attached
trivial patches merged to help mitigate this security issue without requiring
users to wrap complicated scripts around rsync or avoid the use of sudo.

Security is hard enough to get right when everything works in a consistent and
intuitive manner.  Having an unusual, unintuitive, and undocumented feature
with significant undocumented security implications is just asking for trouble.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #7 from Kevin Korb <rsync at sanitarium.net> ---
I have been thinking about this a bit and I believe it is a sudo problem and
not an rsync problem.  It is not rsync's job to secure the command line. 
Plus
rsync is far from the only program that uses popt to parse the command line and
therefore not the only program that would be affected by this problem.

However, I do think that Wayne should add at least one of your patches since
this would also affect rrsync and other forms of ssh ForcedCommands.

Note that I don't know much about popt and might be missing something
obvious/simple.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #8 from Paul Donohue <samba-bugs at PaulSD.com> ---
I agree with the general philosophy that it isn't rsync's problem to
secure the
command line.  However, I don't see any good way that sudo can secure the
rsync
command line unless rsync provides some mechanism for disabling popt aliases,
hence why I'm proposing these patches.

Note that popt aliases are an optional feature of popt and are not
automatically enabled when popt is used, so this problem will not necessarily
affect every program that uses popt.  However, perhaps an alternative solution
would be to extend popt itself to allow popt aliases to be disabled in some
standard way in any application that uses them.  (Maybe a standard command line
option that applies to all programs using popt, or an environment variable, or
some code to automatically detect when popt is running under sudo and disable
aliases?)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

--- Comment #9 from Paul Donohue <samba-bugs at PaulSD.com> ---
popt ticket requesting a solution in popt itself:
http://rpm5.org/cvs/tktview?tn=98

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

Wayne Davison <wayned at samba.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #10 from Wayne Davison <wayned at samba.org> ---
One thing I was thinking is that the popt code could have a sanity check when
reading in user aliases from a different user when running as root. So, the
code would check that it was running as uid 0 and just ignore a $HOME-based
popt aliases file if the file wasn't also uid 0.

However, I do also like the idea of not using popt aliases for the --server
side (including --daemon) as that could also cause rsync some problems in how
things get setup between the 2 sides (in addition to being a potential security
issue).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=12576

Wayne Davison <wayned at samba.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Wayne Davison <wayned at samba.org> ---
I have added some code that makes sure that --daemon and --server can never be
aliased to any other value. The code has always disabled popt aliases during
the parsing of a daemon or server command-line, but if the main option itself
was removed then rsync wouldn't know to disable aliases. This new code
overrides any system or user alias to ensure that can never happen.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.