CVE-2017-2298: Improper Input Validation (CWE-20)
value: The mcollective-sshkey-security plugin before 0.5.1 for Puppet uses a server-specified identifier as part of a path where a file is written. A compromised server could use this to write a file to an arbitrary location on the client with the filename appended with the string "_pub.pem".
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 6.5
Published: 2017-06-30T20:29Z
Repository: https://github.com/puppetlabs/mcollective-sshkey-security
Language: Ruby
Files changed: security/sshkey.rb, spec/security/sshkey_spec.rb
Commit date: 2017-06-29 14:16:04 -0700
Commit message: (MCOP-600) Prevent public key overwriting attack via identity

When using two-way automatic public key distribution, each end writes
the others `identity` as a public key file locally. No validation was
done on the `identity`, so it could trigger directory traversal and
allow the attacker to overwrite an unexpected file (like a trusted
public key certificate). Prevent this by verifying identity does not
result in traversing outside the intended distribution directory.

Diff: diff --git a/security/sshkey.rb b/security/sshkey.rb
index eb3f0d0..7cf024d 100644
--- a/security/sshkey.rb
+++ b/security/sshkey.rb
@@ -140,24 +140,31 @@ module MCollective
           Log.info("Public key sent with request but no publickey_dir defined in configuration. Not writing key to disk.")
           return
         end
 
         if File.directory?(publickey_dir)
-          if File.exists?(old_keyfile = File.join(publickey_dir, "#{identity}_pub.pem"))
+          # Reject identity if it would result in directory traversal.
+          old_keyfile = File.join(File.expand_path(publickey_dir), "#{identity}_pub.pem")
+          unless File.expand_path(old_keyfile) == old_keyfile
+            Log.warn("Identity returned by server would result in directory traversal. Not writing key to disk.")
+            return
+          end
+
+          if File.exists?(old_keyfile)
             old_key = File.read(old_keyfile).chomp
 
             unless old_key == key
               unless lookup_config_option('overwrite_stored_keys', 'n') =~ /^1|y/
                 Log.warn("Public key sent from '%s' does not match the stored key. Not overwriting." % identity)
               else
                 Log.warn("Public key sent from '%s' does not match the stored key. Overwriting." % identity)
-                File.open(File.join(publickey_dir, "#{identity}_pub.pem"), 'w') { |f| f.puts key }
+                File.open(old_keyfile, 'w') { |f| f.puts key }
               end
             end
           else
             Log.debug("Discovered a new public key for '%s'. Writing to '%s'" % [identity, publickey_dir])
-            File.open(File.join(publickey_dir, "#{identity}_pub.pem"), 'w') { |f| f.puts key }
+            File.open(old_keyfile, 'w') { |f| f.puts key }
           end
         else
           raise("Cannot write public key to '%s'. Directory does not exist." % publickey_dir)
         end
       end
diff --git a/spec/security/sshkey_spec.rb b/spec/security/sshkey_spec.rb
index f6a6ec7..09e8030 100644
--- a/spec/security/sshkey_spec.rb
+++ b/spec/security/sshkey_spec.rb
@@ -278,42 +278,54 @@ module MCollective
           expect{
             @plugin.send(:write_key_to_disk, '', '')
           }.to raise_error
         end
 
+        it 'should fail if identity would result in directory traversal' do
+          @plugin.stubs(:lookup_config_option).with('learn_public_keys').returns('1')
+          @plugin.stubs(:lookup_config_option).with('publickey_dir').returns('ssh/pkd')
+          File.stubs(:directory?).with('ssh/pkd').returns(true)
+          Log.expects(:warn)
+          File.expects(:open).never
+          @plugin.send(:write_key_to_disk, 'ssh-rsa abcd', '../test')
+        end
+
         it 'should write the public key to disk if its the first time its been seen' do
           @plugin.stubs(:lookup_config_option).with('learn_public_keys').returns('1')
           @plugin.stubs(:lookup_config_option).with('publickey_dir').returns('ssh/pkd')
           File.stubs(:directory?).with('ssh/pkd').returns(true)
-          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(false)
+          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
+          File.stubs(:exists?).with(full_path).returns(false)
           file = mock
