Bug 12576 - popt aliases allow users to bypass sudo argument restrictions
Summary: popt aliases allow users to bypass sudo argument restrictions
Status: RESOLVED FIXED
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.1.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-11 22:21 UTC by Paul Donohue
Modified: 2017-02-20 19:13 UTC (History)
1 user (show)

See Also:


Attachments
Do not enable popt aliases if --server or --daemon is specified (1.94 KB, patch)
2017-02-11 22:22 UTC, Paul Donohue
no flags Details
Add a new --no-popt-aliases option to explicitly disable popt aliases (3.27 KB, patch)
2017-02-11 22:23 UTC, Paul Donohue
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Donohue 2017-02-11 22:21:39 UTC
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'.
Comment 1 Paul Donohue 2017-02-11 22:22:48 UTC
Created attachment 12915 [details]
Do not enable popt aliases if --server or --daemon is specified
Comment 2 Paul Donohue 2017-02-11 22:23:40 UTC
Created attachment 12916 [details]
Add a new --no-popt-aliases option to explicitly disable popt aliases
Comment 3 Kevin Korb 2017-02-11 23:35:43 UTC
There is no reason to involve rsyncd (or even sudo).  See the rrsync script in the support directory.
Comment 4 Paul Donohue 2017-02-12 02:18:35 UTC
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.
Comment 5 Karl O. Pinc 2017-02-12 05:05:59 UTC
On Sun, 12 Feb 2017 02:18:35 +0000
samba-bugs@samba.org wrote:

> https://bugzilla.samba.org/show_bug.cgi?id=12576
> 
> --- Comment #4 from Paul Donohue <samba-bugs@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@meme.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein
Comment 6 Paul Donohue 2017-02-12 17:06:20 UTC
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.
Comment 7 Kevin Korb 2017-02-13 21:12:40 UTC
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.
Comment 8 Paul Donohue 2017-02-17 14:56:24 UTC
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?)
Comment 9 Paul Donohue 2017-02-17 15:28:57 UTC
popt ticket requesting a solution in popt itself: http://rpm5.org/cvs/tktview?tn=98
Comment 10 Wayne Davison 2017-02-19 22:00:25 UTC
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).
Comment 11 Wayne Davison 2017-02-20 19:13:44 UTC
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.