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".

lib/puppet/ssl/certificate_authority.rb

@@ -300,6 +300,17 @@ def check_internal_signing_policies(hostname, csr, allow_dns_alt_names)
       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

spec/unit/ssl/certificate_authority_spec.rb

@@ -246,7 +246,7 @@ def stub_ca_host
       # 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
@@ -303,28 +303,28 @@ def stub_ca_host
 
       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
@@ -452,11 +452,76 @@ def stub_ca_host
         @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/
         )
@@ -466,7 +531,7 @@ def stub_ca_host
         @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/
         )
@@ -479,15 +544,15 @@ def stub_ca_host
                                                      { "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/
         )
@@ -497,15 +562,15 @@ def stub_ca_host
         @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/
         )