debian/patches/0006-fast-import-disallow-feature-export-marks-by-default.diff

   1 From 3b8a460adc5ef811ae7e748fd7b98d8779445e59 Mon Sep 17 00:00:00 2001
   2 From: Jeff King <peff@peff.net>
   3 Date: Thu, 29 Aug 2019 14:37:26 -0400
   4 Subject: fast-import: disallow "feature export-marks" by default
   5
   6 The fast-import stream command "feature export-marks=<path>" lets the
   7 stream write marks to an arbitrary path. This may be surprising if you
   8 are running fast-import against an untrusted input (which otherwise
   9 cannot do anything except update Git objects and refs).
  10
  11 Let's disallow the use of this feature by default, and provide a
  12 command-line option to re-enable it (you can always just use the
  13 command-line --export-marks as well, but the in-stream version provides
  14 an easy way for exporters to control the process).
  15
  16 This is a backwards-incompatible change, since the default is flipping
  17 to the new, safer behavior. However, since the main users of the
  18 in-stream versions would be import/export-based remote helpers, and
  19 since we trust remote helpers already (which are already running
  20 arbitrary code), we'll pass the new option by default when reading a
  21 remote helper's stream. This should minimize the impact.
  22
  23 Note that the implementation isn't totally simple, as we have to work
  24 around the fact that fast-import doesn't parse its command-line options
  25 until after it has read any "feature" lines from the stream. This is how
  26 it lets command-line options override in-stream. But in our case, it's
  27 important to parse the new --allow-unsafe-features first.
  28
  29 There are three options for resolving this:
  30
  31   1. Do a separate "early" pass over the options. This is easy for us to
  32      do because there are no command-line options that allow the
  33      "unstuck" form (so there's no chance of us mistaking an argument
  34      for an option), though it does introduce a risk of incorrect
  35      parsing later (e.g,. if we convert to parse-options).
  36
  37   2. Move the option parsing phase back to the start of the program, but
  38      teach the stream-reading code never to override an existing value.
  39      This is tricky, because stream "feature" lines override each other
  40      (meaning we'd have to start tracking the source for every option).
  41
  42   3. Accept that we might parse a "feature export-marks" line that is
  43      forbidden, as long we don't _act_ on it until after we've parsed
  44      the command line options.
  45
  46      This would, in fact, work with the current code, but only because
  47      the previous patch fixed the export-marks parser to avoid touching
  48      the filesystem.
  49
  50      So while it works, it does carry risk of somebody getting it wrong
  51      in the future in a rather subtle and unsafe way.
  52
  53 I've gone with option (1) here as simple, safe, and unlikely to cause
  54 regressions.
  55
  56 This fixes CVE-2019-1348.
  57
  58 Signed-off-by: Jeff King <peff@peff.net>
  59 (cherry picked from commit 68061e3470210703cb15594194718d35094afdc0)
  60 Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
  61 ---
  62  Documentation/git-fast-import.txt | 14 ++++++++++++++
  63  fast-import.c                     | 25 +++++++++++++++++++++++++
  64  t/t9300-fast-import.sh            | 23 +++++++++++++++--------
  65  transport-helper.c                |  1 +
  66  4 files changed, 55 insertions(+), 8 deletions(-)
  67
  68 diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
  69 index a3f1e0c5e4..a94b576d64 100644
  70 --- a/Documentation/git-fast-import.txt
  71 +++ b/Documentation/git-fast-import.txt
  72 @@ -51,6 +51,20 @@ OPTIONS
  73         memory used by fast-import during this run.  Showing this output
  74         is currently the default, but can be disabled with --quiet.
  75
  76 +--allow-unsafe-features::
  77 +       Many command-line options can be provided as part of the
  78 +       fast-import stream itself by using the `feature` or `option`
  79 +       commands. However, some of these options are unsafe (e.g.,
  80 +       allowing fast-import to access the filesystem outside of the
  81 +       repository). These options are disabled by default, but can be
  82 +       allowed by providing this option on the command line.  This
  83 +       currently impacts only the `feature export-marks` command.
  84 ++
  85 +       Only enable this option if you trust the program generating the
  86 +       fast-import stream! This option is enabled automatically for
  87 +       remote-helpers that use the `import` capability, as they are
  88 +       already trusted to run their own code.
  89 +
  90  Options for Frontends
  91  ~~~~~~~~~~~~~~~~~~~~~
  92
  93 diff --git a/fast-import.c b/fast-import.c
  94 index 9cfc1386a3..1f4e4a0438 100644
  95 --- a/fast-import.c
  96 +++ b/fast-import.c
  97 @@ -217,6 +217,7 @@ static uintmax_t next_mark;
  98  static struct strbuf new_data = STRBUF_INIT;
  99  static int seen_data_command;
 100  static int require_explicit_termination;
 101 +static int allow_unsafe_features;
 102
 103  /* Signal handling */
 104  static volatile sig_atomic_t checkpoint_requested;
 105 @@ -3238,6 +3239,8 @@ static int parse_one_option(const char *option)
 106                 show_stats = 0;
 107         } else if (!strcmp(option, "stats")) {
 108                 show_stats = 1;
 109 +       } else if (!strcmp(option, "allow-unsafe-features")) {
 110 +               ; /* already handled during early option parsing */
 111         } else {
 112                 return 0;
 113         }
 114 @@ -3245,6 +3248,13 @@ static int parse_one_option(const char *option)
 115         return 1;
 116  }
 117
 118 +static void check_unsafe_feature(const char *feature, int from_stream)
 119 +{
 120 +       if (from_stream && !allow_unsafe_features)
 121 +               die(_("feature '%s' forbidden in input without --allow-unsafe-features"),
 122 +                   feature);
 123 +}
 124 +
 125  static int parse_one_feature(const char *feature, int from_stream)
 126  {
 127         const char *arg;
 128 @@ -3256,6 +3266,7 @@ static int parse_one_feature(const char *feature, int from_stream)
 129         } else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
 130                 option_import_marks(arg, from_stream, 1);
 131         } else if (skip_prefix(feature, "export-marks=", &arg)) {
 132 +               check_unsafe_feature(feature, from_stream);
 133                 option_export_marks(arg);
 134         } else if (!strcmp(feature, "alias")) {
 135                 ; /* Don't die - this feature is supported */
 136 @@ -3384,6 +3395,20 @@ int cmd_main(int argc, const char **argv)
 137         avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));
 138         marks = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
 139
 140 +       /*
 141 +        * We don't parse most options until after we've seen the set of
 142 +        * "feature" lines at the start of the stream (which allows the command
 143 +        * line to override stream data). But we must do an early parse of any
 144 +        * command-line options that impact how we interpret the feature lines.
 145 +        */
 146 +       for (i = 1; i < argc; i++) {
 147 +               const char *arg = argv[i];
 148 +               if (*arg != '-' || !strcmp(arg, "--"))
 149 +                       break;
 150 +               if (!strcmp(arg, "--allow-unsafe-features"))
 151 +                       allow_unsafe_features = 1;
 152 +       }
 153 +
 154         global_argc = argc;
 155         global_argv = argv;
 156
 157 diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
 158 index 14df3b0475..b65555750f 100755
 159 --- a/t/t9300-fast-import.sh
 160 +++ b/t/t9300-fast-import.sh
 161 @@ -2154,6 +2154,11 @@ test_expect_success 'R: only one import-marks feature allowed per stream' '
 162         test_must_fail git fast-import <input
 163  '
 164
 165 +test_expect_success 'R: export-marks feature forbidden by default' '
 166 +       echo "feature export-marks=git.marks" >input &&
 167 +       test_must_fail git fast-import <input
 168 +'
 169 +
 170  test_expect_success 'R: export-marks feature results in a marks file being created' '
 171         cat >input <<-EOF &&
 172         feature export-marks=git.marks
 173 @@ -2164,7 +2169,7 @@ test_expect_success 'R: export-marks feature results in a marks file being creat
 174
 175         EOF
 176
 177 -       git fast-import <input &&
 178 +       git fast-import --allow-unsafe-features <input &&
 179         grep :1 git.marks
 180  '
 181
 182 @@ -2177,7 +2182,8 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
 183         hi
 184
 185         EOF
 186 -       git fast-import --export-marks=cmdline-sub/other.marks <input &&
 187 +       git fast-import --allow-unsafe-features \
 188 +                       --export-marks=cmdline-sub/other.marks <input &&
 189         grep :1 cmdline-sub/other.marks &&
 190         test_path_is_missing feature-sub
 191  '
 192 @@ -2185,7 +2191,7 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
 193  test_expect_success 'R: catch typo in marks file name' '
 194         test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
 195         echo "feature import-marks=nonexistent.marks" |
 196 -       test_must_fail git fast-import
 197 +       test_must_fail git fast-import --allow-unsafe-features
 198  '
 199
 200  test_expect_success 'R: import and output marks can be the same file' '
 201 @@ -2287,7 +2293,7 @@ test_expect_success 'R: import to output marks works without any content' '
 202         feature export-marks=marks.new
 203         EOF
 204
 205 -       git fast-import <input &&
 206 +       git fast-import --allow-unsafe-features <input &&
 207         test_cmp marks.out marks.new
 208  '
 209
 210 @@ -2297,7 +2303,7 @@ test_expect_success 'R: import marks prefers commandline marks file over the str
 211         feature export-marks=marks.new
 212         EOF
 213
 214 -       git fast-import --import-marks=marks.out <input &&
 215 +       git fast-import --import-marks=marks.out --allow-unsafe-features <input &&
 216         test_cmp marks.out marks.new
 217  '
 218
 219 @@ -2310,7 +2316,8 @@ test_expect_success 'R: multiple --import-marks= should be honoured' '
 220
 221         head -n2 marks.out > one.marks &&
 222         tail -n +3 marks.out > two.marks &&
 223 -       git fast-import --import-marks=one.marks --import-marks=two.marks <input &&
 224 +       git fast-import --import-marks=one.marks --import-marks=two.marks \
 225 +               --allow-unsafe-features <input &&
 226         test_cmp marks.out combined.marks
 227  '
 228
 229 @@ -2323,7 +2330,7 @@ test_expect_success 'R: feature relative-marks should be honoured' '
 230
 231         mkdir -p .git/info/fast-import/ &&
 232         cp marks.new .git/info/fast-import/relative.in &&
 233 -       git fast-import <input &&
 234 +       git fast-import --allow-unsafe-features <input &&
 235         test_cmp marks.new .git/info/fast-import/relative.out
 236  '
 237
 238 @@ -2335,7 +2342,7 @@ test_expect_success 'R: feature no-relative-marks should be honoured' '
 239         feature export-marks=non-relative.out
 240         EOF
 241
 242 -       git fast-import <input &&
 243 +       git fast-import --allow-unsafe-features <input &&
 244         test_cmp marks.new non-relative.out
 245  '
 246
 247 diff --git a/transport-helper.c b/transport-helper.c
 248 index a9d690297e..413d9d873e 100644
 249 --- a/transport-helper.c
 250 +++ b/transport-helper.c
 251 @@ -435,6 +435,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 252         child_process_init(fastimport);
 253         fastimport->in = xdup(helper->out);
 254         argv_array_push(&fastimport->args, "fast-import");
 255 +       argv_array_push(&fastimport->args, "--allow-unsafe-features");
 256         argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
 257
 258         if (data->bidi_import) {
 259 --
 260 2.24.0.393.g34dc348eaf
 261