From: Jacob Bachmeyer <jcb@gnu.org>
Date: Sat, 12 Nov 2022 04:29:33 +0000 (-0600)
Subject: Rename automake_tests to check_automake_vulnerabilities and simplify
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=152b17828f1cc63193d7d319ccaa92c5ebaf3996;p=gatekeeper.git

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 680a8e3..fef8189 100755
--- 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);
-}
-
-
-#
-# - [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;
 }
 
-
 
 #
 # - [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 1da3f26..2432841 100644
--- 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} {