[PATCH setup draft 0/4] Improve setup.ini validation

[PATCH setup draft 0/4] Improve setup.ini validation

[Ken Brown]

This patch series presupposes the one posted starting at [1].

Currently, signatures are verified using the cygwin signing key and
other keys supplied by the user.  Validation with any key is accepted.
This patch series makes the following changes:

 - For official cygwin mirrors (those listed in mirrors.lst), only the
   cygwin key is tried.

 - For purported private mirrors (from the "last-mirror" user setting
   or the "Add URL box" or the command line), the cygwin key is tried
   first.  If this fails, then the remaining keys are tried.  If one
   of these succeeds, then the site is silently reclassified (with a
   message in the log file) as a 'user site' rather than a 'mirror'.
   The change takes effect on the next setup run or when the user
   selects 'Back'.

 - If the user turns off signature validation with the -X option, a
   weaker check is done: We look for "release: cygwin" in the
   setup.ini file.  If that fails for an official mirror, the file is
   rejected.  If it fails for a purported private mirror, the site is
   silently reclassified, as above.

The reclassification is done silently because it could easily be
necessary, through no fault of the user.  There are three reasons for
this.  First, the distinction between mirrors and user sites is new,
and it will take time for users to become accustomed to it.  Second,
for setup.rc files that were written before [1], "last-mirror"
includes all selected sites, whether mirrors or not.  Finally, sites
specified on the command line are initially assumed to be mirrors
until we can perform the above checks.

[1] https://sourceware.org/ml/cygwin-apps/2017-12/msg00051.html

Ken Brown (4):
  Allow validation of signatures using the cygwin key only
  Insist on cygwin signing key for official mirrors
  Try cygwin signing key for private mirrors
  If signature validation is turned off, check 'release:' tag

 crypto.cc  |  5 ++--
 crypto.h   |  3 ++-
 ini.cc     | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 res.rc     |  1 +
 resource.h |  1 +
 site.h     |  2 ++
 6 files changed, 87 insertions(+), 12 deletions(-)

-- 
2.15.1

[PATCH setup draft 1/4] Allow validation of signatures using the cygwin key only

[Ken Brown]

Add an optional argument 'main_key_only' to
crypto.cc:verify_ini_file_sig() and ini.cc:check_ini_sig().  The
argument is 'false' by default.  If it is 'true', validation uses the
official cygwin signing key only.
---
 crypto.cc | 5 +++--
 crypto.h  | 3 ++-
 ini.cc    | 5 +++--
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/crypto.cc b/crypto.cc
index 5a10e16..6a97acb 100644
--- a/crypto.cc
+++ b/crypto.cc
@@ -429,7 +429,8 @@ add_key_from_sexpr (gcry_sexp_t key)
 
 /*  Verify the signature on an ini file.  Takes care of all key-handling.  */
 bool
-verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file, HWND owner)
+verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file,
+		     HWND owner, bool main_key_only)
 {
   /*  DSA public key in s-expr format.  */
   gcry_sexp_t dsa_key;
@@ -629,7 +630,7 @@ verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file, HWND owner)
       // Well, we're actually there!  Try it against the main key.
       rv = gcry_pk_verify (dsa_sig, dsa_hash, dsa_key);
       // If not that, try any supplied on the commandline.
-      if (rv != GPG_ERR_NO_ERROR)
+      if (rv != GPG_ERR_NO_ERROR && !main_key_only)
 	{
 	  std::vector<gcry_sexp_t>::iterator it;
 	  for (it = keys_to_try.begin (); it < keys_to_try.end (); ++it)
diff --git a/crypto.h b/crypto.h
index 860df6c..9725b19 100644
--- a/crypto.h
+++ b/crypto.h
@@ -30,7 +30,8 @@ class io_stream;
   file in another.  It is called from ini.cc/do_remote_ini() and returns
   true if the signature verified OK; if it returns false, you MUST NOT
   use the failed ini file - doubly so if it's a compressed stream!  */
-extern bool verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file, HWND owner);
+extern bool verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file,
+				 HWND owner, bool main_key_only = false);
 
 /*
 5.2.2.  Version 3 Signature Packet Format
diff --git a/ini.cc b/ini.cc
index f021ed2..18ab2e3 100644
--- a/ini.cc
+++ b/ini.cc
@@ -172,7 +172,8 @@ decompress_ini (io_stream *ini_file)
 
 static io_stream*
 check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
-	       bool& sig_fail, const char* site, const char* sig_name, HWND owner)
+	       bool& sig_fail, const char* site, const char* sig_name,
+	       HWND owner, bool main_key_only = false)
 {
   /* Unless the NoVerifyOption is set, check the signature for the
      current setup and record the result.  On a failed signature check
@@ -192,7 +193,7 @@ check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
 	    sig_fail = true;
 	  }
       }
-      else if (!verify_ini_file_sig (ini_file, ini_sig_file, owner))
+      else if (!verify_ini_file_sig (ini_file, ini_sig_file, owner, main_key_only))
 	{
 	  note (owner, IDS_SIG_INVALID, sig_name, site);
 	  delete ini_sig_file;
-- 
2.15.1

[PATCH setup draft 3/4] Try cygwin signing key for private mirrors

[Ken Brown]

If validation with the cygwin signing key fails for a purported
private mirror, retry with other supplied keys.  If this succeeds,
silently change the status of the site to "user site" and put a note
in the log file.  This change will take effect on the next setup run
or if the user selects 'Back'.

This uses a new optional argument 'retry_sig_ok' to
ini.cc:check_ini_sig().  If this is 'true', don't destroy ini_file and
ini_sig_file on a validation failure.
---
 ini.cc | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 site.h |  2 ++
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/ini.cc b/ini.cc
index 4be8263..62b7e83 100644
--- a/ini.cc
+++ b/ini.cc
@@ -18,6 +18,8 @@
    flex parsers are provided also.  We check to see if this setup.ini
    is older than the one we used last time, and if so, warn the user. */
 
