CVE-2012-3867: Permissions, Privileges, and Access Controls (CWE-264)
value: lib/puppet/ssl/certificate_authority.rb in Puppet before 2.6.17 and 2.7.x before 2.7.18, and Puppet Enterprise before 2.5.2, does not properly restrict the characters in the Common Name field of a Certificate Signing Request (CSR), which makes it easier for user-assisted remote attackers to trick administrators into signing a crafted agent certificate via ANSI control sequences.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
Published: 2012-08-06T16:55Z
Repository: https://github.com/puppetlabs/puppet
Language: Ruby
Files changed: lib/puppet/ssl/certificate_authority.rb, spec/unit/ssl/certificate_authority_spec.rb
Commit date: 2012-06-26 17:12:28 -0700
Commit message: Validate CSR CN and provided certname before signing

This adds a few new checks when signing CSRs, to validate the CN. First,
it must conform to a small set of characters, which are the printable
ASCII characters, except for '/' (because we store these in files). This
prevents attacks such as a CN "foo^H^H^Hbar", which appears as "bar" to
"puppet cert list".

The other check is that the certname for the SSL::Host that we think
we're signing must match the CN. This prevents a CSR with the CN "foo"
from being submitted as a CSR for "bar", which would cause it to appear
as "bar" to "puppet cert list", but to issue a certificate for "foo".

Diff: diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/ssl/certificate_authority.rb
index 179acd9984..69c8ebb179 100644
--- a/lib/puppet/ssl/certificate_authority.rb
+++ b/lib/puppet/ssl/certificate_authority.rb
@@ -301,10 +301,21 @@ class Puppet::SSL::CertificateAuthority
     if unknown_req and not unknown_req.empty?
       names = unknown_req.map {|x| x["oid"] }.sort.uniq.join(", ")
       raise CertificateSigningError.new(hostname), "CSR has request extensions that are not permitted: #{names}"
     end
 
+    # Do not sign misleading CSRs
+    cn = csr.content.subject.to_a.assoc("CN")[1]
+    if hostname != cn
+      raise CertificateSigningError.new(hostname), "CSR subject common name #{cn.inspect} does not match expected certname #{hostname.inspect}"
+    end
+
+    # Only allow printing ascii characters, excluding /
+    if hostname !~ /\A[ -.0-~]+\Z/
+      raise CertificateSigningError.new(hostname), "CSR #{hostname.inspect} subject contains unprintable or non-ASCII characters"
+    end
+
     # Wildcards: we don't allow 'em at any point.
     #
     # The stringification here makes the content visible, and saves us having
     # to scrobble through the content of the CSR subject field to make sure it
     # is what we expect where we expect it.
diff --git a/spec/unit/ssl/certificate_authority_spec.rb b/spec/unit/ssl/certificate_authority_spec.rb
index 222b72e5fd..7fddeb3bcb 100755
--- a/spec/unit/ssl/certificate_authority_spec.rb
+++ b/spec/unit/ssl/certificate_authority_spec.rb
@@ -244,11 +244,11 @@ describe Puppet::SSL::CertificateAuthority do
       Puppet::SSL::Certificate.indirection.stubs(:save)
 
       # Stub out the factory
       Puppet::SSL::CertificateFactory.stubs(:build).returns "my real cert"
 
-      @request_content = stub "request content stub", :subject => @name
+      @request_content = stub "request content stub", :subject => OpenSSL::X509::Name.new([['CN', @name]])
       @request = stub 'request', :name => @name, :request_extensions => [], :subject_alt_names => [], :content => @request_content
 
       # And the inventory
       @inventory = stub 'inventory', :add => nil
       @ca.stubs(:inventory).returns @inventory
@@ -301,32 +301,32 @@ describe Puppet::SSL::CertificateAuthority do
         @ca.sign(@name, true, @request)
       end
 
       it "should use a certificate type of :ca" do
         Puppet::SSL::CertificateFactory.expects(:build).with do |*args|
-          args[0] == :ca
+          args[0].should == :ca
         end.returns "my real cert"
         @ca.sign(@name, :ca, @request)
       end
 
       it "should pass the provided CSR as the CSR" do
         Puppet::SSL::CertificateFactory.expects(:build).with do |*args|
-          args[1] == @request
+          args[1].should == @request
         end.returns "my real cert"
         @ca.sign(@name, :ca, @request)
       end
 
       it "should use the provided CSR's content as the issuer" do
         Puppet::SSL::CertificateFactory.expects(:build).with do |*args|
