Skip to content

Commit bbe9972

Browse files
marksmithmarksmith
authored
Merge pull request #184 from AikidoSec/conditionally-harden-methods
Conditionally harden methods
2 parents 897179b + 263886e commit bbe9972

File tree

5 files changed

+131
-33
lines changed

5 files changed

+131
-33
lines changed

‎docs/config.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ set it via `Aikido::Zen.config.token = <token>`.
3434

3535
**NOTE**: Never commit your token to the source code repository in plain text.
3636

37+
## Hardened mode
38+
39+
Zen hardens methods, restricting dangerous undocumented behavior to improve
40+
security and performance.
41+
42+
To disable method hardening, set `AIKIDO_HARDEN=false` in your environment,
43+
or set `Aikido::Zen.config.harden = false`.
44+
3745
## Logger
3846

3947
Zen logs to standard output by default. You can change this by changing the

‎lib/aikido/zen/config.rb‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,11 @@ class Config
153153
# allow known hosts that should be able to resolve to the IMDS service.
154154
attr_accessor :imds_allowed_hosts
155155

156+
# @return [Boolean] whether Aikido Zen should harden methods where possible.
157+
# Defaults to true. Can be set through AIKIDO_HARDEN environment variable.
158+
attr_accessor :harden
159+
alias_method :harden?, :harden
160+
156161
def initialize
157162
self.disabled = read_boolean_from_env(ENV.fetch("AIKIDO_DISABLED", false))
158163
self.blocking_mode = read_boolean_from_env(ENV.fetch("AIKIDO_BLOCK", false))
@@ -185,6 +190,7 @@ def initialize
185190
self.api_schema_collection_max_properties = 20
186191
self.stored_ssrf = read_boolean_from_env(ENV.fetch("AIKIDO_FEATURE_STORED_SSRF", true))
187192
self.imds_allowed_hosts = ["metadata.google.internal", "metadata.goog"]
193+
self.harden = read_boolean_from_env(ENV.fetch("AIKIDO_HARDEN", true))
188194
end
189195

190196
# Set the base URL for API requests.

‎lib/aikido/zen/context.rb‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,45 @@ def extract_payloads_from(data, source_type, prefix = nil)
9999
extract_payloads_from(value, source_type, [prefix, key].compact.join("."))
100100
end
101101
elsif data.respond_to?(:to_ary)
102-
data.to_ary.flat_map.with_index do |value, index|
102+
array = data.to_ary
103+
return array if array.empty?
104+
105+
payloads = array.flat_map.with_index do |value, index|
103106
extract_payloads_from(value, source_type, [prefix, index].compact.join("."))
104107
end
108+
109+
unless Aikido::Zen.config.harden?
110+
# Special case for File.join given a possibly nested array of strings,
111+
# as might occur when a query parameter is an array.
112+
begin
113+
string = File.join__internal_for_aikido_zen(*array)
114+
if unsafe_path?(string)
115+
payloads << Payload.new(string, source_type, [prefix, "__File.join__"].compact.join("."))
116+
end
117+
rescue
118+
# Could not create special payload for File.join.
119+
end
120+
end
121+
122+
payloads
105123
else
106124
[Payload.new(data, source_type, prefix.to_s)]
107125
end
108126
end
127+
128+
def unsafe_path?(filepath)
129+
normalized_filepath = Pathname.new(filepath).cleanpath.to_s.downcase
130+
131+
Scanners::PathTraversal::DANGEROUS_PATH_PARTS.each do |dangerous_path_part|
132+
return true if normalized_filepath.include?(dangerous_path_part)
133+
end
134+
135+
Scanners::PathTraversal::DANGEROUS_PATH_STARTS.each do |dangerous_path_start|
136+
return true if normalized_filepath.start_with?(dangerous_path_start)
137+
end
138+
139+
false
140+
end
109141
end
110142
end
111143

‎lib/aikido/zen/sinks/file.rb‎

Lines changed: 34 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -82,38 +82,40 @@ def self.load_sinks!
8282
end
8383

8484
def join(*args, **kwargs, &blk)
85-
# IMPORTANT: THE BEHAVIOR OF THIS METHOD IS CHANGED!
86-
#
87-
# File.join has undocumented behavior:
88-
#
89-
# File.join recursively joins nested string arrays.
90-
#
91-
# This prevents path traversal detection when an array originates
92-
# from user input that was assumed to be a string.
93-
#
94-
# This undocumented behavior has been restricted to support path
95-
# traversal detection.
96-
#
97-
# File.join no longer joins nested string arrays, but still accepts
98-
# a single string array argument.
99-
100-
# File.join is often incorrectly called with a single array argument.
101-
#
102-
# i.e.
103-
#
104-
# File.join(["prefix", "filename"])
105-
#
106-
# This is considered acceptable.
107-
#
108-
# Calling File.join with a single string argument returns the string
109-
# argument itself, having no practical effect. Therefore, it can be
110-
# presumed that if File.join is called with a single array argument
111-
# then this was its intended usage, and the array did not originate
112-
# from user input that was assumed to be a string.
113-
strings = args
114-
strings = args.first if args.size == 1 && args.first.is_a?(Array)
115-
strings.each do |string|
116-
raise TypeError.new("Zen prevented implicit conversion of Array to String") if string.is_a?(Array)
85+
if Aikido::Zen.config.harden?
86+
# IMPORTANT: THE BEHAVIOR OF THIS METHOD IS CHANGED!
87+
#
88+
# File.join has undocumented behavior:
89+
#
90+
# File.join recursively joins nested string arrays.
91+
#
92+
# This prevents path traversal detection when an array originates
93+
# from user input that was assumed to be a string.
94+
#
95+
# This undocumented behavior has been restricted to support path
96+
# traversal detection.
97+
#
98+
# File.join no longer joins nested string arrays, but still accepts
99+
# a single string array argument.
100+
101+
# File.join is often incorrectly called with a single array argument.
102+
#
103+
# i.e.
104+
#
105+
# File.join(["prefix", "filename"])
106+
#
107+
# This is considered acceptable.
108+
#
109+
# Calling File.join with a single string argument returns the string
110+
# argument itself, having no practical effect. Therefore, it can be
111+
# presumed that if File.join is called with a single array argument
112+
# then this was its intended usage, and the array did not originate
113+
# from user input that was assumed to be a string.
114+
strings = args
115+
strings = args.first if args.size == 1 && args.first.is_a?(Array)
116+
strings.each do |string|
117+
raise TypeError.new("Zen prevented implicit conversion of Array to String in hardened method. Visit https://github.com/AikidoSec/firewall-ruby for more information.") if string.is_a?(Array)
118+
end
117119
end
118120