+#include <algorithm>
+
 #include "ini.h"
 
 #include "csu_util/rfc1738.h"
@@ -173,7 +175,7 @@ decompress_ini (io_stream *ini_file)
 static io_stream*
 check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
 	       bool& sig_fail, const char* site, const char* sig_name,
-	       HWND owner, bool main_key_only = false)
+	       HWND owner, bool main_key_only = false, bool retry_sig_ok = false)
 {
   /* Unless the NoVerifyOption is set, check the signature for the
      current setup and record the result.  On a failed signature check
@@ -195,12 +197,15 @@ check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
       }
       else if (!verify_ini_file_sig (ini_file, ini_sig_file, owner, main_key_only))
 	{
-	  note (owner, IDS_SIG_INVALID, sig_name, site);
-	  delete ini_sig_file;
-	  ini_sig_file = NULL;
-	  delete ini_file;
-	  ini_file = NULL;
 	  sig_fail = true;
+	  if (!retry_sig_ok)
+	    {
+	      note (owner, IDS_SIG_INVALID, sig_name, site);
+	      delete ini_sig_file;
+	      ini_sig_file = NULL;
+	      delete ini_file;
+	      ini_file = NULL;
+	    }
 	}
     }
   return ini_file;
@@ -265,6 +270,27 @@ do_local_ini (HWND owner)
   return ini_error;
 }
 
+static void
+mirror_warn (site_list_type site)
+{
+  Log (LOG_BABBLE) << "Signature validation failed for " << site.url
+		   << " using the cygwin key but succeeded using other keys.  "
+		   << endLog;
+  Log (LOG_BABBLE) << "Changing status from 'mirror' to 'user site'." << endLog;
+  SiteList::iterator i = find (all_site_list.begin (), all_site_list.end (), site);
+  if (i != all_site_list.end ())
+    {
+      all_site_list.erase (i);
+      site_list_type newsite (site.url, "", "", "", false, false);
+      all_site_list.push_back (newsite);
+      selected_usersite_list.push_back (newsite);
+      SiteList::iterator j = find (selected_mirror_list.begin (),
+				   selected_mirror_list.end (), site);
+      if (j != selected_mirror_list.end ())
+	selected_mirror_list.erase (j);
+    }
+}
+
 static bool
 do_remote_ini (HWND owner)
 {
@@ -293,14 +319,33 @@ do_remote_ini (HWND owner)
 	  ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
 	  ini_file = get_url_to_membuf (current_ini_name, owner);
 
-	  // Official mirrors must be signed by the cygwin key.
-	  bool main_key_only = n->from_mirrors_lst;
+	  // Mirrors should be signed by the cygwin key.
+	  bool main_key_only = n->is_mirror;
+
+	  // If this fails for a purported private mirror, allow a
+	  // retry with additional keys; if this succeeds, reclassify
+	  // the site as a user site.
+	  bool retry_sig_ok = n->is_mirror && !n->from_mirrors_lst;
+	retry:
 	  ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
-				    n->url.c_str (), current_ini_sig_name.c_str (), owner, main_key_only);
+				    n->url.c_str (),
+				    current_ini_sig_name.c_str (), owner,
+				    main_key_only, retry_sig_ok);
+	  if (retry_sig_ok && sig_fail)
+	    {
+	      sig_fail = false;
+	      retry_sig_ok = false;
+	      main_key_only = false;
+	      goto retry;
+	    }
 
 	  // stop searching as soon as we find a setup file
 	  if (ini_file)
-	    break;
+	    {
+	      if (n->is_mirror && !main_key_only)
+		mirror_warn (*n);
+	      break;
+	    }
 	}
       if (ini_file)
 	ini_file = decompress_ini (ini_file);
diff --git a/site.h b/site.h
index 3525931..5b01829 100644
--- a/site.h
+++ b/site.h
@@ -79,6 +79,8 @@ typedef std::vector <site_list_type> SiteList;
 
 /* user chosen sites */
 extern SiteList site_list;
+extern SiteList selected_mirror_list;
+extern SiteList selected_usersite_list;
 /* potential sites */
 extern SiteList all_site_list;
 
-- 
2.15.1

[PATCH setup draft 4/4] If signature validation is turned off, check 'release:' tag

[Ken Brown]

If we aren't doing signature validation, look instead for "release:
cygwin" in the setup.ini files.  If this fails for an official mirror,
reject the mirror.  If it fails for a purported private mirror,
silently change the status of the site to "user site" and put a note
in the log file.  This change will take effect on the next setup run
or if the user selects 'Back'.
---
 ini.cc     | 29 ++++++++++++++++++++++++-----
 res.rc     |  1 +
 resource.h |  1 +
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/ini.cc b/ini.cc
index 62b7e83..3cffa82 100644
--- a/ini.cc
+++ b/ini.cc
@@ -271,11 +271,15 @@ do_local_ini (HWND owner)
 }
 
 static void
