From 9cada49ed6ad24556dbe9861ad5b0a9f526167f9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 19 Dec 2016 13:48:56 +0000 Subject: [PATCH] Tell `git revert` not to follow renames Otherwise, we have an authorization bypass vulnerability: rcs_preprevert looks at what changed in the commit we are reverting, not at what would result from reverting it now. In particular, if some files were renamed since the commit we are reverting, a revert of changes that were within the designated subdirectory and allowed by check_canchange() might now affect files that are outside the designated subdirectory or disallowed by check_canchange(). Signed-off-by: Simon McVittie --- IkiWiki/Plugin/git.pm | 4 +++- ...ypass_authorization_if_affected_files_were_renamed.mdwn | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/IkiWiki/Plugin/git.pm b/IkiWiki/Plugin/git.pm index 249338d4d..7511f09cb 100644 --- a/IkiWiki/Plugin/git.pm +++ b/IkiWiki/Plugin/git.pm @@ -973,7 +973,9 @@ sub rcs_revert ($) { ensure_committer(); - if (run_or_non('git', 'revert', '--no-commit', $sha1)) { + if (run_or_non('git', 'revert', '--strategy=recursive', + '--strategy-option=no-renames', + '--no-commit', $sha1)) { return undef; } else { diff --git a/doc/bugs/rcs_revert_can_bypass_authorization_if_affected_files_were_renamed.mdwn b/doc/bugs/rcs_revert_can_bypass_authorization_if_affected_files_were_renamed.mdwn index 09a23793d..f8e3b59a3 100644 --- a/doc/bugs/rcs_revert_can_bypass_authorization_if_affected_files_were_renamed.mdwn +++ b/doc/bugs/rcs_revert_can_bypass_authorization_if_affected_files_were_renamed.mdwn @@ -16,3 +16,10 @@ when reverting. > vulnerabilities (such as authorization bypass) by private email to the > maintainers, so that they are not visible to the general public > until we have had a chance to fix the bug. --[[smcv]] + +> Fixed by using +> `git revert --strategy=recursive --strategy-option=no-renames`. +> I tried to do something more clever (doing the revert, and checking +> whether it made changes that aren't allowed) but couldn't get it to +> work in a reasonable time, so I'm going with the simpler fix. +> [[Fix committed|done]], a release will follow later today. --[[smcv]]