Merge pull request #4921 from underscorgan/merge-cve-to-stable

(maint) merge-back for work for CVE-2016-2785

Author: Kylo Ginsberg

lib/puppet/indirector/catalog/compiler.rb

@@ -254,7 +254,8 @@ def compile(node, options)
       raise Puppet::Error, "Unable to find a common checksum type between agent '#{options[:checksum_type]}' and master '#{known_checksum_types}'." unless checksum_type
     end
 
-    str = "Compiled %s for #{node.name}" % [checksum_type ? 'static catalog' : 'catalog']
+    str = "Compiled %s for " % [checksum_type ? 'static catalog' : 'catalog']
+    str += node.name
     str += " in environment #{node.environment}" if node.environment
     config = nil
 

lib/puppet/network/http/api/indirected_routes.rb

@@ -104,8 +104,6 @@ def uri2indirection(http_method, uri, params)
       raise ArgumentError, "No request key specified in #{uri}"
     end
 
-    key = URI.unescape(key)
-
     [indirection, method, key, params]
   end
 

lib/puppet/network/http/rack/rest.rb

@@ -2,6 +2,7 @@
 require 'cgi'
 require 'puppet/network/http/handler'
 require 'puppet/util/ssl'
+require 'uri'
 
 class Puppet::Network::HTTP::RackREST
   include Puppet::Network::HTTP::Handler
@@ -79,7 +80,15 @@ def params(request)
 
   # what path was requested? (this is, without any query parameters)
   def path(request)
-    request.path
+    # The value that Passenger provides for 'path' is escaped
+    # (URL percent-encoded), see
+    # https://github.com/phusion/passenger/blob/release-5.0.26/src/apache2_module/Hooks.cpp#L885
+    # for the implementation as hooked up to an Apache web server.  Code
+    # in the indirector / HTTP layer which consumes this path, however, assumes
+    # that it has already been unescaped, so it is unescaped here.
+    if request.path
+      URI.unescape(request.path)
+    end
   end
 
   # return the request body

spec/unit/indirector/catalog/compiler_spec.rb

@@ -94,6 +94,13 @@
       @compiler.find(@request)
     end
 
+    it "should pass node containing percent character to the compiler" do
+      node_with_percent_character = Puppet::Node.new "%6de"
+      Puppet::Node.indirection.stubs(:find).returns(node_with_percent_character)
+      Puppet::Parser::Compiler.expects(:compile).with(node_with_percent_character, anything)
+      @compiler.find(@request)
+    end
+
     it "should extract and save any facts from the request" do
       Puppet::Node.indirection.expects(:find).with(@name, anything).returns @node
       @compiler.expects(:extract_facts_from_request).with(@request)

spec/unit/network/http/api/indirected_routes_spec.rb

@@ -127,22 +127,17 @@
       expect(lambda { handler.uri2indirection("UPDATE", "#{master_url_prefix}/node/bar", params) }).to raise_error(ArgumentError)
     end
 
-    it "should URI unescape the indirection key" do
+    it "should not URI unescape the indirection key" do
       escaped = URI.escape("foo bar")
-      indirection, method, key, final_params = handler.uri2indirection("GET", "#{master_url_prefix}/node/#{escaped}", params)
-      expect(key).to eq("foo bar")
+      indirection, _, key, _ = handler.uri2indirection("GET", "#{master_url_prefix}/node/#{escaped}", params)
+      expect(key).to eq(escaped)
     end
 
-    it "should pass through a proper environment param in a call to check_authorization" do
-      handler.expects(:check_authorization).with(anything,
-                                                 anything,
-                                                 all_of(
-                                                   has_entry(:environment,
-                                                             is_a(Puppet::Node::Environment)),
-                                                   has_entry(:environment,
-                                                             responds_with(:name,
-                                                                           :env))))
-      handler.uri2indirection("GET", "#{master_url_prefix}/node/bar", params)
+    it "should not unescape the URI passed through in a call to check_authorization" do
+      key_escaped = URI.escape("foo bar")
+      uri_escaped = "#{master_url_prefix}/node/#{key_escaped}"
+      handler.expects(:check_authorization).with(anything, uri_escaped, anything)
+      indirection, _, _, _ = handler.uri2indirection("GET", uri_escaped, params)
     end
 
     it "should not pass through an environment to check_authorization and fail if the environment is unknown" do
@@ -153,6 +148,19 @@
                                               "#{master_url_prefix}/node/bar",
                                               {:environment => 'bogus'}) }).to raise_error(ArgumentError)
     end
+
+    it "should not URI unescape the indirection key as passed through to a call to check_authorization" do
+      handler.expects(:check_authorization).with(anything,
+                                                 anything,
+                                                 all_of(
+                                                     has_entry(:environment,
+                                                               is_a(Puppet::Node::Environment)),
+                                                     has_entry(:environment,
+                                                               responds_with(:name,
+                                                                             :env))))
+      handler.uri2indirection("GET", "#{master_url_prefix}/node/bar", params)
+    end
+
   end
 
   describe "when converting a request into a URI" do

spec/unit/network/http/rack/rest_spec.rb

@@ -65,6 +65,13 @@ def mk_req(uri, opts = {})
         expect(@handler.path(req)).to eq("/foo/bar")
       end
 
+      it "should return the unescaped path for an escaped request path" do
+        unescaped_path = '/foo/bar baz'
+        escaped_path = URI.escape(unescaped_path)
+        req = mk_req(escaped_path)
+        expect(@handler.path(req)).to eq(unescaped_path)
+      end
+
       it "should return the request body as the body" do
         req = mk_req('/foo/bar', :input => 'mybody')
         expect(@handler.body(req)).to eq("mybody")