Skip to content

Commit 62f6f55

Browse files
Watson1978Watson1978
authored
Merge commit from fork
The check required a separator after "..", so a tag of exactly ".." or "foo/.." passed and injected a parent directory component into the output path. Validate ".." as a path component, reject Windows drive specifications, and always run the parent directory count check after the substitution. Signed-off-by: Shizuo Fujita <fujita@clear-code.com>
1 parent 5385b6f commit 62f6f55

2 files changed

Lines changed: 80 additions & 11 deletions

File tree

‎lib/fluent/plugin/output.rb‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,21 @@ class Output < Base
4545
CHUNK_KEY_PLACEHOLDER_PATTERN = /\$\{([-_.@$a-zA-Z0-9]+)\}/
4646
CHUNK_TAG_PLACEHOLDER_PATTERN = /\$\{(tag(?:\[-?\d+\])?)\}/
4747
CHUNK_ID_PLACEHOLDER_PATTERN = /\$\{chunk_id\}/
48-
INVALID_PATH_COMPONENT_PATTERN = %r{\.\.[/\\]|^[/\\]}
49-
PARENT_DIRECTORY_PATTERN = %r{\.\.[/\\]}
48+
49+
# Matches absolute paths, including the POSIX root, Windows drive roots, and UNC paths (e.g., \\host\share).
50+
# It also accounts for forward slashes used as directory separators in Windows.
51+
ABSOLUTE_PATH_PATTERN = %r{\A[/\\]}
52+
# Matches Windows drive specifications.
53+
# A trailing separator after the colon is not required, because a drive-relative path like C:Windows can still escape a relative path template.
54+
WINDOWS_DRIVE_PATTERN = %r{\A[a-zA-Z]:}
55+
# Matches .. only when it acts as an independent path component (e.g., ../etc or foo/..),
56+
# ignoring dots embedded within strings like app..web or a..b/c.
57+
# It uses zero-width negative lookarounds to avoid consuming characters.
58+
# This ensures that adjacent components like /a/../../b are accurately detected and counted without missing any.
59+
PARENT_DIRECTORY_PATTERN = %r{(?<![^/\\])\.\.(?![^/\\])}
60+
61+
INVALID_PATH_COMPONENT_PATTERN =
62+
Regexp.union(ABSOLUTE_PATH_PATTERN, WINDOWS_DRIVE_PATTERN, PARENT_DIRECTORY_PATTERN)
5063

5164
CHUNKING_FIELD_WARN_NUM = 4
5265

@@ -879,12 +892,13 @@ def extract_placeholders(str, chunk)
879892

880893
replace
881894
end
882-
# Check if the number of parent directory components (../) has increased due to variable substitution
883-
if rvalue.match?(PARENT_DIRECTORY_PATTERN)
884-
if rvalue.scan(PARENT_DIRECTORY_PATTERN).size > str.scan(PARENT_DIRECTORY_PATTERN).size
885-
raise Fluent::UnrecoverableError, "Invalid path component detected, replaced to: #{rvalue}"
886-
end
887-
end
895+
end
896+
897+
# Verifies that the number of parent directory traversals (..) has not increased after variable substitution.
898+
# While each replaced value is validated individually, this step prevents harmless values from concatenating into a dangerous path (e.g., ${tag}${key}/ becoming ../ if both tag and key are .).
899+
if rvalue.match?(PARENT_DIRECTORY_PATTERN) &&
900+
rvalue.scan(PARENT_DIRECTORY_PATTERN).size > str.scan(PARENT_DIRECTORY_PATTERN).size
901+
raise Fluent::UnrecoverableError, "Invalid path component detected, replaced to: #{rvalue}"
888902
end
889903

890904
if rvalue =~ CHUNK_KEY_PLACEHOLDER_PATTERN

