Try revert operations (on a branch) before approving them

Otherwise, we have a time-of-check/time-of-use vulnerability:
rcs_preprevert previously looked 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().

It is not sufficient to disable rename detection, since git older
than 2.8.0rc0 (in particular the version in Debian stable) silently
accepts and ignores the relevant options.

OVE-20161226-0002

IkiWiki/Plugin/git.pm

@@ -425,6 +425,16 @@ sub parse_diff_tree ($) {
 	}
 	shift @{ $dt_ref } if $dt_ref->[0] =~ /^$/;
 
+	$ci{details} = [parse_changed_files($dt_ref)];
+
+	return \%ci;
+}
+
+sub parse_changed_files {
+	my $dt_ref = shift;
+
+	my @files;
+
 	# Modified files.
 	while (my $line = shift @{ $dt_ref }) {
 		if ($line =~ m{^
@@ -442,7 +452,7 @@ sub parse_diff_tree ($) {
 			my $status = shift(@tmp);
 
 			if (length $file) {
-				push @{ $ci{'details'} }, {
+				push @files, {
 					'file'      => decode_git_file($file),
 					'sha1_from' => $sha1_from[0],
 					'sha1_to'   => $sha1_to,
@@ -456,7 +466,7 @@ sub parse_diff_tree ($) {
 		last;
 	}
 
-	return \%ci;
+	return @files;
 }
 
 sub git_commit_info ($;$) {
@@ -955,10 +965,14 @@ sub rcs_preprevert ($) {
 	my $rev=shift;
 	my ($sha1) = $rev =~ /^($sha1_pattern)$/; # untaint
 
+	my @undo;      # undo stack for cleanup in case of an error
+
+	ensure_committer();
+
 	# Examine changes from root of git repo, not from any subdir,
 	# in order to see all changes.
 	my ($subdir, $rootdir) = git_find_root();
-	in_git_dir($rootdir, sub {
+	return in_git_dir($rootdir, sub {
 		my @commits=git_commit_info($sha1, 1);
 	
 		if (! @commits) {
@@ -971,7 +985,68 @@ sub rcs_preprevert ($) {
 			error gettext("you are not allowed to revert a merge");
 		}
 
+		# Due to the presence of rename-detection, we cannot actually
+		# see what will happen in a revert without trying it.
+		# But we can guess, which is enough to rule out most changes
+		# that we won't allow reverting.
 		git_parse_changes(1, @commits);
+
+		my $failure;
+		my @ret;
+		# If it looks OK, do it for real, on a branch.
+		eval {
+			IkiWiki::disable_commit_hook();
+			push @undo, sub {
+				IkiWiki::enable_commit_hook();
+			};
+			my $branch = "ikiwiki_revert_${sha1}"; # supposed to be unique
+
+			push @undo, sub {
+				run_or_cry('git', 'branch', '-D', $branch) if $failure;
+			};
+			if (run_or_non('git', 'rev-parse', '--quiet', '--verify', $branch)) {
+				run_or_non('git', 'branch', '-D', $branch);
+			}
+			run_or_die('git', 'branch', $branch, $config{gitmaster_branch});
+
+			push @undo, sub {
+				if (!run_or_cry('git', 'checkout', '--quiet', $config{gitmaster_branch})) {
+					run_or_cry('git', 'checkout','-f', '--quiet', $config{gitmaster_branch});
+				}
+			};
+			run_or_die('git', 'checkout', '--quiet', $branch);
+
+			run_or_die('git', 'revert', '--no-commit', $sha1);
+			run_or_non('git', 'commit', '-m', "revert $sha1", '-a');
+
+			# Re-switch to master.
+			run_or_die('git', 'checkout', '--quiet', $config{gitmaster_branch});
+
+			my @raw_lines;
+			@raw_lines = run_or_die('git', 'diff', '--pretty=raw',
+				'--raw', '--abbrev=40', '--always', '--no-renames',
+				"ikiwiki_revert_${sha1}..");
+
+			my $ci = {
+				details => [parse_changed_files(\@raw_lines)],
+			};
+
+			@ret = git_parse_changes(0, $ci);
+		};
+		$failure = $@;
+
+		# Process undo stack (in reverse order).  By policy cleanup
+		# actions should normally print a warning on failure.
+		while (my $handle = pop @undo) {
+			$handle->();
+		}
+
+		if ($failure) {
+			my $message = sprintf(gettext("Failed to revert commit %s"), $sha1);
+			error("$message\n$failure\n");
+		}
+
+		return @ret;
 	});
 }
 
@@ -982,11 +1057,11 @@ sub rcs_revert ($) {
 
 	ensure_committer();
 
-	if (run_or_non('git', 'revert', '--no-commit', $sha1)) {
+	if (run_or_non('git', 'merge', '--ff-only', "ikiwiki_revert_$sha1")) {
 		return undef;
 	}
 	else {
-		run_or_die('git', 'reset', '--hard');
+		run_or_non('git', 'branch', '-D', "ikiwiki_revert_$sha1");
 		return sprintf(gettext("Failed to revert commit %s"), $sha1);
 	}
 }

debian/changelog

@@ -5,6 +5,13 @@ ikiwiki (3.20161220) UNRELEASED; urgency=medium
     analogous to CVE-2014-1572. In ikiwiki this could be used to
     forge commit metadata, but thankfully nothing more serious.
     (OVE-20161226-0001)
+  * Security: try revert operations before approving them. Previously,
+    automatic rename detection could result in a revert writing outside
+    the wiki srcdir or altering a file that the reverting user should not be
+    able to alter, an authorization bypass. The incomplete fix released in
+    3.20161219 was not effective for git versions prior to 2.8.0rc0.
+    (CVE-2016-10026 represents the original vulnerability)
+    (OVE-20161226-0002 represents the incomplete fix released in 3.20161219)
   * Add CVE references for CVE-2016-10026
   * Add missing ikiwiki.setup for the manual test for CVE-2016-10026
   * git: don't issue a warning if the rcsinfo CGI parameter is undefined

doc/security.mdwn

@@ -561,8 +561,12 @@ result in `policy.mdwn` being altered.
 This affects sites with the `git` VCS and the `recentchanges` plugin,
 which are both used in most ikiwiki installations.
 
-This bug was reported on 2016-12-17. The fixed version 3.20161219
-was released on 2016-12-19. ([[!cve CVE-2016-10026]])
+This bug was reported on 2016-12-17. A partially fixed version
+3.20161219 was released on 2016-12-19, but the solution used in that
+version was not effective with git versions older than 2.8.0.
+
+([[!cve CVE-2016-10026]] represents the original vulnerability.
+OVE-20161226-0002 represents the incomplete fix in 3.20161219.)
 
 ## Commit metadata forgery via CGI::FormBuilder context-dependent APIs