Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from Apr 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/puppet/indirector/catalog/compiler.rb
Expand Up @@ -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

Expand Down
2 changes: 0 additions & 2 deletions lib/puppet/network/http/api/indirected_routes.rb
Expand Up @@ -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

Expand Down
11 changes: 10 additions & 1 deletion lib/puppet/network/http/rack/rest.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions spec/unit/indirector/catalog/compiler_spec.rb
Expand Up @@ -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)
Expand Down
34 changes: 21 additions & 13 deletions spec/unit/network/http/api/indirected_routes_spec.rb
Expand Up @@ -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
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions spec/unit/network/http/rack/rest_spec.rb
Expand Up @@ -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")
Expand Down