@matheusbsbs / matheus-bagliano / commits / a8cc594333

hooks: fix an obscure TOCTOU "did we just run a hook?" race

Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when 680ee550d72 was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See 0855331941b (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see 67541597670 (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see 66618a50f9c (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Mar 7, 2022 at 13:33 UTC a8cc594333848713b8e772cccf8159196ea85ede

8 files changed +47 -18

builtin/commit.c

+11 -7

@@ -726,11 +726,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

726 int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);

727 int old_display_comment_prefix;

728 int merge_contains_scissors = 0;

729 + int invoked_hook;

730

731 /* This checks and barfs if author is badly specified */

732 determine_author_info(author_ident);

733

733 - if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))

734 + if (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,

735 + "pre-commit", NULL))

736 return 0;

737

738 if (squash_message) {

@@ -1053,10 +1055,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

1055 return 0;

1056 }

1057

1056 - if (!no_verify && hook_exists("pre-commit")) {

1058 + if (!no_verify && invoked_hook) {

1059 /*

1058 - * Re-read the index as pre-commit hook could have updated it,

1059 - * and write it out as a tree. We must do this before we invoke

1060 + * Re-read the index as the pre-commit-commit hook was invoked

1061 + * and could have updated it. We must do this before we invoke

1062 * the editor and after we invoke run_status above.

1063 */

1064 discard_cache();

@@ -1068,7 +1070,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

1070 return 0;

1071 }

1072

1071 - if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",

1073 + if (run_commit_hook(use_editor, index_file, NULL, "prepare-commit-msg",

1074 git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))

1075 return 0;

1076

@@ -1085,7 +1087,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

1087 }

1088

1089 if (!no_verify &&

1088 - run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {

1090 + run_commit_hook(use_editor, index_file, NULL, "commit-msg",

1091 + git_path_commit_editmsg(), NULL)) {

1092 return 0;

1093 }

1094

@@ -1841,7 +1844,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

1844

1845 repo_rerere(the_repository, 0);

1846 run_auto_maintenance(quiet);

1844 - run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);

1847 + run_commit_hook(use_editor, get_index_file(), NULL, "post-commit",

1848 + NULL);

1849 if (amend && !no_post_rewrite) {

1850 commit_post_rewrite(the_repository, current_head, &oid);

1851 }

builtin/merge.c

+7 -4

@@ -846,7 +846,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)

846 const char *index_file = get_index_file();

847

848 if (!no_verify) {

849 - if (run_commit_hook(0 < option_edit, index_file,

849 + int invoked_hook;

850 +

851 + if (run_commit_hook(0 < option_edit, index_file, &invoked_hook,

852 "pre-merge-commit", NULL))

853 abort_commit(remoteheads, NULL);

854 /*

@@ -854,7 +856,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)

856 * and write it out as a tree. We must do this before we invoke

857 * the editor and after we invoke run_status above.

858 */

857 - if (hook_exists("pre-merge-commit"))

859 + if (invoked_hook)

860 discard_cache();

861 }

862 read_cache_from(index_file);

@@ -878,7 +880,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)

880 append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);

881 write_merge_heads(remoteheads);

882 write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);

881 - if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",

883 + if (run_commit_hook(0 < option_edit, get_index_file(), NULL,

884 + "prepare-commit-msg",

885 git_path_merge_msg(the_repository), "merge", NULL))

886 abort_commit(remoteheads, NULL);

887 if (0 < option_edit) {

@@ -887,7 +890,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)

890 }

891

892 if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),

890 - "commit-msg",

893 + NULL, "commit-msg",

894 git_path_merge_msg(the_repository), NULL))

895 abort_commit(remoteheads, NULL);

896

builtin/receive-pack.c

+5 -3

@@ -1408,10 +1408,12 @@ static const char *push_to_deploy(unsigned char *sha1,

1408 static const char *push_to_checkout_hook = "push-to-checkout";

1409

1410 static const char *push_to_checkout(unsigned char *hash,

1411 + int *invoked_hook,

1412 struct strvec *env,

1413 const char *work_tree)

1414 {

1415 struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;

1416 + opt.invoked_hook = invoked_hook;

1417

1418 strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));

