Attachment 916998 Details for Bug 1105713 – mcollective-2.5.2-flaw-in-aes_security.patch

[patch] mcollective-2.5.2-flaw-in-aes_security.patch

mcollective-2.5.2-flaw-in-aes_security.patch (text/plain), 14.31 KB, created by Kurt Seifried on 2014-07-10 06:06:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kurt Seifried

Created: 2014-07-10 06:06:52 UTC

Size: 14.31 KB

patch

obsolete

>From 31689058c85fe11020f99ef897299a4d62f5202f Mon Sep 17 00:00:00 2001
>From: Pieter Loubser <ploubser@gmail.com>
>Date: Wed, 28 May 2014 17:24:57 +0100
>Subject: [PATCH] (MCO-329) - Potential flaw in aes_security
>
>In the past there existed a possible security flaw when enabling
>learn_pubkeys on a MCollective server. Keys distributed in this manner
>weren't verified at all, allowing for the possibility of a rogue key
>being cached.
>
>This commit increases the default level of security when using
>learn_pubkeys. When enabling this configuration the client_public key
>configured must be a X509 certificate while on the server side a CA cert
>must be configured with the aes.ca_cert configuration option. The cert
>will now be verified by the CA before writing it to disk.
>
>The previous functionality can be restored by setting
>aes.insecure_learning but be aware that enabling this should not be
>configured in sensitive environments.
>
>This commit also adds test coverage for Aes_security#decodemsg
>
>diff --git a/plugins/mcollective/security/aes_security.rb b/plugins/mcollective/security/aes_security.rb
>index 4f2a55c..ccfa9a6 100644
>--- a/plugins/mcollective/security/aes_security.rb
>+++ b/plugins/mcollective/security/aes_security.rb
>@@ -47,9 +47,16 @@ module MCollective
>     #    # Where to cache client keys or find manually distributed ones
>     #    plugin.aes.client_cert_dir = /etc/mcollective/ssl/clients
>     #
>-    #    # Cache public keys promiscuously from the network
>+    #    # Cache public keys promiscuously from the network (this requires either a ca_cert to be set
>+    #      or insecure_learning to be enabled)
>     #    plugin.aes.learn_pubkeys = 1
>     #
>+    #    # Do not check if client certificate can be verified by a CA
>+    #    plugin.aes.insecure_learning = 1
>+    #
>+    #    # CA cert used to verify public keys when in learning mode
>+    #    plugin.aes.ca_cert = /etc/mcollective/ssl/ca.cert
>+    #
>     #    # Log but accept messages that may have been tampered with
>     #    plugin.aes.enforce_ttl = 0
>     #
>@@ -62,22 +69,31 @@ module MCollective
>         body = deserialize(msg.payload)
> 
>         should_process_msg?(msg, body[:requestid])
>-
>         # if we get a message that has a pubkey attached and we're set to learn
>         # then add it to the client_cert_dir this should only happen on servers
>         # since clients will get replies using their own pubkeys
>-        if @config.pluginconf.include?("aes.learn_pubkeys") && @config.pluginconf["aes.learn_pubkeys"] == "1"
>-          if body.include?(:sslpubkey)
>-            if client_cert_dir
>-              certname = certname_from_callerid(body[:callerid])
>-              if certname
>-                certfile = "#{client_cert_dir}/#{certname}.pem"
>-                unless File.exist?(certfile)
>-                  Log.debug("Caching client cert in #{certfile}")
>-                  File.open(certfile, "w") {|f| f.print body[:sslpubkey]}
>-                end
>+        if Util.str_to_bool(@config.pluginconf.fetch("aes.learn_pubkeys", false)) && body.include?(:sslpubkey)
>+          certname = certname_from_callerid(body[:callerid])
>+          certfile = "#{client_cert_dir}/#{certname}.pem"
>+          if !File.exist?(certfile)
>+            if !Util.str_to_bool(@config.pluginconf.fetch("aes.insecure_learning", false))
>+              if !@config.pluginconf.fetch("aes.ca_cert", nil)
>+                raise "Cannot verify certificate for '#{certname}'. No CA certificate specified."
>               end
>+
>+              if !validate_certificate(body[:sslpubkey], certname)
>+                raise "Unable to validate certificate '#{certname}' against CA"
>+              end
>+
>+              Log.debug("Verified certificate '#{certname}' against CA")
>+            else
>+              Log.warn("Insecure key learning is not a secure method of key distribution. Do NOT use this mode in sensitive environments.")
>             end
>+
>+            Log.debug("Caching client cert in #{certfile}")
>+            File.open(certfile, "w") {|f| f.print body[:sslpubkey]}
>+          else
>+            Log.debug("Not caching client cert. File #{certfile} already exists.")
>           end
>         end
> 
>@@ -86,6 +102,12 @@ module MCollective
>         if @initiated_by == :client
>           body[:body] = deserialize(decrypt(cryptdata, nil))
>         else
>+          certname = certname_from_callerid(body[:callerid])
>+          certfile = "#{client_cert_dir}/#{certname}.pem"
>+          # if aes.ca_cert is set every certificate is validated before we try and use it
>+          if @config.pluginconf.fetch("aes.ca_cert", nil) && !validate_certificate(File.read(certfile), certname)
>+            raise "Unable to validate certificate '#{certname}' against CA"
>+          end
>           body[:body] = deserialize(decrypt(cryptdata, body[:callerid]))
> 
>           # If we got a hash it's possible that this is a message with secure
>@@ -212,14 +234,19 @@ module MCollective
>       # sets the caller id to the md5 of the public key
>       def callerid
>         if @initiated_by == :client
>-          id = "cert=#{File.basename(client_public_key).gsub(/\.pem$/, '')}"
>-          raise "Invalid callerid generated from client public key" unless valid_callerid?(id)
>+          key = client_public_key
>         else
>-          # servers need to set callerid as well, not usually needed but
>-          # would be if you're doing registration or auditing or generating
>-          # requests for some or other reason
>-          id = "cert=#{File.basename(server_public_key).gsub(/\.pem$/, '')}"
>-          raise "Invalid callerid generated from server public key" unless valid_callerid?(id)
>+          key = server_public_key
>+        end
>+
>+        # First try and create a X509 certificate object. If that is possible,
>+        # we lift the callerid from the cert
>+        begin
>+          ssl_cert = OpenSSL::X509::Certificate.new(File.read(key))
>+          id = "cert=#{certname_from_certificate(ssl_cert)}"
>+        rescue
>+          # If the public key is not a certificate, use the file name as callerid
>+          id = "cert=#{File.basename(key).gsub(/\.pem$/, '')}"
>         end
> 
>         return id
>@@ -256,12 +283,42 @@ module MCollective
>           return @ssl.decrypt_with_private(string)
>         else
>           Log.debug("Decrypting message using public key for #{certid}")
>-
>           ssl = SSL.new(public_key_path_for_client(certid))
>           return ssl.decrypt_with_public(string)
>         end
>       end
> 
>+      def validate_certificate(client_cert, certid)
>+        cert_file = @config.pluginconf.fetch("aes.ca_cert", nil)
>+
>+        begin
>+          ssl_cert = OpenSSL::X509::Certificate.new(client_cert)
>+        rescue OpenSSL::X509::CertificateError
>+          Log.warn("Received public key that is not a X509 certficate")
>+          return false
>+        end
>+
>+        ssl_certname = certname_from_certificate(ssl_cert)
>+
>+        if certid != ssl_certname
>+          Log.warn("certname '#{certid}' doesn't match certificate '#{ssl_certname}'")
>+          return false
>+        end
>+
>+        Log.debug("Loading CA Cert for verification")
>+        ca_cert = OpenSSL::X509::Store.new
>+        ca_cert.add_file cert_file
>+
>+        if ca_cert.verify(ssl_cert)
>+          Log.debug("Verified certificate '#{ssl_certname}' against CA")
>+        else
>+          # TODO add cert id
>+          Log.warn("Unable to validate certificate '#{ssl_certname}'' against CA")
>+          return false
>+        end
>+        return true
>+      end
>+
>       # On servers this will look in the aes.client_cert_dir for public
>       # keys matching the clientid, clientid is expected to be in the format
>       # set by callerid
>@@ -320,8 +377,16 @@ module MCollective
>         if id =~ /^cert=([\w\.\-]+)/
>           return $1
>         else
>-          Log.warn("Received a callerid in an unexpected format: '#{id}', ignoring")
>-          return nil
>+          raise("Received a callerid in an unexpected format: '#{id}', ignoring")
>+        end
>+      end
>+
>+      def certname_from_certificate(cert)
>+        id = cert.subject
>+        if id.to_s =~ /^\/CN=([\w\.\-]+)/
>+          return $1
>+        else
>+          raise("Received a callerid in an unexpected format in an SSL certificate: '#{id}', ignoring")
>         end
>       end
>     end
>diff --git a/spec/unit/plugins/mcollective/security/aes_security_spec.rb b/spec/unit/plugins/mcollective/security/aes_security_spec.rb
>new file mode 100644
>index 0000000..8105c37
>--- /dev/null
>+++ b/spec/unit/plugins/mcollective/security/aes_security_spec.rb
>@@ -0,0 +1,165 @@
>+#!/usr/bin/env rspec
>+
>+require 'spec_helper'
>+require File.dirname(__FILE__) + '/../../../../../plugins/mcollective/security/aes_security.rb'
>+
>+module MCollective
>+  module Security
>+    # Clear the PluginManager so that security plugin tests do not conflict
>+    PluginManager.clear
>+    describe Aes_security do
>+      let(:pluginconf) do
>+        {"aes.client_cert_dir" => "testing"}
>+      end
>+
>+      let(:config) do
>+        conf = mock
>+        conf.stubs(:identity).returns("test")
>+        conf.stubs(:configured).returns(true)
>+        conf.stubs(:pluginconf).returns(pluginconf)
>+        conf
>+      end
>+
>+      let(:plugin) do
>+        Aes_security.new
>+      end
>+
>+      let(:msg) do
>+        m = mock
>+        m.stubs(:payload)
>+        m
>+      end
>+
>+      before :each do
>+        stats = mock("stats")
>+        MCollective::PluginManager << {:type => "global_stats", :class => stats}
>+        MCollective::Config.stubs("instance").returns(config)
>+        MCollective::Log.stubs(:debug)
>+        MCollective::Log.stubs(:warn)
>+      end
>+
>+      describe "#decodemsg" do
>+        let(:body) do
>+        {:sslpubkey => "ssl_public_key",
>+         :callerid  => "cert=testing",
>+         :requestid => 1}
>+        end
>+
>+        before :each do
>+          pluginconf["aes.learn_pubkeys"] = "1"
>+          plugin.stubs(:should_process_msg?)
>+          plugin.stubs(:deserialize).returns(body)
>+          plugin.stubs(:decrypt)
>+          plugin.stubs(:deserialize).returns(body)
>+          plugin.stubs(:update_secure_property)
>+        end
>+
>+        it "should not learn the public key if the key has not been passed" do
>+          body.delete(:sslpubkey)
>+          plugin.decodemsg(msg)
>+          File.expects(:exist?).never
>+          File.expects(:open).never
>+        end
>+
>+        it "should not learn the public key if keyfile is present on disk" do
>+          File.expects(:exist?).with("testing/testing.pem").returns(true)
>+          File.expects(:open).never
>+          plugin.decodemsg(msg)
>+        end
>+
>+        it "should not learn the key if there is no ca_cert and insecure_learning is false" do
>+          File.expects(:exist?).returns(false)
>+          Log.expects(:warn).with() do |msg|
>+            msg =~ /No CA certificate specified/
>+          end
>+          expect {
>+            plugin.decodemsg(msg)
>+          }.to raise_error SecurityValidationFailed
>+        end
>+
>+        it "should not learn the key if the cert cannot be verified against the CA" do
>+          File.expects(:exist?).returns(false)
>+          pluginconf["aes.ca_cert"] = "ca_cert"
>+          plugin.expects(:validate_certificate).with("ssl_public_key", "testing").returns(false)
>+          Log.expects(:warn).with() do |msg|
>+            msg.should match(/Unable to validate certificate/)
>+          end
>+          expect {
>+            plugin.decodemsg(msg)
>+          }.to raise_error SecurityValidationFailed
>+        end
>+
>+        it "it should learn the public key if insecure_learning is enabled" do
>+          pluginconf["aes.insecure_learning"] = "1"
>+          File.expects(:exist?).returns(false)
>+          Log.expects(:warn).with() do |msg|
>+            msg.should match(/Do NOT use this mode in sensitive environments/)
>+          end
>+          File.expects(:open)
>+          plugin.decodemsg(msg)
>+        end
>+
>+        it "should learn the public key if the CA can verify the cert" do
>+          File.expects(:exist?).returns(false)
>+          pluginconf["aes.ca_cert"] = "ca_cert"
>+          File.expects(:read).with("testing/testing.pem").returns("ssl_public_key")
>+          plugin.expects(:validate_certificate).with("ssl_public_key", "testing").twice.returns(true)
>+          File.expects(:open)
>+          plugin.decodemsg(msg)
>+        end
>+      end
>+
>+      describe "#validate_certificate" do
>+        let(:cert) do
>+          mock
>+        end
>+
>+        let(:ca_cert) do
>+          ca = mock
>+          ca.stubs(:add_file).returns(true)
>+          ca
>+        end
>+
>+        let(:callerid) do
>+          "rspec_caller"
>+        end
>+
>+        it "should fail if the cert is not a X509 certificate" do
>+          OpenSSL::X509::Certificate.expects(:new).with("ssl_cert").raises(OpenSSL::X509::CertificateError)
>+          Log.expects(:warn).with() do |msg|
>+            msg.should match(/Received public key that is not a X509 certficate/)
>+          end
>+          plugin.validate_certificate("ssl_cert", callerid).should be_false
>+        end
>+
>+        it "should fail if the name in the cert doesn't match the callerid" do
>+          OpenSSL::X509::Certificate.expects(:new).with("ssl_cert").returns(cert)
>+          plugin.stubs(:certname_from_certificate).with(cert).returns("not_rspec_caller")
>+          Log.expects(:warn).with() do |msg|
>+            msg.should match(/certname 'rspec_caller' doesn't match certificate 'not_rspec_caller'/)
>+          end
>+          plugin.validate_certificate("ssl_cert", callerid).should be_false
>+        end
>+
>+        it "should fail if the cert wasn't signed by the CA" do
>+          OpenSSL::X509::Certificate.expects(:new).with("ssl_cert").returns(cert)
>+          plugin.stubs(:certname_from_certificate).with(cert).returns("rspec_caller")
>+          OpenSSL::X509::Store.stubs(:new).returns(ca_cert)
>+          ca_cert.stubs(:verify).with(cert).returns(false)
>+          Log.expects(:warn).with() do |msg|
>+            msg.should match(/Unable to validate certificate/)
>+          end
>+          plugin.validate_certificate("ssl_cert", callerid).should be_false
>+        end
>+
>+        it "should validate the cert" do
>+          OpenSSL::X509::Certificate.expects(:new).with("ssl_cert").returns(cert)
>+          plugin.stubs(:certname_from_certificate).with(cert).returns("rspec_caller")
>+          OpenSSL::X509::Store.stubs(:new).returns(ca_cert)
>+          ca_cert.stubs(:verify).with(cert).returns(true)
>+          plugin.validate_certificate("ssl_cert", callerid).should be_true
>+        end
>+      end
>+    end
>+  end
>+end
>-- 
>1.8.4.3
>

Actions: View | Diff

Attachments on bug 1105713: 916998