-mirror_warn (site_list_type site)
+mirror_warn (site_list_type site, bool sig)
 {
-  Log (LOG_BABBLE) << "Signature validation failed for " << site.url
-		   << " using the cygwin key but succeeded using other keys.  "
-		   << endLog;
+  if (sig)
+    Log (LOG_BABBLE) << "Signature validation failed for " << site.url
+		     << " using the cygwin key but succeeded using other keys.  "
+		     << endLog;
+  else
+    Log (LOG_BABBLE) << "setup.ini from " << site.url
+		     << " is not from the cygwin release." << endLog;
   Log (LOG_BABBLE) << "Changing status from 'mirror' to 'user site'." << endLog;
   SiteList::iterator i = find (all_site_list.begin (), all_site_list.end (), site);
   if (i != all_site_list.end ())
@@ -343,7 +347,7 @@ do_remote_ini (HWND owner)
 	  if (ini_file)
 	    {
 	      if (n->is_mirror && !main_key_only)
-		mirror_warn (*n);
+		mirror_warn (*n, true);
 	      break;
 	    }
 	}
@@ -369,6 +373,21 @@ do_remote_ini (HWND owner)
 	    }
 	  else
 	    {
+	      if (NoVerifyOption && n->is_mirror && aBuilder.release != "cygwin")
+		{
+		  if (n->from_mirrors_lst)
+		    {
+		      // Reject setup.ini.
+		      note (owner, IDS_SETUPINI_NOTCYGWIN,
+			    SetupBaseName.c_str (), n->url.c_str ());
+		      delete ini_file;
+		      ini_file = NULL;
+		      continue;
+		    }
+		  else
+		    mirror_warn (*n, false);
+		}
+
 	      /* save known-good setup.ini locally */
 	      const std::string fp = "file://" + local_dir + "/" +
 				      rfc1738_escape_part (n->url) +
diff --git a/res.rc b/res.rc
index 14f1109..cd202e0 100644
--- a/res.rc
+++ b/res.rc
@@ -562,6 +562,7 @@ BEGIN
     IDS_MIRROR_LST          "http://cygwin.com/mirrors.lst"
     IDS_ERR_OPEN_WRITE      "Can't open %s for writing: %s"
     IDS_SETUPINI_MISSING    "Unable to get %s from <%s>"
+    IDS_SETUPINI_NOTCYGWIN  "%s from <%s> is not from cygwin release"
     IDS_OLD_SETUPINI        "This setup.ini is older than the one you used last time you installed cygwin.  Proceed anyway?"
     IDS_NOTHING_INSTALLED   "Nothing needed to be installed"
     IDS_INSTALL_COMPLETE    "Installation Complete"
diff --git a/resource.h b/resource.h
index 79575fb..666e93b 100644
--- a/resource.h
+++ b/resource.h
@@ -39,6 +39,7 @@
 #define IDS_ELEVATED			  139
 #define IDS_INSTALLEDB_VERSION            140
 #define IDS_DOWNLOAD_INCOMPLETE_EXIT      141
+#define IDS_SETUPINI_NOTCYGWIN            142
 
 // Dialogs
 
-- 
2.15.1

[PATCH setup draft 2/4] Insist on cygwin signing key for official mirrors

[Ken Brown]

If a mirror comes from mirrors.lst, validate the signature using the
cygwin signing key only.
---
 ini.cc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ini.cc b/ini.cc
index 18ab2e3..4be8263 100644
--- a/ini.cc
+++ b/ini.cc
@@ -292,8 +292,12 @@ do_remote_ini (HWND owner)
 	  current_ini_sig_name = current_ini_name + ".sig";
 	  ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
 	  ini_file = get_url_to_membuf (current_ini_name, owner);
+
+	  // Official mirrors must be signed by the cygwin key.
+	  bool main_key_only = n->from_mirrors_lst;
 	  ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
-				    n->url.c_str (), current_ini_sig_name.c_str (), owner);
+				    n->url.c_str (), current_ini_sig_name.c_str (), owner, main_key_only);
+
 	  // stop searching as soon as we find a setup file
 	  if (ini_file)
 	    break;
-- 
2.15.1