Rename automake_tests to check_automake_vulnerabilities and simplify

This also moves the checks for known GNU Automake CVE issues to the
top-level, and eliminates the now-otherwise-useless check_vulnerabilities
and check_files procedures.  The major impetus for this simplification
of the call graph was the observation that check_vulnerabilities, while
named generically, was associated with a log message citing specifically
CVE-2009-4029 and CVE-2012-3386, combined with noticing that all other
functionality had been factored out of check_files.

diff --git a/gatekeeper.pl b/gatekeeper.pl
index 680a8e3d6217e13f6f29b3155144a4bf40501dcb..fef81893686875e7a500a23112dec6605b432291 100755 (executable)
--- a/gatekeeper.pl
+++ b/gatekeeper.pl
@@ -2300,7 +2300,7 @@ sub check_replay {
   }
 }
 
-=item automake_tests ( $upload_file )
+=item check_automake_vulnerabilities ( $upload_file )
 
 Examine UPLOAD_FILE for known vulnerabilities that certain (now ancient)
 versions of GNU Automake can introduce.  Returns nothing, but throws an
@@ -2308,7 +2308,7 @@ exception if UPLOAD_FILE is found to be exploitable.
 
 =cut
 
-sub automake_tests {
+sub check_automake_vulnerabilities {
   my $upload_file = shift;
 
   die "file not found: $upload_file"
@@ -2359,59 +2359,12 @@ sub automake_tests {
     throw known_vulnerability => file => $upload_file, issues => \%issues
       if %issues;
   }
-}
-
-=item check_vulnerabilities ( $upload_file )
-
-Examine UPLOAD_FILE for known vulnerabilities.  This is only used for
-issues related to packaging; notably an old issue in GNU Automake.
-
-Returns nothing on success and throws an exception if a known vulnerability
-is found in UPLOAD_FILE.
-
-=cut
-
-sub check_vulnerabilities {
-  my $upload_file = shift;
-
-  automake_tests($upload_file);
-}
-
-\f
-#
-# - [AZ] Authorization
-#
-
-=item check_files ( $directory, $oplist_header )
-
-Check the file pair in an upload by verifying the detached signature and
-ensuring that the upload, if a tarball, does not show signs of known
-vulnerabilities.  This function needs only the operation list header hash
-and the directory in which the files are staged.  The allowed keyrings are
-found using information in the operation list header.
-
-The upload file name is taken from the operation list header, where it was
-obtained from the "filename" directive element.  The name of the detached
-signature is constructed by appending a ".sig" suffix, per PGP conventions.
-
-An exception is thrown if any of the checks fail.
-
-=cut
-
-# TODO: this is currently invoked during EX phase; should be moved to VL
-sub check_files {
-  my $directory = shift;
-  my $header = shift;
-
-  my $upload_file = File::Spec->catfile($directory, $header->{filename});
-
-  check_vulnerabilities($upload_file);
 
   ftp_syslog('debug', "DEBUG: "
-            ."tested negative for CVE-2009-4029 and CVE-2012-3386") if DEBUG;
+            ."tested negative for CVE-2009-4029 and CVE-2012-3386")
+    if DEBUG;
 }
 
-
 \f
 #
 # - [EX] Execution
@@ -2575,7 +2528,6 @@ sub execute_commands {
 
   foreach my $step (@{$oplist}[1..$#$oplist]) {        # skip the header
     if ($step->[0] eq 'install') {
-      check_files($Scratch_dir, $header);
       install_files($header, $step);
     } elsif ($step->[0] eq 'symlink') {
       my $target = $step->[1];
@@ -2817,6 +2769,13 @@ foreach my $packet (@packets) {  # each list element is an array reference
          if $fsig_info->{exitcode} != 0 || defined $fsig_info->{TILT};
     }
 
+    $Phase = 'VL';
+
+    # If the upload carries a file, check it for known Automake CVE issues.
+    check_automake_vulnerabilities
+      (File::Spec->catfile($Scratch_dir, $op_header->{filename}))
+       if find_directive_elements($directive, 'filename');
+
     $Phase = 'EX';
     # do the work
     execute_commands($oplist);

diff --git a/testsuite/lib/gatekeeper.exp b/testsuite/lib/gatekeeper.exp
index 1da3f26102f1e9fd3b199fc15ea98d7b3855ce76..2432841119f9c3c0cb9ff21644d3eaf64c4e1c5c 100644 (file)
--- a/testsuite/lib/gatekeeper.exp
+++ b/testsuite/lib/gatekeeper.exp
@@ -810,35 +810,32 @@ proc analyze_log { base_dir name assess } {
                     set A(gpgv,upload-verify-failed,$expect_out(1,string)) 1
                     exp_continue
                 }
-       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[EX\]\
+       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[(?:VL|EX)\]\
                 DEBUG: tested negative for CVE-[^\r\n]+} {
                     # from check_files, when checks for known issues pass
                     exp_continue
                     # tests are not sensitive to this message because it
                     # is likely to be revised as part of other refactoring
                 }
-       # TODO: move check_files to VL phase and factor out signature check
-       #       to AA phase
 
-       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[EX\]\
+       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[(?:VL|EX)\]\
                 DEBUG: testing .+ for presence of Makefile.in} {
                     # from check_vulnerabilities via check_files
                     set A(exploit-check,check-Makefile.in) 1
                     exp_continue
                 }
-       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[EX\]\
+       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[(?:VL|EX)\]\
                 DEBUG: found Makefile.in, testing for [^\r\n]+} {
                     # from check_vulnerabilities via check_files
                     set A(exploit-check,found-Makefile.in) 1
                     exp_continue
                 }
-       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[EX\]\
+       -re {^gatekeeper\[[0-9]+\]: \(Test\) \[(?:VL|EX)\]\
                 upload found vulnerable to (CVE-[0-9-]+)[^\r\n]*} {
                     # from check_vulnerabilities via check_files
                     set A(exploit-check-fail,$expect_out(1,string)) 1
                     exp_continue
                 }
-       # TODO:  move CVE checks to VL phase
 
        -re {^gatekeeper\[[0-9]+\]: \(Test\) \[EX\]\
                 [^ ]+ exists and 'replace' was not selected} {