Skip to content

Commit ca87bf7

Browse files
author
Toshi MARUYAMA
committed
mercurial: reject malicious command argument (#27516)
We've got a security report from the Phabricator team, which basically says --config and --debugger arguments can be injected anywhere to lead to an arbitrary command execution. https://secure.phabricator.com/rPa7921a4448093d00defa8bd18f35b8c8f8bf3314 This is a fundamental issue of the argument parsing rules in Mercurial, which allows extensions to populate their parsing rules and such extensions can be loaded by "--config extensions.<name>=". There's a chicken and egg problem. We're working on hardening the parsing rules, but which won't come in by default as it would be a behavior change. This patch adds a verification to reject malicious command arguments as a last ditch. The subsequent patches will fix the problem in more appropriate way. Contributed by Yuya Nishihara. git-svn-id: http://svn.redmine.org/redmine/trunk@17060 e93f8b46-1217-0410-a6f0-8f06a7374b81
1 parent d6d2d23 commit ca87bf7

File tree

2 files changed

+34
-0
lines changed

2 files changed

+34
-0
lines changed

Diff for: lib/redmine/scm/adapters/mercurial_adapter.rb

+15
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ class MercurialAdapter < AbstractAdapter
3232

3333
# raised if hg command exited with error, e.g. unknown revision.
3434
class HgCommandAborted < CommandFailed; end
35+
# raised if bad command argument detected before executing hg.
36+
class HgCommandArgumentError < CommandFailed; end
3537

3638
class << self
3739
def client_command
@@ -286,8 +288,21 @@ def format_identifier
286288
end
287289
end
288290

291+
# command options which may be processed earlier, by faulty parser in hg
292+
HG_EARLY_BOOL_ARG = /^--(debugger|profile|traceback)$/
293+
HG_EARLY_LIST_ARG = /^(--(config|cwd|repo(sitory)?)\b|-R)/
294+
private_constant :HG_EARLY_BOOL_ARG, :HG_EARLY_LIST_ARG
295+
289296
# Runs 'hg' command with the given args
290297
def hg(*args, &block)
298+
# as of hg 4.4.1, early parsing of bool options is not terminated at '--'
299+
if args.any? { |s| s =~ HG_EARLY_BOOL_ARG }
300+
raise HgCommandArgumentError, "malicious command argument detected"
301+
end
302+
if args.take_while { |s| s != '--' }.any? { |s| s =~ HG_EARLY_LIST_ARG }
303+
raise HgCommandArgumentError, "malicious command argument detected"
304+
end
305+
291306
repo_path = root_url || url
292307
full_args = ['-R', repo_path, '--encoding', 'utf-8']
293308
full_args << '--config' << "extensions.redminehelper=#{HG_HELPER_EXT}"

Diff for: test/unit/lib/redmine/scm/adapters/mercurial_adapter_test.rb

+19
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ class MercurialAdapterTest < ActiveSupport::TestCase
2121
HELPERS_DIR = Redmine::Scm::Adapters::MercurialAdapter::HELPERS_DIR
2222
TEMPLATE_NAME = Redmine::Scm::Adapters::MercurialAdapter::TEMPLATE_NAME
2323
TEMPLATE_EXTENSION = Redmine::Scm::Adapters::MercurialAdapter::TEMPLATE_EXTENSION
24+
HgCommandArgumentError = Redmine::Scm::Adapters::MercurialAdapter::HgCommandArgumentError
2425

2526
REPOSITORY_PATH = repository_path('mercurial')
2627
CHAR_1_HEX = "\xc3\x9c"
@@ -443,6 +444,24 @@ def test_path_encoding_default_utf8
443444
assert_equal "UTF-8", adpt2.path_encoding
444445
end
445446

447+
def test_bad_early_options
448+
assert_raise HgCommandArgumentError do
449+
@adapter.diff('sources/welcome_controller.rb', '--config=alias.rhdiff=!xterm')
450+
end
451+
assert_raise HgCommandArgumentError do
452+
@adapter.entries('--debugger')
453+
end
454+
assert_raise HgCommandArgumentError do
455+
@adapter.revisions(nil, nil, nil, limit: '--repo=otherrepo')
456+
end
457+
assert_raise HgCommandArgumentError do
458+
@adapter.nodes_in_branch('default', limit: '--repository=otherrepo')
459+
end
460+
assert_raise HgCommandArgumentError do
461+
@adapter.nodes_in_branch('-Rotherrepo')
462+
end
463+
end
464+
446465
private
447466

448467
def test_hgversion_for(hgversion, version)

0 commit comments

Comments
 (0)