1419 strvec_pushv(&opt.env, env->v);

@@ -1426,6 +1428,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w

1428 {

1429 const char *retval, *git_dir;

1430 struct strvec env = STRVEC_INIT;

1431 + int invoked_hook;

1432

1433 if (!worktree || !worktree->path)

1434 BUG("worktree->path must be non-NULL");

@@ -1436,10 +1439,9 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w

1439

1440 strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));

1441

1439 - if (!hook_exists(push_to_checkout_hook))

1442 + retval = push_to_checkout(sha1, &invoked_hook, &env, worktree->path);

1443 + if (!invoked_hook)

1444 retval = push_to_deploy(sha1, &env, worktree->path);

1441 - else

1442 - retval = push_to_checkout(sha1, &env, worktree->path);

1445

1446 strvec_clear(&env);

1447 return retval;

commit.c

+1 -1

@@ -1713,7 +1713,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)

1713 }

1714

1715 int run_commit_hook(int editor_is_used, const char *index_file,

1716 - const char *name, ...)

1716 + int *invoked_hook, const char *name, ...)

1717 {

1718 struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;

1719 va_list args;

commit.h

+2 -1

@@ -369,7 +369,8 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)

369 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);

370

371 LAST_ARG_MUST_BE_NULL

372 -int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);

372 +int run_commit_hook(int editor_is_used, const char *index_file,

373 + int *invoked_hook, const char *name, ...);

374

375 /* Sign a commit or tag buffer, storing the result in a header. */

376 int sign_with_header(struct strbuf *buf, const char *keyid);

hook.c

@@ -96,9 +96,13 @@ static int notify_hook_finished(int result,

96 void *pp_task_cb)

97 {

98 struct hook_cb_data *hook_cb = pp_cb;

99 + struct run_hooks_opt *opt = hook_cb->options;

100

101 hook_cb->rc |= result;

102

103 + if (opt->invoked_hook)

104 + *opt->invoked_hook = 1;

105 +

106 return 0;

107 }

108

@@ -123,6 +127,9 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)

127 if (!options)

128 BUG("a struct run_hooks_opt must be provided to run_hooks");

129

130 + if (options->invoked_hook)

131 + *options->invoked_hook = 0;

132 +

133 if (!hook_path && !options->error_if_missing)

134 goto cleanup;

135

hook.h

+12

@@ -18,6 +18,18 @@ struct run_hooks_opt

18 * translates to "struct child_process"'s "dir" member.

19 */

20 const char *dir;

21 +

22 + /**

23 + * A pointer which if provided will be set to 1 or 0 depending

24 + * on if a hook was started, regardless of whether or not that

25 + * was successful. I.e. if the underlying start_command() was

26 + * successful this will be set to 1.

27 + *

28 + * Used for avoiding TOCTOU races in code that would otherwise

29 + * call hook_exist() after a "maybe hook run" to see if a hook

30 + * was invoked.

31 + */

32 + int *invoked_hook;

33 };

35 #define RUN_HOOKS_OPT_INIT { \

sequencer.c

+2 -2

@@ -1220,7 +1220,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,

1220 } else {

1221 arg1 = "message";

1222 }

1223 - if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,

1223 + if (run_commit_hook(0, r->index_file, NULL, "prepare-commit-msg", name,

1224 arg1, arg2, NULL))

1225 ret = error(_("'prepare-commit-msg' hook failed"));

1226

@@ -1552,7 +1552,7 @@ static int try_to_commit(struct repository *r,

1552 goto out;

1553 }

1554

1555 - run_commit_hook(0, r->index_file, "post-commit", NULL);

1555 + run_commit_hook(0, r->index_file, NULL, "post-commit", NULL);

1556 if (flags & AMEND_MSG)

1557 commit_post_rewrite(r, current_head, oid);

1558