debian/patches/0029-submodule-reject-submodule.update-command-in-.gitmodu.diff

   1 From b1272b7bc9afec2dcfada666a8f2001aadaef314 Mon Sep 17 00:00:00 2001
   2 From: Jonathan Nieder <jrnieder@gmail.com>
   3 Date: Thu, 5 Dec 2019 01:28:28 -0800
   4 Subject: submodule: reject submodule.update = !command in .gitmodules
   5
   6 Since ac1fbbda2013 (submodule: do not copy unknown update mode from
   7 .gitmodules, 2013-12-02), Git has been careful to avoid copying
   8
   9         [submodule "foo"]
  10                 update = !run an arbitrary scary command
  11
  12 from .gitmodules to a repository's local config, copying in the
  13 setting 'update = none' instead.  The gitmodules(5) manpage documents
  14 the intention:
  15
  16         The !command form is intentionally ignored here for security
  17         reasons
  18
  19 Unfortunately, starting with v2.20.0-rc0 (which integrated ee69b2a9
  20 (submodule--helper: introduce new update-module-mode helper,
  21 2018-08-13, first released in v2.20.0-rc0)), there are scenarios where
  22 we *don't* ignore it: if the config store contains no
  23 submodule.foo.update setting, the submodule-config API falls back to
  24 reading .gitmodules and the repository-supplied !command gets run
  25 after all.
  26
  27 This was part of a general change over time in submodule support to
  28 read more directly from .gitmodules, since unlike .git/config it
  29 allows a project to change values between branches and over time
  30 (while still allowing .git/config to override things).  But it was
  31 never intended to apply to this kind of dangerous configuration.
  32
  33 The behavior change was not advertised in ee69b2a9's commit message
  34 and was missed in review.
  35
  36 Let's take the opportunity to make the protection more robust, even in
  37 Git versions that are technically not affected: instead of quietly
  38 converting 'update = !command' to 'update = none', noisily treat it as
  39 an error.  Allowing the setting but treating it as meaning something
  40 else was just confusing; users are better served by seeing the error
  41 sooner.  Forbidding the construct makes the semantics simpler and
  42 means we can check for it in fsck (in a separate patch).
  43
  44 As a result, the submodule-config API cannot read this value from
  45 .gitmodules under any circumstance, and we can declare with confidence
  46
  47         For security reasons, the '!command' form is not accepted
  48         here.
  49
  50 Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
  51 Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
  52 Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
  53 (cherry picked from commit e904deb89d9a9669a76a426182506a084d3f6308)
  54 Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
  55 ---
  56  Documentation/gitmodules.txt |  5 ++---
  57  submodule-config.c           | 12 ++++++++++--
  58  t/t7406-submodule-update.sh  | 14 ++++++++------
  59  3 files changed, 20 insertions(+), 11 deletions(-)
  60
  61 diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
  62 index f2a65ba0ca..ba9ec5b405 100644
  63 --- a/Documentation/gitmodules.txt
  64 +++ b/Documentation/gitmodules.txt
  65 @@ -44,9 +44,8 @@ submodule.<name>.update::
  66         submodule init` to initialize the configuration variable of
  67         the same name. Allowed values here are 'checkout', 'rebase',
  68         'merge' or 'none'. See description of 'update' command in
  69 -       linkgit:git-submodule[1] for their meaning. Note that the
  70 -       '!command' form is intentionally ignored here for security
  71 -       reasons.
  72 +       linkgit:git-submodule[1] for their meaning. For security
  73 +       reasons, the '!command' form is not accepted here.
  74
  75  submodule.<name>.branch::
  76         A remote branch name for tracking updates in the upstream submodule.
  77 diff --git a/submodule-config.c b/submodule-config.c
  78 index 401a9b2382..b93482d834 100644
  79 --- a/submodule-config.c
  80 +++ b/submodule-config.c
  81 @@ -409,6 +409,13 @@ struct parse_config_parameter {
  82         int overwrite;
  83  };
  84
  85 +/*
  86 + * Parse a config item from .gitmodules.
  87 + *
  88 + * This does not handle submodule-related configuration from the main
  89 + * config store (.git/config, etc).  Callers are responsible for
  90 + * checking for overrides in the main config store when appropriate.
  91 + */
  92  static int parse_config(const char *var, const char *value, void *data)
  93  {
  94         struct parse_config_parameter *me = data;
  95 @@ -486,8 +493,9 @@ static int parse_config(const char *var, const char *value, void *data)
  96                         warn_multiple_config(me->treeish_name, submodule->name,
  97                                              "update");
  98                 else if (parse_submodule_update_strategy(value,
  99 -                        &submodule->update_strategy) < 0)
 100 -                               die(_("invalid value for %s"), var);
 101 +                        &submodule->update_strategy) < 0 ||
 102 +                        submodule->update_strategy.type == SM_UPDATE_COMMAND)
 103 +                       die(_("invalid value for %s"), var);
 104         } else if (!strcmp(item.buf, "shallow")) {
 105                 if (!me->overwrite && submodule->recommend_shallow != -1)
 106                         warn_multiple_config(me->treeish_name, submodule->name,
 107 diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
 108 index df34c994d2..425dc3ab8e 100755
 109 --- a/t/t7406-submodule-update.sh
 110 +++ b/t/t7406-submodule-update.sh
 111 @@ -406,12 +406,12 @@ test_expect_success 'submodule update - command in .git/config' '
 112         )
 113  '
 114
 115 -test_expect_success 'submodule update - command in .gitmodules is ignored' '
 116 +test_expect_success 'submodule update - command in .gitmodules is rejected' '
 117         test_when_finished "git -C super reset --hard HEAD^" &&
 118         git -C super config -f .gitmodules submodule.submodule.update "!false" &&
 119         git -C super commit -a -m "add command to .gitmodules file" &&
 120         git -C super/submodule reset --hard $submodulesha1^ &&
 121 -       git -C super submodule update submodule
 122 +       test_must_fail git -C super submodule update submodule
 123  '
 124
 125  cat << EOF >expect
 126 @@ -480,6 +480,9 @@ test_expect_success 'recursive submodule update - command in .git/config catches
 127  '
 128
 129  test_expect_success 'submodule init does not copy command into .git/config' '
 130 +       test_when_finished "git -C super update-index --force-remove submodule1" &&
 131 +       test_when_finished git config -f super/.gitmodules \
 132 +               --remove-section submodule.submodule1 &&
 133         (cd super &&
 134          git ls-files -s submodule >out &&
 135          H=$(cut -d" " -f2 out) &&
 136 @@ -488,10 +491,9 @@ test_expect_success 'submodule init does not copy command into .git/config' '
 137          git config -f .gitmodules submodule.submodule1.path submodule1 &&
 138          git config -f .gitmodules submodule.submodule1.url ../submodule &&
 139          git config -f .gitmodules submodule.submodule1.update !false &&
 140 -        git submodule init submodule1 &&
 141 -        echo "none" >expect &&
 142 -        git config submodule.submodule1.update >actual &&
 143 -        test_cmp expect actual
 144 +        test_must_fail git submodule init submodule1 &&
 145 +        test_expect_code 1 git config submodule.submodule1.update >actual &&
 146 +        test_must_be_empty actual
 147         )
 148  '
 149
 150 --
 151 2.24.0.393.g34dc348eaf
 152