-          args[2].subject == "myhost"
+          args[2].subject.to_s.should == "/CN=myhost"
         end.returns "my real cert"
         @ca.sign(@name, :ca, @request)
       end
 
       it "should pass the next serial as the serial number" do
         Puppet::SSL::CertificateFactory.expects(:build).with do |*args|
-          args[3] == @serial
+          args[3].should == @serial
         end.returns "my real cert"
         @ca.sign(@name, :ca, @request)
       end
 
       it "should sign the certificate request even if it contains alt names" do
@@ -450,25 +450,90 @@ describe Puppet::SSL::CertificateAuthority do
 
         Puppet::SSL::CertificateRequest.indirection.stubs(:find).with(@name).returns @request
         @cert.stubs :save
       end
 
+      it "should reject CSRs whose CN doesn't match the name for which we're signing them" do
+        # Shorten this so the test doesn't take too long
+        Puppet[:keylength] = 1024
+        key = Puppet::SSL::Key.new('the_certname')
+        key.generate
+
+        csr = Puppet::SSL::CertificateRequest.new('the_certname')
+        csr.generate(key)
+
+        expect do
+          @ca.check_internal_signing_policies('not_the_certname', csr, false)
+        end.to raise_error(
+          Puppet::SSL::CertificateAuthority::CertificateSigningError,
+          /common name "the_certname" does not match expected certname "not_the_certname"/
+        )
+      end
+
+      describe "when validating the CN" do
+        before :all do
+          Puppet[:keylength] = 1024
+          @signing_key = Puppet::SSL::Key.new('my_signing_key')
+          @signing_key.generate
+        end
+
+        [
+         'completely_okay',
+         'sure, why not? :)',
+         'so+many(things)-are=allowed.',
+         'this"is#just&madness%you[see]',
+         'and even a (an?) \\!',
+         'waltz, nymph, for quick jigs vex bud.',
+         '{552c04ca-bb1b-11e1-874b-60334b04494e}'
+        ].each do |name|
+          it "should accept #{name.inspect}" do
+            csr = Puppet::SSL::CertificateRequest.new(name)
+            csr.generate(@signing_key)
+
+            @ca.check_internal_signing_policies(name, csr, false)
+          end
+        end
+
+        [
+         'super/bad',
+         "not\neven\tkind\rof",
+         "ding\adong\a",
+         "hidden\b\b\b\b\b\bmessage",
+         "☃ :("
+        ].each do |name|
+          it "should reject #{name.inspect}" do
+            # We aren't even allowed to make objects with these names, so let's
+            # stub that to simulate an invalid one coming from outside Puppet
+            Puppet::SSL::CertificateRequest.stubs(:validate_certname)
+            csr = Puppet::SSL::CertificateRequest.new(name)
+            csr.generate(@signing_key)
+
+            expect do
+              @ca.check_internal_signing_policies(name, csr, false)
+            end.to raise_error(
+              Puppet::SSL::CertificateAuthority::CertificateSigningError,
+              /subject contains unprintable or non-ASCII characters/
+            )
+          end
+        end
+      end
+
       it "should reject a critical extension that isn't on the whitelist" do
         @request.stubs(:request_extensions).returns [{ "oid" => "banana",
                                                        "value" => "yumm",
                                                        "critical" => true }]
-        expect { @ca.sign(@name) }.to raise_error(
+        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /request extensions that are not permitted/
         )
       end
 
       it "should reject a non-critical extension that isn't on the whitelist" do
         @request.stubs(:request_extensions).returns [{ "oid" => "peach",
                                                        "value" => "meh",
                                                        "critical" => false }]
-        expect { @ca.sign(@name) }.to raise_error(
+        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /request extensions that are not permitted/
         )
       end
 
@@ -477,37 +542,37 @@ describe Puppet::SSL::CertificateAuthority do
                                                        "value" => "meh",
                                                        "critical" => false },
                                                      { "oid" => "subjectAltName",
                                                        "value" => "DNS:foo",
                                                        "critical" => true }]
-        expect { @ca.sign(@name) }.to raise_error(
+        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /request extensions that are not permitted/
         )
       end
 
       it "should reject a subjectAltName for a non-DNS value" do
         @request.stubs(:subject_alt_names).returns ['DNS:foo', 'email:bar@example.com']
