From 428f81808910e5c966dfa4683dd904c68ea2ac63 Mon Sep 17 00:00:00 2001 From: Alf Henrik Sauge Date: Fri, 14 Aug 2026 14:26:47 +0200 Subject: [PATCH] Fix src/git/Filter.cpp passing data into the shell without escaping it Without escaping the data, remote shell execution can occur, as evidenced by the test case --- src/git/Filter.cpp | 8 +- test/CMakeLists.txt | 5 + test/Filter.cpp | 120 ++++++++++++++++++++++ test/testRepos/FilterCommandInjection.zip | Bin 0 -> 8249 bytes 4 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 test/Filter.cpp create mode 100644 test/testRepos/FilterCommandInjection.zip diff --git a/src/git/Filter.cpp b/src/git/Filter.cpp index 91d0f1423..c07ca3a69 100644 --- a/src/git/Filter.cpp +++ b/src/git/Filter.cpp @@ -38,7 +38,13 @@ struct FilterInfo { QByteArray attributes; }; -QString quote(const QString &path) { return QString("\"%1\"").arg(path); } +QString quote(const QString &path) { + QString escapedPath = path; + // Ensure that the path is properly escaped to avoid shell injection + // This is inspired by git's sq_quote_buf + escapedPath.replace("'", "'\\''"); + return QString("'%1'").arg(escapedPath); +} struct Stream { int init(git_filter *self, const git_filter_source *src, git_writestream *); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 9a37ae7f3..b6d290724 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -116,6 +116,11 @@ test(NAME Setting) test(NAME commitMessageTemplate) test(NAME commitEditor) +# Test case not compatible with Windows +if(NOT WIN32) + test(NAME Filter) +endif() + option(GITTYUP_CI_TESTS "Run tests that change global settings" OFF) if(GITTYUP_CI_TESTS) test(NAME config) diff --git a/test/Filter.cpp b/test/Filter.cpp new file mode 100644 index 000000000..b8ff7c724 --- /dev/null +++ b/test/Filter.cpp @@ -0,0 +1,120 @@ +// +// Copyright (c) 2026, Gittyup Community +// +// This software is licensed under the MIT License. The LICENSE.md file +// describes the conditions under which this software may be distributed. +// + +#include "Test.h" + +#include "git/Commit.h" +#include "git/Reference.h" +#include "git/Repository.h" + +#include +#include +#include +#include + +namespace { + +// Registers a throwaway global filter driver, scoped to a fake $HOME so +// the developer's real ~/.gitconfig is never touched. git::Filter::init() +// reads global filters once, during Application's constructor, before any +// QTest slot runs - so $HOME must be redirected during static +// initialization, which is why this lives in a namespace-scope static. +struct FakeHome { + QTemporaryDir dir; + QString argsOutPath; + + FakeHome() { + argsOutPath = dir.filePath("filter_args.txt"); + + QFile gitconfig(dir.filePath(".gitconfig")); + bool opened = gitconfig.open(QIODevice::WriteOnly | QIODevice::Truncate | + QIODevice::Text); + Q_ASSERT(opened); + Q_UNUSED(opened); + + QTextStream out(&gitconfig); + // "clean" is unused but must be non-empty for the driver to register. + out << "[filter \"gittyupsecuritytest\"]\n"; + out << " clean = cat\n"; + out << " smudge = printf 'ARG=[%s]' %f > \"" << argsOutPath << "\"\n"; + gitconfig.close(); + + qputenv("HOME", dir.path().toLocal8Bit()); + qputenv("USERPROFILE", dir.path().toLocal8Bit()); + } +}; + +FakeHome &fakeHome() { + static FakeHome home; + return home; +} + +// Force construction (and therefore the $HOME override) before main(). +FakeHome &gFakeHomeInit = fakeHome(); + +} // namespace + +class TestFilter : public QObject { + Q_OBJECT + +private slots: + void rejectsShellInjectionInFilename(); +}; + +// Regression test: a filename embedding a single quote, combined with a +// .gitattributes rule assigning it to any globally-registered filter (e.g. +// Git LFS), could break out of the quoted %f argument and run arbitrary +// shell commands during smudge. +void TestFilter::rejectsShellInjectionInFilename() { + FakeHome &home = fakeHome(); + QFile::remove(home.argsOutPath); + + // FilterCommandInjection.zip's single commit contains one file, matching + // this name, whose .gitattributes assigns it to the "gittyupsecuritytest" + // filter registered above. If %f isn't escaped correctly, the embedded + // "'" ends the quoted shell argument early and the ';'s that follow start + // new, attacker-controlled statements. + QString maliciousName = "evil';>injected_marker;echo'safe"; + + QString path = Test::extractRepository("FilterCommandInjection.zip", true); + QVERIFY(!path.isEmpty()); + git::Repository repo = git::Repository::open(path); + QVERIFY(repo.isValid()); + QDir workdir = repo.workdir(); + + // The injected ">injected_marker" redirection, if it runs, creates this + // file in the checkout's working directory. + QString markerPath = workdir.filePath("injected_marker"); + QFile::remove(markerPath); + + // Delete the working copy so checkout has to rewrite it from the repo, + // which is what actually triggers the smudge filter. + QVERIFY(QFile::remove(workdir.filePath(maliciousName))); + + // libgit2's own checkout implementation invokes the registered smudge + // filter driver as it writes each file to the working directory. + git::Reference head = repo.head(); + QVERIFY(head.isValid()); + QVERIFY( + repo.checkout(head.target(), nullptr, QStringList(), GIT_CHECKOUT_FORCE)); + + QVERIFY2(!QFile::exists(markerPath), + "filter command injection via crafted filename was not blocked"); + + // Confirms the filter actually ran (not just failed to start). + QFile args(home.argsOutPath); + QVERIFY2(args.exists(), "smudge filter did not run"); + QVERIFY(args.open(QIODevice::ReadOnly | QIODevice::Text)); + QString content = QString::fromUtf8(args.readAll()); + args.close(); + + QCOMPARE(content, QString("ARG=[%1]").arg(maliciousName)); +} + +TEST_MAIN(TestFilter) + +#include "Filter.moc" diff --git a/test/testRepos/FilterCommandInjection.zip b/test/testRepos/FilterCommandInjection.zip new file mode 100644 index 0000000000000000000000000000000000000000..96db204683ddb79bcaf0c22df6a1fa20797358ae GIT binary patch literal 8249 zcmdT}c~nzp77trgj3^q8%b-OBB#^)h30YjKRG@?q76lnR1NxG@K*ACYiA6_I(PFg* z97a@H0im>1T&fJ{U|p(KmVjcNXi*rg%8VN#=qcLj9Og>`A&(|`FLnNzmveJo{<**V zyZ3(gyZ0uR@8IZ6&^^Z_F3I|rp|7a~cS5){Ly0Js$TKtHEHN)jhKQ6>c~)RT{5XO` z^~+pY0}xftc!HhdBL@OO{}qc;xT!$4aRE@M6K~5315%|*SgBMQJ*2gJ zllg0J4YuYc`Fvep|Et~e^KPUK>mQeR@9_2xE8%_VedzLH6^YX(3S$&LbN`~mkMU|} zr~j>U+XaWq*{S_xb|CR%ZEc&oD*fBGGe@-DMcO^DyS}gN(Rx1BcJJEo^ZoDZ;)(gY zJrWZS=JJShoE!q)E1XiSd7EBvzO&(8veVt(^7ZXsl^@@!={WypQT3{>uQvLX+AW%T zr)a-w1_2#z;27hB<)I9DYPJnYZqy52d@!7^SFJ!KzgIsMxBfH)7Gt9rQb>39XDYSN zcLN4DfCSlyk_l&nI$=Bty3#PJET|hpT=JkvPS@(o?#rW?S@}z=riso(rjIEdn_0E& zLH`7H-_cn=Y=Syiil!~JV72!Y^?Ucz1W#{QsJ1?JZ@8A^@UP1HzP#4cowxlzyHmQp zwqU(`%cRWSb!A~E_Idf`&O1QYBtE^E?h~CW2@QA=;a)Kvh6g`*@dDm6H)hJ6;KNm8 zykK^vHa5Plrn~MxcDm#+&mE&o1?nRh5f&2q|4a9C>4LjUK$!VJ{4hejBt{_M#jgwt z<;4q@eQ1rd(}fEYHhWBR{O){0Pm@m%@nYT|`akJ{sdr0i0&jM2sfp&Z;~vQx^QJZb z^>Lo}*=^-LGuvm(PTviG(^tHZ?9a@2Gb$?QlJ?z@028+Z@x(AwB+rsaQ;p`CDVV{# zLYyx_B**pv{})Ho=~CCrW1&%KIF9 z&t2x1#dJ1)ko;$E1o7)vXZyno|C!|U^enOK5&57xFB|UJ3~TQ&Pil&S#_iPRMV(46 zEd5AUKfUgW`nqyy=~DGzvg6=@;HN_EE`O&K*Js4fVsi@iM@()Yy{St3WrE-Mo5d-? zO34uTidfYXBJC#SJP&83~G#*XD znZLCZ`LQT@V6b-O-f^H1_z%5s^4CyU&WC5&4u`-@7Z2tT)e zjc|qEC2lw)LDK7XDzSFs#Lt&6l3%?~$x0zycJ!mZyKhB^4ZhWENV@3;{HhbYz6Agf zoxutUh8U0#=7@zb7h*}+!7MI|E#V?u5zOE+nJgx3^sSluFvpIsX^B}1iJBv>w6r|5 zC(OL;Nd6y?7u!I7SCmtH6d+~mc9Tn@yBm_ir^!jMQ1mJs2xS#)|3EDUA~5eO1X z5O^f9tdM7{ilDW&#hp6Y+SVE=2;d2JvX7!lTs++NqR7 zRGM$JAb_T!9-{JS0r%_)AG%J@vFtMn&Wa_?lE`iF8gQ2J!3(h(7Ny$&;V;duo(e#8 z;(@^e)rpw7_CT*W&qLpE8X7`Ft7bBkjGp_jn&Wq9Ua^{Ey=o==yPI#U@@wJ-r7>K`q;_SgdyG3CNVH8q<4OXAZAspPLp$2_d^V-fG@sT;57sPnD?NB?#nr~b<^w&Orxp)1FYqYZtV-%gq5Hc!zb`C*J8(RjGfn;Rl-d<` zbHaXod8whfY2d<>hWzKfW6+riykLB=`r|aYJRPgIw6mY(V7|MYN|-i;G>PymME7cO zGvs9042>H9V)tI3w>_p#6al&x zoH(@m0SIRD)!<3PnfUGIXBa`}$p#FBp*@f_81vaM86_ST6EEmN@lHToj*LBX)V zg3+44OcyJ*V^L99dvRk8FIlW1j?^#oip4Y)-`a>ce7o3=>pjHOaN1rw;_#}W#lQ!J zUM-kXeCT8yYV@K3A0B!~V2W1&7Q5CGW_V*@4aa=`d!dpgc8qnLk-6S>q`4@hV>prY zGd?~r^n^cLJ?scxTPBXhM+<=AYhx1P@tFjQvhhLlXO6>Xl770z2Zo-lP5Bq&2g5sQ z<1@)f9=xFW?*{2;Eu)7sO+NwS14GZhru_TygW;Wq@tI_qzj{1Q{<tzd*vrf2gZ|S2(W;_~3?yTYPZn(bnK_!&S5$0}lHZGGDFH-O6hW zl%eSs9~sk7p}Uo3F4lc#I_cVug)R$!!-IvVQ5o?cdge8a5_uvHVy2r0>nCxfH$soK zrjS(_kf