]> git.vanrenterghem.biz Git - git.ikiwiki.info.git/commitdiff
Try revert operations (on a branch) before approving them
authorSimon McVittie <smcv@debian.org>
Mon, 19 Dec 2016 13:48:56 +0000 (13:48 +0000)
committerSimon McVittie <smcv@debian.org>
Wed, 28 Dec 2016 21:32:12 +0000 (21:32 +0000)
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
debian/changelog
doc/security.mdwn

index 64a47c8e863998e4a82ec245e54d9d1c71f95991..56d649372f02ad7dc1a9e07c2a9a983e44bce41b 100644 (file)
@@ -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);
        }
 }
index ccf830b2768a5c88add904666f79f1546fb59c87..b057ec7f2e54a487cdd85db508f6b4415c2dfb38 100644 (file)
@@ -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
index 9818e0c9429de9f2e50111ef79c467f110dc2e02..c08d658c84b0cc0bee96d0343a04d0ce2eead690 100644 (file)
@@ -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