CVE-2012-3408: Improper Authentication (CWE-287)
value: lib/puppet/network/authstore.rb in Puppet before 2.7.18, and Puppet Enterprise before 2.5.2, supports use of IP addresses in certnames without warning of potential risks, which might allow remote attackers to spoof an agent by acquiring a previously used IP address.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
Published: 2012-08-06T16:55Z
Repository: https://github.com/puppetlabs/puppet
Language: Ruby
Files changed: lib/puppet/network/authstore.rb, spec/integration/network/rest_authconfig_spec.rb
Commit date: 2012-06-26 17:11:37 -0700
Commit message: Deprecate IP-based authentication

This will need to be made explicit in a future version, so it's good to
get people to stop using it if possible.

Diff: diff --git a/lib/puppet/network/authstore.rb b/lib/puppet/network/authstore.rb
index 51fd34138..bd19aeb9c 100755
--- a/lib/puppet/network/authstore.rb
+++ b/lib/puppet/network/authstore.rb
@@ -148,11 +148,20 @@ module Puppet
         name == :ip
       end
 
       # Does this declaration match the name/ip combo?
       def match?(name, ip)
-        ip? ? pattern.include?(IPAddr.new(ip)) : matchname?(name)
+        if ip?
+          if pattern.include?(IPAddr.new(ip))
+            Puppet.deprecation_warning "Authentication based on IP address is deprecated; please use certname-based rules instead"
+            true
+          else
+            false
+          end
+        else
+          matchname?(name)
+        end
       end
 
       # Set the pattern appropriately.  Also sets the name and length.
       def pattern=(pattern)
         parse(pattern)
@@ -210,11 +219,10 @@ module Puppet
         end
       end
 
       # Convert the name to a common pattern.
       def munge_name(name)
-        # LAK:NOTE http://snurl.com/21zf8  [groups_google_com]
         # Change to name.downcase.split(".",-1).reverse for FQDN support
         name.downcase.split(".").reverse
       end
 
       # Parse our input pattern and figure out what kind of allowal
diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
index fb21abddd..129a9550c 100644
--- a/spec/integration/network/rest_authconfig_spec.rb
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -42,16 +42,35 @@ describe Puppet::Network::RestAuthConfig do
     end
     @auth = Puppet::Network::RestAuthConfig.new(Puppet[:rest_authconfig], true)
   end
 
   def request(args = {})
-    { :ip => '10.1.1.1', :node => 'host.domain.com', :key => 'key', :authenticated => true }.each do |k,v|
-      args[k] ||= v
-    end
+    args = {
+      :key => 'key',
+      :node => 'host.domain.com',
+      :ip => '10.1.1.1',
+      :authenticated => true
+    }.merge(args)
     ['test', :find, args[:key], args]
   end
 
+  it "should warn when matching against IP addresses" do
+    add_rule("allow 10.1.1.1")
+
+    @auth.should allow(request)
+
+    @logs.should be_any {|log| log.level == :warning and log.message =~ /Authentication based on IP address is deprecated/}
+  end
+
+  it "should not warn when matches against IP addresses fail" do
+    add_rule("allow 10.1.1.2")
+
+    @auth.should_not allow(request)
+
+    @logs.should_not be_any {|log| log.level == :warning and log.message =~ /Authentication based on IP address is deprecated/}
+  end
+
   it "should support IPv4 address" do
     add_rule("allow 10.1.1.1")
 
     @auth.should allow(request)
   end

Vulnerable code:         ip? ? pattern.include?(IPAddr.new(ip)) : matchname?(name)
        # LAK:NOTE http://snurl.com/21zf8  [groups_google_com]
    { :ip => '10.1.1.1', :node => 'host.domain.com', :key => 'key', :authenticated => true }.each do |k,v|
      args[k] ||= v
    end
Fixed code:         if ip?
          if pattern.include?(IPAddr.new(ip))
            Puppet.deprecation_warning "Authentication based on IP address is deprecated; please use certname-based rules instead"
            true
          else
            false
          end
        else
          matchname?(name)
        end
    args = {
      :key => 'key',
      :node => 'host.domain.com',
      :ip => '10.1.1.1',
      :authenticated => true
    }.merge(args)
  it "should warn when matching against IP addresses" do
    add_rule("allow 10.1.1.1")

    @auth.should allow(request)

    @logs.should be_any {|log| log.level == :warning and log.message =~ /Authentication based on IP address is deprecated/}
  end

  it "should not warn when matches against IP addresses fail" do
    add_rule("allow 10.1.1.2")

    @auth.should_not allow(request)

    @logs.should_not be_any {|log| log.level == :warning and log.message =~ /Authentication based on IP address is deprecated/}
  end