-        expect { @ca.sign(@name, true) }.to raise_error(
+        expect { @ca.check_internal_signing_policies(@name, @request, true) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /subjectAltName outside the DNS label space/
         )
       end
 
       it "should reject a wildcard subject" do
         @request.content.stubs(:subject).
           returns(OpenSSL::X509::Name.new([["CN", "*.local"]]))
 
-        expect { @ca.sign(@name) }.to raise_error(
+        expect { @ca.check_internal_signing_policies('*.local', @request, false) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /subject contains a wildcard/
         )
       end
 
       it "should reject a wildcard subjectAltName" do
         @request.stubs(:subject_alt_names).returns ['DNS:foo', 'DNS:*.bar']
-        expect { @ca.sign(@name, true) }.to raise_error(
+        expect { @ca.check_internal_signing_policies(@name, @request, true) }.to raise_error(
           Puppet::SSL::CertificateAuthority::CertificateSigningError,
           /subjectAltName contains a wildcard/
         )
       end
     end

Vulnerable code:       @request_content = stub "request content stub", :subject => @name
          args[0] == :ca
          args[1] == @request
          args[2].subject == "myhost"
          args[3] == @serial
        expect { @ca.sign(@name) }.to raise_error(
        expect { @ca.sign(@name) }.to raise_error(
        expect { @ca.sign(@name) }.to raise_error(
        expect { @ca.sign(@name, true) }.to raise_error(
        expect { @ca.sign(@name) }.to raise_error(
        expect { @ca.sign(@name, true) }.to raise_error(
Fixed code:     # Do not sign misleading CSRs
    cn = csr.content.subject.to_a.assoc("CN")[1]
    if hostname != cn
      raise CertificateSigningError.new(hostname), "CSR subject common name #{cn.inspect} does not match expected certname #{hostname.inspect}"
    end

    # Only allow printing ascii characters, excluding /
    if hostname !~ /\A[ -.0-~]+\Z/
      raise CertificateSigningError.new(hostname), "CSR #{hostname.inspect} subject contains unprintable or non-ASCII characters"
    end

      @request_content = stub "request content stub", :subject => OpenSSL::X509::Name.new([['CN', @name]])
          args[0].should == :ca
          args[1].should == @request
          args[2].subject.to_s.should == "/CN=myhost"
          args[3].should == @serial
      it "should reject CSRs whose CN doesn't match the name for which we're signing them" do
        # Shorten this so the test doesn't take too long
        Puppet[:keylength] = 1024
        key = Puppet::SSL::Key.new('the_certname')
        key.generate

        csr = Puppet::SSL::CertificateRequest.new('the_certname')
        csr.generate(key)

        expect do
          @ca.check_internal_signing_policies('not_the_certname', csr, false)
        end.to raise_error(
          Puppet::SSL::CertificateAuthority::CertificateSigningError,
          /common name "the_certname" does not match expected certname "not_the_certname"/
        )
      end

      describe "when validating the CN" do
        before :all do
          Puppet[:keylength] = 1024
          @signing_key = Puppet::SSL::Key.new('my_signing_key')
          @signing_key.generate
        end

        [
         'completely_okay',
         'sure, why not? :)',
         'so+many(things)-are=allowed.',
         'this"is#just&madness%you[see]',
         'and even a (an?) \\!',
         'waltz, nymph, for quick jigs vex bud.',
         '{552c04ca-bb1b-11e1-874b-60334b04494e}'
        ].each do |name|
          it "should accept #{name.inspect}" do
            csr = Puppet::SSL::CertificateRequest.new(name)
            csr.generate(@signing_key)

            @ca.check_internal_signing_policies(name, csr, false)
          end
        end

        [
         'super/bad',
         "not\neven\tkind\rof",
         "ding\adong\a",
         "hidden\b\b\b\b\b\bmessage",
         "☃ :("
        ].each do |name|
          it "should reject #{name.inspect}" do
            # We aren't even allowed to make objects with these names, so let's
            # stub that to simulate an invalid one coming from outside Puppet
            Puppet::SSL::CertificateRequest.stubs(:validate_certname)
            csr = Puppet::SSL::CertificateRequest.new(name)
            csr.generate(@signing_key)

            expect do
              @ca.check_internal_signing_policies(name, csr, false)
            end.to raise_error(
              Puppet::SSL::CertificateAuthority::CertificateSigningError,
              /subject contains unprintable or non-ASCII characters/
            )
          end
        end
      end

        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
        expect { @ca.check_internal_signing_policies(@name, @request, false) }.to raise_error(
        expect { @ca.check_internal_signing_policies(@name, @request, true) }.to raise_error(
        expect { @ca.check_internal_signing_policies('*.local', @request, false) }.to raise_error(
        expect { @ca.check_internal_signing_policies(@name, @request, true) }.to raise_error(