‎test/plugin/test_output.rb‎

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,11 @@ def waiting(seconds)
454454
'safe_slash' => 'app/web',
455455
'symbol_mixed' => 'my_tag-123',
456456
'no_match' => 'app..web/log',
457+
'dots_in_middle' => 'a..b',
458+
'dots_as_prefix' => '..b/log',
459+
'dots_as_suffix' => 'a../log',
460+
'single_dot' => '.',
461+
'not_drive_spec' => 'http://example.com',
457462
)
458463
test 'allows valid tags including safe slashes' do |tag|
459464
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'tag')]))
@@ -468,7 +473,15 @@ def waiting(seconds)
468473
'unix_relative' => '../etc/cron.d',
469474
'windows_relative' => '..\\Windows\\System32',
470475
'unix_absolute' => '/etc/passwd',
471-
'windows_absolute' => '\\Windows'
476+
'windows_absolute' => '\\Windows',
477+
'windows_unc' => '\\\\host\\share',
478+
'bare_parent' => '..',
479+
'trailing_parent' => 'app/..',
480+
'middle_parent' => 'app/../etc',
481+
'trailing_parent_windows' => 'app\\..',
482+
'windows_drive' => 'C:\\Windows\\Temp',
483+
'windows_drive_slash' => 'C:/Windows/Temp',
484+
'windows_drive_relative' => 'C:Temp',
472485
)
473486
test 'rejects tags containing parent directory relative or absolute root paths' do |tag|
474487
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'tag')]))
@@ -486,7 +499,10 @@ def waiting(seconds)
486499
'unix_relative' => '../etc/cron.d',
487500
'windows_relative' => '..\\Windows\\System32',
488501
'unix_absolute' => '/etc/passwd',
489-
'windows_absolute' => '\\Windows'
502+
'windows_absolute' => '\\Windows',
503+
'bare_parent' => '..',
504+
'trailing_parent' => 'app/..',
505+
'windows_drive' => 'C:\\Windows\\Temp',
490506
)
491507
test 'rejects tags containing traversal paths even when only parts are used' do |tag|
492508
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'tag')]))
@@ -504,7 +520,12 @@ def waiting(seconds)
504520
'unix_relative' => '../etc/cron.d',
505521
'windows_relative' => '..\\Windows\\System32',
506522
'unix_absolute' => '/etc/passwd',
507-
'windows_absolute' => '\\Windows'
523+
'windows_absolute' => '\\Windows',
524+
'bare_parent' => '..',
525+
'trailing_parent' => 'app/..',
526+
'middle_parent' => 'app/../etc',
527+
'windows_drive' => 'C:\\Windows\\Temp',
528+
'windows_drive_relative' => 'C:Temp',
508529
)
509530
test 'rejects variables containing parent directory relative or absolute root paths' do |var_value|
510531
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'tag,file')]))
@@ -534,6 +555,40 @@ def waiting(seconds)
534555
assert_match(/Invalid path component detected, replaced to/, err.message)
535556
end
536557

558+
data(
559+
'tag and tag' => ['/data/${tag}${tag}/log', 'tag'],
560+
'tag and variable' => ['/data/${tag}${dot}/log', 'tag,dot'],
561+
)
562+
test 'rejects traversal built up by concatenating placeholders' do |(tmpl, chunk_keys)|
563+
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', chunk_keys)]))
564+
t = event_time('2016-04-11 20:30:00 +0900')
565+
m = create_metadata(timekey: t, tag: '.', variables: { dot: '.' })
566+
567+
err = assert_raise(Fluent::UnrecoverableError) do
568+
@i.extract_placeholders(tmpl, m)
569+
end
570+
assert_match(/Invalid path component detected, replaced to/, err.message)
571+
end
572+
573+
test 'rejects traversal placed at the end of the path' do
574+
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'dot')]))
575+
t = event_time('2016-04-11 20:30:00 +0900')
576+
m = create_metadata(timekey: t, variables: { dot: '.' })
577+
578+
err = assert_raise(Fluent::UnrecoverableError) do
579+
@i.extract_placeholders('/data/${dot}${dot}', m)
580+
end
581+
assert_match(/Invalid path component detected, replaced to/, err.message)
582+
end
583+
584+
test 'passes safely when a template itself contains ../ and only tag is replaced' do
585+
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'tag')]))
586+
t = event_time('2016-04-11 20:30:00 +0900')
587+
m = create_metadata(timekey: t, tag: 'app.web', variables: {})
588+
589+
assert_equal '../data/app.web/log', @i.extract_placeholders('../data/${tag}/log', m)
590+
end
591+
537592
test 'passes safely when placeholder expansion does NOT introduce additional ../' do
538593
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'val1,val2,val3')]))
539594
t = event_time('2016-04-11 20:30:00 +0900')

0 commit comments

Comments
 (0)