119121
result = join__internal_for_aikido_zen(*args, **kwargs, &blk)

‎test/aikido/zen/context_test.rb‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,32 @@ module GenericTests
7777
assert_includes context.payloads, stub_payload(:body, "7", "y.w.j")
7878
end
7979

80+
test "relative path arrays in complex parameter structures are joined for File.join" do
81+
context = build_context_for("/path?path1[]=..&path1[]=..&path1[]=..&path1[]=etc&path1[]=passwd", {
82+
method: "POST",
83+
params: {path2: ["..", "..", "..", "etc", "passwd"]}
84+
})
85+
86+
assert_includes context.payloads, stub_payload(:query, "../../../etc/passwd", "path1.__File.join__")
87+
88+
assert_includes context.payloads, stub_payload(:body, "../../../etc/passwd", "path2.__File.join__")
89+
end
90+
91+
test "absolute path arrays in complex parameter structures are joined for File.join" do
92+
context = build_context_for("/path?path1[]=&path1[]=etc&path1[]=passwd&path2[]=/&path2[]=etc&path2[]=passwd&&path3[]=/etc&path3[]=passwd", {
93+
method: "POST",
94+
params: {path4: ["/", "etc", "passwd"], path5: ["", "etc", "passwd"], path6: ["/etc", "passwd"]}
95+
})
96+
97+
assert_includes context.payloads, stub_payload(:query, "/etc/passwd", "path1.__File.join__")
98+
assert_includes context.payloads, stub_payload(:query, "/etc/passwd", "path2.__File.join__")
99+
assert_includes context.payloads, stub_payload(:query, "/etc/passwd", "path3.__File.join__")
100+
101+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path4.__File.join__")
102+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path5.__File.join__")
103+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path6.__File.join__")
104+
end
105+
80106
test "#protection_disabled? allows requests from allowed IPs" do
81107
settings = Aikido::Zen.runtime_settings
82108
settings.update_from_json({
@@ -103,6 +129,7 @@ class RackRequestTest < ActiveSupport::TestCase
103129

104130
setup do
105131
Aikido::Zen.config.request_builder = Aikido::Zen::Context::RACK_REQUEST_BUILDER
132+
Aikido::Zen.config.harden = false
106133
end
107134

108135
def build_context_for(path, env = {})
@@ -196,6 +223,7 @@ class RailsRequestTest < ActiveSupport::TestCase
196223

197224
setup do
198225
Aikido::Zen.config.request_builder = Aikido::Zen::Context::RAILS_REQUEST_BUILDER
226+
Aikido::Zen.config.harden = false
199227
end
200228

201229
def env_for(path, env = {})
@@ -238,6 +266,28 @@ def stub_payload(source, value, path)
238266
assert_includes context.payloads, stub_payload(:body, 3, "array.2")
239267
end
240268

269+
test "relative path arrays in body payloads read from JSON-encoded bodies are joined for File.join" do
270+
context = build_context_for("/example", {
271+
:method => "POST",
272+
:input => %({"path":["..", "..", "..", "etc", "passwd"]}),
273+
"CONTENT_TYPE" => "application/json"
274+
})
275+
276+
assert_includes context.payloads, stub_payload(:body, "../../../etc/passwd", "path.__File.join__")
277+
end
278+
279+
test "absolute path arrays in body payloads read from JSON-encoded bodies are joined for File.join" do
280+
context = build_context_for("/example", {
281+
:method => "POST",
282+
:input => %({"path1": ["/", "etc", "passwd"], "path2": ["", "etc", "passwd"], "path3": ["/etc", "passwd"]}),
283+
"CONTENT_TYPE" => "application/json"
284+
})
285+
286+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path1.__File.join__")
287+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path2.__File.join__")
288+
assert_includes context.payloads, stub_payload(:body, "/etc/passwd", "path3.__File.join__")
289+
end
290+
241291
test "route payloads are read from the data extracted by the router" do
242292
router = MockedRailsRouter.build do
243293
match "/example/:resource(/:id)(.:format)",

0 commit comments

Comments
 (0)