trace2: redact passwords from https:// URLs by default
It is an unsafe practice to call something like git clone https://user:password@example.com/ This not only risks leaking the password "over the shoulder" or into the readline history of the current Unix shell, it also gets logged via Trace2 if enabled. Let's at least avoid logging such secrets via Trace2, much like we avoid logging secrets in `http.c`. Much like the code in `http.c` is guarded via `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via `GIT_TRACE2_REDACT` (also defaulting to `true`). The new tests added in this commit uncover leaks in `builtin/clone.c` and `remote.c`. Therefore we need to turn off `TEST_PASSES_SANITIZE_LEAK`. The reasons: - We observed that `the_repository->remote_status` is not released properly. - We are using `url...insteadOf` and that runs into a code path where an allocated URL is replaced with another URL, and the original URL is never released. - `remote_states` contains plenty of `struct remote`s whose refspecs seem to be usually allocated by never released. More investigation is needed here to identify the exact cause and proper fixes for these leaks/bugs. Co-authored-by: Jeff Hostetler <jeffhostetler@github.com> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johannes Schindelin committed
Nov 22, 2023 at 19:18 UTC
b7d49ac1ecd05669edada26b990eee677a7d3c25
2 files changed
+136
-4
t/t0210-trace2-normal.sh
+19
-1
@@ -2,7 +2,7 @@
2
3
test_description='test trace2 facility (normal target)'
4
5
-TEST_PASSES_SANITIZE_LEAK=true
5
+TEST_PASSES_SANITIZE_LEAK=false
6
. ./test-lib.sh
7
8
# Turn off any inherited trace2 settings for this test.
@@ -283,4 +283,22 @@ test_expect_success 'using global config with include' '
283
test_cmp expect actual
284
'
285
286
+test_expect_success 'unsafe URLs are redacted by default' '
287
+ test_when_finished \
288
+ "rm -r trace.normal unredacted.normal clone clone2" &&
289
+
290
+ test_config_global \
291
+ "url.$(pwd).insteadOf" https://user:pwd@example.com/ &&
292
+ test_config_global trace2.configParams "core.*,remote.*.url" &&
293
+
294
+ GIT_TRACE2="$(pwd)/trace.normal" \
295
+ git clone https://user:pwd@example.com/ clone &&
296
+ ! grep user:pwd trace.normal &&
297
+
298
+ GIT_TRACE2_REDACT=0 GIT_TRACE2="$(pwd)/unredacted.normal" \
299
+ git clone https://user:pwd@example.com/ clone2 &&
300
+ grep "start .* clone https://user:pwd@example.com" unredacted.normal &&
301
+ grep "remote.origin.url=https://user:pwd@example.com" unredacted.normal
302
+'
303
+
304
test_done
trace2.c
+117
-3
@@ -20,6 +20,7 @@
20
#include "trace2/tr2_tmr.h"
21
22
static int trace2_enabled;
23
+static int trace2_redact = 1;
24
25
static int tr2_next_child_id; /* modify under lock */
26
static int tr2_next_exec_id; /* modify under lock */
@@ -227,6 +228,8 @@ void trace2_initialize_fl(const char *file, int line)
228
if (!tr2_tgt_want_builtins())
229
return;
230
trace2_enabled = 1;
231
+ if (!git_env_bool("GIT_TRACE2_REDACT", 1))
232
+ trace2_redact = 0;
233
234
tr2_sid_get();
235
@@ -247,12 +250,93 @@ int trace2_is_enabled(void)
250
return trace2_enabled;
251
}
252
253
+/*
254
+ * Redacts an argument, i.e. ensures that no password in
255
+ * https://user:password@host/-style URLs is logged.
256
+ *
257
+ * Returns the original if nothing needed to be redacted.
258
+ * Returns a pointer that needs to be `free()`d otherwise.
259
+ */
260
+static const char *redact_arg(const char *arg)
261
+{
262
+ const char *p, *colon;
263
+ size_t at;
264
+
265
+ if (!trace2_redact ||
266
+ (!skip_prefix(arg, "https://", &p) &&
267
+ !skip_prefix(arg, "http://", &p)))
268
+ return arg;
269
+
270
+ at = strcspn(p, "@/");
271
+ if (p[at] != '@')
272
+ return arg;
273
+
274
+ colon = memchr(p, ':', at);
275
+ if (!colon)
276
+ return arg;
277
+
278
+ return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
279
+}
280
+
281
+/*
282
+ * Redacts arguments in an argument list.
283
+ *
284
+ * Returns the original if nothing needed to be redacted.
285
+ * Otherwise, returns a new array that needs to be released
286
+ * via `free_redacted_argv()`.
287
+ */
288
+static const char **redact_argv(const char **argv)
289
+{
290
+ int i, j;
291
+ const char *redacted = NULL;
292
+ const char **ret;
293
+
294
+ if (!trace2_redact)
295
+ return argv;
296
+
297
+ for (i = 0; argv[i]; i++)
298
+ if ((redacted = redact_arg(argv[i])) != argv[i])
299
+ break;
300
+
301
+ if (!argv[i])
302
+ return argv;
303
+
304
+ for (j = 0; argv[j]; j++)
305
+ ; /* keep counting */
306
+
307
+ ALLOC_ARRAY(ret, j + 1);
308
+ ret[j] = NULL;
309
+
310
+ for (j = 0; j < i; j++)
311
+ ret[j] = argv[j];
312
+ ret[i] = redacted;
313
+ for (++i; argv[i]; i++) {
314
+ redacted = redact_arg(argv[i]);
315
+ ret[i] = redacted ? redacted : argv[i];
316
+ }
317
+
318
+ return ret;
319
+}
320
+
321
+static void free_redacted_argv(const char **redacted, const char **argv)
322
+{
323
+ int i;
324
+
325
+ if (redacted != argv) {
326
+ for (i = 0; argv[i]; i++)
327
+ if (redacted[i] != argv[i])
328
+ free((void *)redacted[i]);
329
+ free((void *)redacted);
330
+ }
331
+}
332
+
333
void trace2_cmd_start_fl(const char *file, int line, const char **argv)
334
{
335
struct tr2_tgt *tgt_j;
336
int j;
337
uint64_t us_now;
338
uint64_t us_elapsed_absolute;
339
+ const char **redacted;
340
341
if (!trace2_enabled)
342
return;
@@ -260,10 +344,14 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
344
us_now = getnanotime() / 1000;
345
us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
346
347
+ redacted = redact_argv(argv);
348
+
349
for_each_wanted_builtin (j, tgt_j)
350
if (tgt_j->pfn_start_fl)
351
tgt_j->pfn_start_fl(file, line, us_elapsed_absolute,
266
- argv);
352
+ redacted);
353
+
354
+ free_redacted_argv(redacted, argv);
355
}
356
357
void trace2_cmd_exit_fl(const char *file, int line, int code)
@@ -409,6 +497,7 @@ void trace2_child_start_fl(const char *file, int line,
497
int j;
498
uint64_t us_now;
499
uint64_t us_elapsed_absolute;
500
+ const char **orig_argv = cmd->args.v;
501
502
if (!trace2_enabled)
503
return;
@@ -419,10 +508,24 @@ void trace2_child_start_fl(const char *file, int line,
508
cmd->trace2_child_id = tr2tls_locked_increment(&tr2_next_child_id);
509
cmd->trace2_child_us_start = us_now;
510
511
+ /*
512
+ * The `pfn_child_start_fl` API takes a `struct child_process`
513
+ * rather than a simple `argv` for the child because some
514
+ * targets make use of the additional context bits/values. So
515
+ * temporarily replace the original argv (inside the `strvec`)
516
+ * with a possibly redacted version.
517
+ */
518
+ cmd->args.v = redact_argv(orig_argv);
519
+
520
for_each_wanted_builtin (j, tgt_j)
521
if (tgt_j->pfn_child_start_fl)
522
tgt_j->pfn_child_start_fl(file, line,
523
us_elapsed_absolute, cmd);
524
+
525
+ if (cmd->args.v != orig_argv) {
526
+ free_redacted_argv(cmd->args.v, orig_argv);
527
+ cmd->args.v = orig_argv;
528
+ }
529
}
530
531
void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,
@@ -493,6 +596,7 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
596
int exec_id;
597
uint64_t us_now;
598
uint64_t us_elapsed_absolute;
599
+ const char **redacted;
600
601
if (!trace2_enabled)
602
return -1;
@@ -502,10 +606,14 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
606
607
exec_id = tr2tls_locked_increment(&tr2_next_exec_id);
608
609
+ redacted = redact_argv(argv);
610
+
611
for_each_wanted_builtin (j, tgt_j)
612
if (tgt_j->pfn_exec_fl)
613
tgt_j->pfn_exec_fl(file, line, us_elapsed_absolute,
508
- exec_id, exe, argv);
614
+ exec_id, exe, redacted);
615
+
616
+ free_redacted_argv(redacted, argv);
617
618
return exec_id;
619
}
@@ -637,13 +745,19 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
745
{
746
struct tr2_tgt *tgt_j;
747
int j;
748
+ const char *redacted;
749
750
if (!trace2_enabled)
751
return;
752
753
+ redacted = redact_arg(value);
754
+
755
for_each_wanted_builtin (j, tgt_j)
756
if (tgt_j->pfn_param_fl)
646
- tgt_j->pfn_param_fl(file, line, param, value, kvi);
757
+ tgt_j->pfn_param_fl(file, line, param, redacted, kvi);
758
+
759
+ if (redacted != value)
760
+ free((void *)redacted);
761
}
762
763
void trace2_def_repo_fl(const char *file, int line, struct repository *repo)