On 2011-08-28 08:30:04 AM, wearetherob...@puppetlabs.com wrote: > In order to support use cases where an authorized_key file is written to > a non-standard location, which may not be writable by the user, this patch > removes the step in the flush method that switches users before writing > the authorized_key file to disk. As a result, the authorized_key can now > be written to any location. > > This patch does not change the core functionality of the > ssh_authorized_key type. This seems dangerous, as when the authorized_keys file is in a location that is writable by the user, the user can make it a symlink to say, /etc/shadow and get puppet to write to it.
Looking at the rest of this code, there is currently a chown that occurs
before privileges are dropped, which looks like it might be a security
vulnerability:
In the flush method in lib/puppet/provider/ssh_authorized_key/parsed.rb:
unless File.exist?(dir = File.dirname(target))
Puppet.debug "Creating #{dir}"
Dir.mkdir(dir, dir_perm)
File.chown(uid, nil, dir)
end
If a user manages to replace the directory with a symlink to /etc right before
the chown call, then it will be chowned to the user (chown follows symlinks,
lchown does not).
The chown and chmod commands at the end of the function are also potentially
dangerous, since both of these will follow symlinks. Here's a patch which
moves both of these into the block which is run with dropped privileges. I
removed the chown call entirely, as it should the file should already be owned
by the right user when it's created.
Thanks,
Ricky
From 08cbb08c2e579b4bff0850a375d40580e3b9b29c Mon Sep 17 00:00:00 2001
From: Ricky Zhou <ri...@fedoraproject.org>
Date: Mon, 29 Aug 2011 16:01:12 -0400
Subject: [PATCH] Drop privileges for creating and chmodding SSH keys.
Previously, potentially abusable chown and chmod calls were performed as
root. This tries to moves as much as possible into code which is run
after privileges have been dropped.
---
lib/puppet/provider/ssh_authorized_key/parsed.rb | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb
b/lib/puppet/provider/ssh_authorized_key/parsed.rb
index 81b1fbc..65c4cef 100644
--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb
+++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -50,21 +50,22 @@ require 'puppet/provider/parsedfile'
def flush
raise Puppet::Error, "Cannot write SSH authorized keys without user"
unless @resource.should(:user)
raise Puppet::Error, "User '#{@resource.should(:user)}' does not exist"
unless uid = Puppet::Util.uid(@resource.should(:user))
- unless File.exist?(dir = File.dirname(target))
- Puppet.debug "Creating #{dir}"
- Dir.mkdir(dir, dir_perm)
- File.chown(uid, nil, dir)
- end
-
# ParsedFile usually calls backup_target much later in the flush process,
# but our SUID makes that fail to open filebucket files for writing.
# Fortunately, there's already logic to make sure it only ever happens
once,
# so calling it here supresses the later attempt by our superclass's flush
method.
self.class.backup_target(target)
- Puppet::Util::SUIDManager.asuser(@resource.should(:user)) { super }
- File.chown(uid, nil, target)
- File.chmod(file_perm, target)
+ Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
+ unless File.exist?(dir = File.dirname(target))
+ Puppet.debug "Creating #{dir}"
+ Dir.mkdir(dir, dir_perm)
+ end
+
+ super
+
+ File.chmod(file_perm, target)
+ end
end
# parse sshv2 option strings, wich is a comma separated list of
--
1.7.6
pgpOavy6g45iK.pgp
Description: PGP signature
