From 62983f0d68e9771d5d78c24ff6840bfac1b2a359 Mon Sep 17 00:00:00 2001 From: Todd Zullinger Date: Thu, 29 Sep 2011 19:31:28 -0400 Subject: [PATCH] Apply upstream security patches Fixes for CVE-2011-3869, CVE-2011-3870, CVE-2011-3871, and upstream ticket #9793 are included. Further details can be found in the upstream announcement: http://groups.google.com/group/puppet-announce/browse_thread/thread/91e3b46d2328a1cb --- 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch | 107 ++++++ ...dictable-temporary-filename-in-ralsh.patch | 69 ++++ ...rector-file-backed-terminus-base-cla.patch | 330 ++++++++++++++++++ ...an-overwrite-arbitrary-files-as-root.patch | 40 +++ puppet.spec | 21 +- 5 files changed, 566 insertions(+), 1 deletion(-) create mode 100644 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch create mode 100644 2.6.x-9792-Predictable-temporary-filename-in-ralsh.patch create mode 100644 2.6.x-9793-secure-indirector-file-backed-terminus-base-cla.patch create mode 100644 2.6.x-9794-k5login-can-overwrite-arbitrary-files-as-root.patch diff --git a/2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch b/2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch new file mode 100644 index 0000000..a1b7728 --- /dev/null +++ b/2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch @@ -0,0 +1,107 @@ +From 8d9575775737c08c6cbfdf7f9a22f2ea4ab21b20 Mon Sep 17 00:00:00 2001 +From: Ricky Zhou +Date: Mon, 29 Aug 2011 16:01:12 -0400 +Subject: [PATCH] Drop privileges before 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. + +Huge thanks to Ricky Zhou for discovering this and +supplying the security fix. Awesome work. + +Signed-off-by: Daniel Pittman +--- + lib/puppet/provider/ssh_authorized_key/parsed.rb | 19 ++++++++++--------- + .../provider/ssh_authorized_key/parsed_spec.rb | 16 ++++++++-------- + 2 files changed, 18 insertions(+), 17 deletions(-) + +diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb +index 6a3855c..5243477 100644 +--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb ++++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb +@@ -56,21 +56,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 +diff --git a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb +index 2e5be16..64935df 100755 +--- a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb ++++ b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb +@@ -133,15 +133,15 @@ describe provider_class do + @provider.flush + end + +- it "should chown the directory to the user" do ++ it "should absolutely not chown the directory to the user" do + uid = Puppet::Util.uid("random_bob") +- File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir") ++ File.expects(:chown).never + @provider.flush + end + +- it "should chown the key file to the user" do ++ it "should absolutely not chown the key file to the user" do + uid = Puppet::Util.uid("random_bob") +- File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir/place_to_put_authorized_keys") ++ File.expects(:chown).never + @provider.flush + end + +@@ -177,11 +177,11 @@ describe provider_class do + @provider.flush + end + +- it "should chown the directory to the user if it creates it" do ++ it "should absolutely not chown the directory to the user if it creates it" do + File.stubs(:exist?).with(@dir).returns false + Dir.stubs(:mkdir).with(@dir,0700) + uid = Puppet::Util.uid("nobody") +- File.expects(:chown).with(uid, nil, @dir) ++ File.expects(:chown).never + @provider.flush + end + +@@ -192,9 +192,9 @@ describe provider_class do + @provider.flush + end + +- it "should chown the key file to the user" do ++ it "should absolutely not chown the key file to the user" do + uid = Puppet::Util.uid("nobody") +- File.expects(:chown).with(uid, nil, File.expand_path("~nobody/.ssh/authorized_keys")) ++ File.expects(:chown).never + @provider.flush + end + +-- +1.7.6.4 + diff --git a/2.6.x-9792-Predictable-temporary-filename-in-ralsh.patch b/2.6.x-9792-Predictable-temporary-filename-in-ralsh.patch new file mode 100644 index 0000000..5d9647b --- /dev/null +++ b/2.6.x-9792-Predictable-temporary-filename-in-ralsh.patch @@ -0,0 +1,69 @@ +From 906da37374def334b62722acf84e4b0d1324e1f7 Mon Sep 17 00:00:00 2001 +From: Daniel Pittman +Date: Wed, 28 Sep 2011 23:35:19 -0700 +Subject: [PATCH] (#9792) Predictable temporary filename in ralsh. + +When ralsh is used in edit mode the temporary filename is in a shared +directory, and is absolutely predictable. Worse, it won't be touched until +well after the startup of the command. + +It can be tricked into writing through a symlink to edit any file on the +system, or to create through it, but worse - the file is reopened with the +same name later, so it can have the target replaced between edit and +operate... + +The only possible mitigation comes from the system editor and the behaviour it +has around editing through symbolic links, which is very weak. + +This improves this to prefer the current working directory for the temporary +file, and to be somewhat less predictable and more safe in conjuring it into +being. + +Signed-off-by: Daniel Pittman +--- + lib/puppet/application/resource.rb | 27 +++++++++++++++++---------- + 1 files changed, 17 insertions(+), 10 deletions(-) + +diff --git a/lib/puppet/application/resource.rb b/lib/puppet/application/resource.rb +index bc4faf5..3e4147e 100644 +--- a/lib/puppet/application/resource.rb ++++ b/lib/puppet/application/resource.rb +@@ -88,18 +88,25 @@ class Puppet::Application::Resource < Puppet::Application + end.map(&format).join("\n") + + if options[:edit] +- file = "/tmp/x2puppet-#{Process.pid}.pp" ++ require 'tempfile' ++ # Prefer the current directory, which is more likely to be secure ++ # and, in the case of interactive use, accessible to the user. ++ tmpfile = Tempfile.new('x2puppet', Dir.pwd) + begin +- File.open(file, "w") do |f| +- f.puts text +- end +- ENV["EDITOR"] ||= "vi" +- system(ENV["EDITOR"], file) +- system("puppet -v #{file}") ++ # sync write, so nothing buffers before we invoke the editor. ++ tmpfile.sync = true ++ tmpfile.puts text ++ ++ # edit the content ++ system(ENV["EDITOR"] || 'vi', tmpfile.path) ++ ++ # ...and, now, pass that file to puppet to apply. Because ++ # many editors rename or replace the original file we need to ++ # feed the pathname, not the file content itself, to puppet. ++ system('puppet -v ' + tmpfile.path) + ensure +- #if FileTest.exists? file +- # File.unlink(file) +- #end ++ # The temporary file will be safely removed. ++ tmpfile.close(true) + end + else + puts text +-- +1.7.6.4 + diff --git a/2.6.x-9793-secure-indirector-file-backed-terminus-base-cla.patch b/2.6.x-9793-secure-indirector-file-backed-terminus-base-cla.patch new file mode 100644 index 0000000..3e15fa1 --- /dev/null +++ b/2.6.x-9793-secure-indirector-file-backed-terminus-base-cla.patch @@ -0,0 +1,330 @@ +From 40f025a9ae0373e2e642f7811face3486bfa34d4 Mon Sep 17 00:00:00 2001 +From: Daniel Pittman +Date: Thu, 29 Sep 2011 00:07:16 -0700 +Subject: [PATCH] (#9793) "secure" indirector file backed terminus base class. + +The file base class in the indirector trusted the request key directly, which +made it vulnerable to the same potential for injection attacks as other +terminus base classes. + +However, this is somewhat mitigated by the fact that base class is entirely +unused. We can simple eliminate it from the system, because nothing is more +secure than code that doesn't exist. + +The only consumer of the code was in the tests, and didn't care what base +class was used, so that was substituted with a continuing class. + +Signed-off-by: Daniel Pittman +--- + lib/puppet/indirector/file.rb | 79 -------------- + spec/unit/indirector/file_spec.rb | 181 --------------------------------- + spec/unit/indirector/terminus_spec.rb | 6 +- + 3 files changed, 3 insertions(+), 263 deletions(-) + delete mode 100644 lib/puppet/indirector/file.rb + delete mode 100755 spec/unit/indirector/file_spec.rb + +diff --git a/lib/puppet/indirector/file.rb b/lib/puppet/indirector/file.rb +deleted file mode 100644 +index b3746b7..0000000 +--- a/lib/puppet/indirector/file.rb ++++ /dev/null +@@ -1,79 +0,0 @@ +-require 'puppet/indirector/terminus' +- +-# Store instances as files, usually serialized using some format. +-class Puppet::Indirector::File < Puppet::Indirector::Terminus +- # Where do we store our data? +- def data_directory +- name = Puppet.run_mode.master? ? :server_datadir : :client_datadir +- +- File.join(Puppet.settings[name], self.class.indirection_name.to_s) +- end +- +- def file_format(path) +- path =~ /\.(\w+)$/ and return $1 +- end +- +- def file_path(request) +- File.join(data_directory, request.key + ".#{serialization_format}") +- end +- +- def latest_path(request) +- files = Dir.glob(File.join(data_directory, request.key + ".*")) +- return nil if files.empty? +- +- # Return the newest file. +- files.sort { |a, b| File.stat(b).mtime <=> File.stat(a).mtime }[0] +- end +- +- def serialization_format +- model.default_format +- end +- +- # Remove files on disk. +- def destroy(request) +- begin +- removed = false +- Dir.glob(File.join(data_directory, request.key.to_s + ".*")).each do |file| +- removed = true +- File.unlink(file) +- end +- rescue => detail +- raise Puppet::Error, "Could not remove #{request.key}: #{detail}" +- end +- +- raise Puppet::Error, "Could not find files for #{request.key} to remove" unless removed +- end +- +- # Return a model instance for a given file on disk. +- def find(request) +- return nil unless path = latest_path(request) +- format = file_format(path) +- +- raise ArgumentError, "File format #{format} is not supported by #{self.class.indirection_name}" unless model.support_format?(format) +- +- begin +- return model.convert_from(format, File.read(path)) +- rescue => detail +- raise Puppet::Error, "Could not convert path #{path} into a #{self.class.indirection_name}: #{detail}" +- end +- end +- +- # Save a new file to disk. +- def save(request) +- path = file_path(request) +- +- dir = File.dirname(path) +- +- raise Puppet::Error.new("Cannot save #{request.key}; parent directory #{dir} does not exist") unless File.directory?(dir) +- +- begin +- File.open(path, "w") { |f| f.print request.instance.render(serialization_format) } +- rescue => detail +- raise Puppet::Error, "Could not write #{request.key}: #{detail}" % [request.key, detail] +- end +- end +- +- def path(key) +- key +- end +-end +diff --git a/spec/unit/indirector/file_spec.rb b/spec/unit/indirector/file_spec.rb +deleted file mode 100755 +index 86673f0..0000000 +--- a/spec/unit/indirector/file_spec.rb ++++ /dev/null +@@ -1,181 +0,0 @@ +-#!/usr/bin/env ruby +- +-require File.dirname(__FILE__) + '/../../spec_helper' +-require 'puppet/indirector/file' +- +- +-describe Puppet::Indirector::File do +- before :each do +- Puppet::Indirector::Terminus.stubs(:register_terminus_class) +- @model = mock 'model' +- @indirection = stub 'indirection', :name => :mystuff, :register_terminus_type => nil, :model => @model +- Puppet::Indirector::Indirection.stubs(:instance).returns(@indirection) +- +- @file_class = Class.new(Puppet::Indirector::File) do +- def self.to_s +- "Testing::Mytype" +- end +- end +- +- @searcher = @file_class.new +- +- @path = "/my/file" +- @dir = "/my" +- +- @request = stub 'request', :key => @path +- end +- +- describe "when finding files" do +- it "should provide a method to return file contents at a specified path" do +- @searcher.should respond_to(:find) +- end +- +- it "should use the server data directory plus the indirection name if the run_mode is master" do +- Puppet.run_mode.expects(:master?).returns true +- Puppet.settings.expects(:value).with(:server_datadir).returns "/my/dir" +- +- @searcher.data_directory.should == File.join("/my/dir", "mystuff") +- end +- +- it "should use the client data directory plus the indirection name if the run_mode is not master" do +- Puppet.run_mode.expects(:master?).returns false +- Puppet.settings.expects(:value).with(:client_datadir).returns "/my/dir" +- +- @searcher.data_directory.should == File.join("/my/dir", "mystuff") +- end +- +- it "should use the newest file in the data directory matching the indirection key without extension" do +- @searcher.expects(:data_directory).returns "/data/dir" +- @request.stubs(:key).returns "foo" +- Dir.expects(:glob).with("/data/dir/foo.*").returns %w{/data1.stuff /data2.stuff} +- +- stat1 = stub 'data1', :mtime => (Time.now - 5) +- stat2 = stub 'data2', :mtime => Time.now +- File.expects(:stat).with("/data1.stuff").returns stat1 +- File.expects(:stat).with("/data2.stuff").returns stat2 +- +- @searcher.latest_path(@request).should == "/data2.stuff" +- end +- +- it "should return nil when no files are found" do +- @searcher.stubs(:latest_path).returns nil +- +- @searcher.find(@request).should be_nil +- end +- +- it "should determine the file format from the file extension" do +- @searcher.file_format("/data2.pson").should == "pson" +- end +- +- it "should fail if the model does not support the file format" do +- @searcher.stubs(:latest_path).returns "/my/file.pson" +- +- @model.expects(:support_format?).with("pson").returns false +- +- lambda { @searcher.find(@request) }.should raise_error(ArgumentError) +- end +- end +- +- describe "when saving files" do +- before do +- @content = "my content" +- @file = stub 'file', :content => @content, :path => @path, :name => @path, :render => "mydata" +- @request.stubs(:instance).returns @file +- end +- +- it "should provide a method to save file contents at a specified path" do +- @searcher.should respond_to(:save) +- end +- +- it "should choose the file extension based on the default format of the model" do +- @model.expects(:default_format).returns "pson" +- +- @searcher.serialization_format.should == "pson" +- end +- +- it "should place the file in the data directory, named after the indirection, key, and format" do +- @searcher.stubs(:data_directory).returns "/my/dir" +- @searcher.stubs(:serialization_format).returns "pson" +- +- @request.stubs(:key).returns "foo" +- @searcher.file_path(@request).should == File.join("/my/dir", "foo.pson") +- end +- +- it "should fail intelligently if the file's parent directory does not exist" do +- @searcher.stubs(:file_path).returns "/my/dir/file.pson" +- @searcher.stubs(:serialization_format).returns "pson" +- +- @request.stubs(:key).returns "foo" +- File.expects(:directory?).with(File.join("/my/dir")).returns(false) +- +- proc { @searcher.save(@request) }.should raise_error(Puppet::Error) +- end +- +- it "should render the instance using the file format and print it to the file path" do +- @searcher.stubs(:file_path).returns "/my/file.pson" +- @searcher.stubs(:serialization_format).returns "pson" +- +- File.stubs(:directory?).returns true +- +- @request.instance.expects(:render).with("pson").returns "data" +- +- fh = mock 'filehandle' +- File.expects(:open).with("/my/file.pson", "w").yields fh +- fh.expects(:print).with("data") +- +- @searcher.save(@request) +- end +- +- it "should fail intelligently if a file cannot be written" do +- filehandle = mock 'file' +- File.stubs(:directory?).returns(true) +- File.stubs(:open).yields(filehandle) +- filehandle.expects(:print).raises(ArgumentError) +- +- @searcher.stubs(:file_path).returns "/my/file.pson" +- @model.stubs(:default_format).returns "pson" +- +- @instance.stubs(:render).returns "stuff" +- +- proc { @searcher.save(@request) }.should raise_error(Puppet::Error) +- end +- end +- +- describe "when removing files" do +- it "should provide a method to remove files" do +- @searcher.should respond_to(:destroy) +- end +- +- it "should remove files in all formats found in the data directory that match the request key" do +- @searcher.stubs(:data_directory).returns "/my/dir" +- @request.stubs(:key).returns "me" +- +- Dir.expects(:glob).with(File.join("/my/dir", "me.*")).returns %w{/one /two} +- +- File.expects(:unlink).with("/one") +- File.expects(:unlink).with("/two") +- +- @searcher.destroy(@request) +- end +- +- it "should throw an exception if no file is found" do +- @searcher.stubs(:data_directory).returns "/my/dir" +- @request.stubs(:key).returns "me" +- +- Dir.expects(:glob).with(File.join("/my/dir", "me.*")).returns [] +- +- proc { @searcher.destroy(@request) }.should raise_error(Puppet::Error) +- end +- +- it "should fail intelligently if a file cannot be removed" do +- @searcher.stubs(:data_directory).returns "/my/dir" +- @request.stubs(:key).returns "me" +- +- Dir.expects(:glob).with(File.join("/my/dir", "me.*")).returns %w{/one} +- +- File.expects(:unlink).with("/one").raises ArgumentError +- +- proc { @searcher.destroy(@request) }.should raise_error(Puppet::Error) +- end +- end +-end +diff --git a/spec/unit/indirector/terminus_spec.rb b/spec/unit/indirector/terminus_spec.rb +index 826b934..63cb7cc 100755 +--- a/spec/unit/indirector/terminus_spec.rb ++++ b/spec/unit/indirector/terminus_spec.rb +@@ -3,7 +3,7 @@ + require File.dirname(__FILE__) + '/../../spec_helper' + require 'puppet/defaults' + require 'puppet/indirector' +-require 'puppet/indirector/file' ++require 'puppet/indirector/memory' + + describe Puppet::Indirector::Terminus do + before :each do +@@ -202,14 +202,14 @@ describe Puppet::Indirector::Terminus, " when parsing class constants for indire + @subclass.expects(:indirection=).with(:test_ind) + @subclass.stubs(:name=) + @subclass.stubs(:terminus_type=) +- Puppet::Indirector::File.inherited(@subclass) ++ Puppet::Indirector::Memory.inherited(@subclass) + end + + it "should convert the indirection name to a downcased symbol" do + @subclass.expects(:indirection=).with(:test_ind) + @subclass.stubs(:name=) + @subclass.stubs(:terminus_type=) +- Puppet::Indirector::File.inherited(@subclass) ++ Puppet::Indirector::Memory.inherited(@subclass) + end + + it "should convert camel case to lower case with underscores as word separators" do +-- +1.7.6.4 + diff --git a/2.6.x-9794-k5login-can-overwrite-arbitrary-files-as-root.patch b/2.6.x-9794-k5login-can-overwrite-arbitrary-files-as-root.patch new file mode 100644 index 0000000..7fe08d6 --- /dev/null +++ b/2.6.x-9794-k5login-can-overwrite-arbitrary-files-as-root.patch @@ -0,0 +1,40 @@ +From bdf728edc4c0b0e0e416f9d3e542b6815a4d3c0a Mon Sep 17 00:00:00 2001 +From: Daniel Pittman +Date: Thu, 29 Sep 2011 00:32:49 -0700 +Subject: [PATCH] (#9794) k5login can overwrite arbitrary files as root + +The k5login type is typically used to manage a file in the home directory of a +user; the explicit purpose of the files is to allow access to other users. + +It writes to the target file directly, as root, without doing anything to +secure the file. That would allow the owner of the home directory to symlink +to anything on the system, and have it replaced with the correct content of +the file. Which is a fairly obvious escalation to root the next time Puppet +runs. + +Now, instead, fix that to securely write the target file in a predictable and +secure fashion, using the `secure_open` helper. + +Signed-off-by: Daniel Pittman +--- + lib/puppet/type/k5login.rb | 4 +++- + 1 files changed, 3 insertions(+), 1 deletions(-) + +diff --git a/lib/puppet/type/k5login.rb b/lib/puppet/type/k5login.rb +index eac142f..2e87ca9 100644 +--- a/lib/puppet/type/k5login.rb ++++ b/lib/puppet/type/k5login.rb +@@ -79,7 +79,9 @@ Puppet::Type.newtype(:k5login) do + + private + def write(value) +- File.open(@resource[:name], "w") { |f| f.puts value.join("\n") } ++ Puppet::Util.secure_open(@resource[:name], "w") do |f| ++ f.puts value.join("\n") ++ end + end + end + end +-- +1.7.6.4 + diff --git a/puppet.spec b/puppet.spec index 4e18bf9..1add98f 100644 --- a/puppet.spec +++ b/puppet.spec @@ -6,7 +6,7 @@ Name: puppet Version: 2.6.6 -Release: 2%{?dist} +Release: 3%{?dist} Summary: A network tool for managing many disparate systems License: GPLv2 URL: http://puppetlabs.com @@ -20,6 +20,17 @@ Patch1: 0001-4922-Don-t-truncate-remotely-sourced-files-on-404.patch Patch2: 0001-5073-Download-plugins-even-if-you-re-filtering-on-ta.patch # http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-3848 Patch3: 0001-Resist-directory-traversal-attacks-2.6.x.patch +# http://projects.puppetlabs.com/issues/9791 +# http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-3870 +Patch4: 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch +# http://projects.puppetlabs.com/issues/9792 +# http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-3871 +Patch5: 2.6.x-9792-Predictable-temporary-filename-in-ralsh.patch +# http://projects.puppetlabs.com/issues/9794 +# http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-3869 +Patch6: 2.6.x-9794-k5login-can-overwrite-arbitrary-files-as-root.patch +# http://projects.puppetlabs.com/issues/9793 +Patch7: 2.6.x-9793-secure-indirector-file-backed-terminus-base-cla.patch Group: System Environment/Base @@ -78,6 +89,10 @@ The server can also function as a certificate authority and file server. %patch1 -p1 %patch2 -p1 %patch3 -p1 +%patch4 -p1 +%patch5 -p1 +%patch6 -p1 +%patch7 -p1 patch -s -p1 < conf/redhat/rundir-perms.patch @@ -265,6 +280,10 @@ fi rm -rf %{buildroot} %changelog +* Thu Sep 29 2011 Todd Zullinger - 2.6.6-3 +- Apply upstream patches for CVE-2011-3869, CVE-2011-3870, CVE-2011-3871, and + upstream #9793 + * Tue Sep 27 2011 Todd Zullinger - 2.6.6-2 - Apply upstream patch for CVE-2011-3848