-          File.expects(:open).with('ssh/pkd/rspec_pub.pem', 'w').yields(file)
+          File.expects(:open).with(full_path, 'w').yields(file)
           file.expects(:puts).with('ssh-rsa abcd')
           @plugin.send(:write_key_to_disk, 'ssh-rsa abcd', 'rspec')
         end
 
         it 'should not overwrite an existing file if overwrite_stored_key is not set' do
           @plugin.stubs(:lookup_config_option).with('learn_public_keys').returns('1')
           @plugin.stubs(:lookup_config_option).with('publickey_dir').returns('ssh/pkd')
           @plugin.stubs(:lookup_config_option).with('overwrite_stored_keys', 'n').returns('n')
           File.stubs(:directory?).with('ssh/pkd').returns(true)
-          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(true)
-          File.stubs(:read).with('ssh/pkd/rspec_pub.pem').returns('ssh-rsa dcba')
+          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
+          File.stubs(:exists?).with(full_path).returns(true)
+          File.stubs(:read).with(full_path).returns('ssh-rsa dcba')
           Log.expects(:warn)
           File.expects(:open).never
           @plugin.send(:write_key_to_disk, 'ssh-rsa abcd', 'rspec')
         end
 
         it 'should overwrite the existing public key if overwrite_stored_key is set' do
           @plugin.stubs(:lookup_config_option).with('learn_public_keys').returns('1')
           @plugin.stubs(:lookup_config_option).with('publickey_dir').returns('ssh/pkd')
           @plugin.stubs(:lookup_config_option).with('overwrite_stored_keys', 'n').returns('1')
           File.stubs(:directory?).with('ssh/pkd').returns(true)
-          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(true)
-          File.stubs(:read).with('ssh/pkd/rspec_pub.pem').returns('ssh-rsa dcba')
+          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
+          File.stubs(:exists?).with(full_path).returns(true)
+          File.stubs(:read).with(full_path).returns('ssh-rsa dcba')
           file = mock
-          File.expects(:open).with('ssh/pkd/rspec_pub.pem', 'w').yields(file)
+          File.expects(:open).with(full_path, 'w').yields(file)
           file.expects(:puts).with('ssh-rsa abcd')
           Log.expects(:warn)
           @plugin.send(:write_key_to_disk, 'ssh-rsa abcd', 'rspec')
         end
       end

Vulnerable code:           if File.exists?(old_keyfile = File.join(publickey_dir, "#{identity}_pub.pem"))
                File.open(File.join(publickey_dir, "#{identity}_pub.pem"), 'w') { |f| f.puts key }
            File.open(File.join(publickey_dir, "#{identity}_pub.pem"), 'w') { |f| f.puts key }
          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(false)
          File.expects(:open).with('ssh/pkd/rspec_pub.pem', 'w').yields(file)
          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(true)
          File.stubs(:read).with('ssh/pkd/rspec_pub.pem').returns('ssh-rsa dcba')
          File.stubs(:exists?).with('ssh/pkd/rspec_pub.pem').returns(true)
          File.stubs(:read).with('ssh/pkd/rspec_pub.pem').returns('ssh-rsa dcba')
          File.expects(:open).with('ssh/pkd/rspec_pub.pem', 'w').yields(file)
Fixed code:           # Reject identity if it would result in directory traversal.
          old_keyfile = File.join(File.expand_path(publickey_dir), "#{identity}_pub.pem")
          unless File.expand_path(old_keyfile) == old_keyfile
            Log.warn("Identity returned by server would result in directory traversal. Not writing key to disk.")
            return
          end

          if File.exists?(old_keyfile)
                File.open(old_keyfile, 'w') { |f| f.puts key }
            File.open(old_keyfile, 'w') { |f| f.puts key }
        it 'should fail if identity would result in directory traversal' do
          @plugin.stubs(:lookup_config_option).with('learn_public_keys').returns('1')
          @plugin.stubs(:lookup_config_option).with('publickey_dir').returns('ssh/pkd')
          File.stubs(:directory?).with('ssh/pkd').returns(true)
          Log.expects(:warn)
          File.expects(:open).never
          @plugin.send(:write_key_to_disk, 'ssh-rsa abcd', '../test')
        end

          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
          File.stubs(:exists?).with(full_path).returns(false)
          File.expects(:open).with(full_path, 'w').yields(file)
          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
          File.stubs(:exists?).with(full_path).returns(true)
          File.stubs(:read).with(full_path).returns('ssh-rsa dcba')
          full_path = File.join(File.expand_path('ssh/pkd'), 'rspec_pub.pem')
          File.stubs(:exists?).with(full_path).returns(true)
          File.stubs(:read).with(full_path).returns('ssh-rsa dcba')
          File.expects(:open).with(full_path, 'w').yields(file)