zonesigner (dnssec-tools) is broken

Ubuntu package dnssec-tools provides a broken zonesigner script, which fails when any the -kskdirectory, -zskdirectory, or -keydirectory options is used. The script (/usr/sbin/zonesigner) fails to dereference file globs when invoking external 'mv' or 'cp' commands. The upstream version (from http://www.dnssec-tools.org/download/ , installed in /usr/local/bin) avoids this problem. The Ubuntu script claims the same version number as the upstream one, but differs.

The difference between the original and Ubuntu versions of the script appears to be due to an incompletely considered attempt to protect against shell command injection.

The Ubuntu script needs to be changed either to use the original code or to dereference file globs explicitly before invoking the relevant external command.

Details follow below.

joe(user)2097: /usr/sbin/zonesigner -Version
zonesigner version: 2.1.0
DNSSEC-Tools Version: 2.1
joe(user)2098: /usr/local/bin/zonesigner -Version
zonesigner version: 2.1.0
DNSSEC-Tools Version: 2.1
joe(user)2099: cat /etc/os-release
NAME="Ubuntu"
VERSION="16.04.2 LTS (Xenial Xerus)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 16.04.2 LTS"
VERSION_ID="16.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
VERSION_CODENAME=xenial
UBUNTU_CODENAME=xenial
joe(user)2100: diff -b -u /usr/sbin/zonesigner /usr/local/bin/zonesigner
--- /usr/sbin/zonesigner 2016-01-22 23:10:55.000000000 +0000
+++ /usr/local/bin/zonesigner 2017-02-13 13:56:56.000000000 +0000
@@ -1087,11 +1087,11 @@
  #
  # Ensure that the zone file has not been signed yet.
  #
- #if(presigned())
- #{
- # print STDERR "zone file $zonefile already signed\n";
- # exit(17);
- #}
+ if(presigned())
+ {
+ print STDERR "zone file $zonefile already signed\n";
+ exit(17);
+ }
 }

 #----------------------------------------------------------------------
@@ -1985,7 +1985,6 @@

  my $keys; # Keys we're modifying.
  my @keylist; # Keys inna list.
- my @args;

  #
  # Change the type of the given signing set to the specified type.
@@ -2021,11 +2020,6 @@
     vmed_print("setting revoke bit($cwd): $cmd\n");

     System($cmd);
- if ($? != 0)
- {
- print STDERR "'$cmd' returned $?\n" if($verbose);
- exit($?);
- }

     #
     # Record the time when key is revoked.
@@ -2057,13 +2051,7 @@
  if(! -e $archdir)
  {
   vmed_print("creating key archive directory $archdir\n\n");
- @args = ($MKDIR, "-p", "-m 0700", $archdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
+ System("$MKDIR -p -m 0700 $archdir");
  }
  else
  {
@@ -2091,13 +2079,7 @@
    $newname = "$archdir/$kronos.$archfn";
    vhigh_print("moving $key to $newname\n\n");

- @args = ($MV, $archfn, $newname);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
+ System("$MV $archfn $newname");

    if($archfn =~ /\.key$/)
    {
@@ -2182,7 +2164,6 @@
 sub keydirs
 {
  my $cwd = getcwd(); # Current directory.
- my @args;

  vmed_print("checking key directories\n");

@@ -2205,24 +2186,12 @@

   foreach my $ksk (@kskcurlist)
   {
- @args = ($MV, "$ksk.*", $kskdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?)
- }
+ System("$MV $ksk.* $kskdir");
   }

   foreach my $ksk (@kskpublist)
   {
- @args = ($MV, "$ksk.*", $kskdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?)
- }
+ System("$MV $ksk.* $kskdir");
   }

  }
@@ -2246,35 +2215,17 @@

   foreach my $zsk (@zskcurlist)
   {
- @args = ($MV, "$zsk.*", $zskdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?)
- }
+ System("$MV $zsk.* $zskdir");
   }

   foreach my $zsk (@zskpublist)
   {
- @args = ($MV, "$zsk.*", $zskdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?)
- }
+ System("$MV $zsk.* $zskdir");
   }

   foreach my $zsk (@zsknewlist)
   {
- @args = ($MV, "$zsk.*", $zskdir);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?)
- }
+ System("$MV $zsk.* $zskdir");
   }
  }

@@ -2291,7 +2242,6 @@
  my $file; # Zone's contents.
  my $flen; # Zone file's length.
  my $newserial; # Zone's new serial number.
- my @args;

  vhigh_print("\n");
  vprint("adding key includes to zone file\n");
@@ -2397,16 +2347,7 @@
   #
   # Copy the zone data to a new file.
   #
- if ($zonefile ne $zoneftmp)
- {
- @args = ($CP, $zonefile, $zoneftmp);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
- }
+ System("$CP $zonefile $zoneftmp") if($zonefile ne $zoneftmp);
   open(ZF,"+< $zonefile");
   @zonestat = stat($zonefile);
  }
@@ -2417,16 +2358,7 @@
   #
   # Copy the zone data to a new file.
   #
- @args = ($CP, $zonefile, $zoneftmp);
- if($zonefile ne $zoneftmp)
- {
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
- }
+ System("$CP $zonefile $zoneftmp") if($zonefile ne $zoneftmp);

   #
   # Get the include-keys section.
@@ -2461,7 +2393,6 @@
  my $status; # Execution return code.
  my $zscmd; # Zone-signing command line.
  my $zcq = '-q'; # Quiet option for zone checker.
- my @args;

  #
  # Get the most recent revoked keys if we're rolling KSKs or if
@@ -2530,8 +2461,8 @@
  #
  vprint("checking zone\n");
  $zcq = '' if($verbose > $VERBOSE_LOW);
- System("$zonecheck $zcopts $zcq $zone $szone");
- if($? != 0)
+ $status = System("$zonecheck $zcopts $zcq $zone $szone");
+ if($status != 0)
  {
   print STDERR "problems with zone signing\n";
   System("$zonecheck $zcopts $zone $szone");
@@ -2661,22 +2592,10 @@
   {
    my $now = time();
    my $newzftmp = "$zoneftmp.$now";
- @args = ($CP, $zoneftmp, $newzftmp);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
+ System("$CP $zoneftmp $newzftmp");
   }

- @args = ($RM, $zoneftmp);
- System(@args);
- if ($? != 0)
- {
- print STDERR "'@args' returned $?\n" if($verbose);
- exit($?);
- }
+ System("$RM $zoneftmp");
  }

  return(0);
joe(user)2101: