Do not allow going from authenticated to unauthenticated repo

Also rework the way we load the Release file, so it only after
Release.gpg verified the Release file. The rational is that we
never want to load untrusted data into our parsers. Only stuff
verified with gpg or by its hashes get loaded. To load untrusted
data you now need to use apt-get update --allow-unauthenticated.

apt-pkg/acquire-item.cc

@@ -193,6 +193,14 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const
 	 Status = StatError;
 	 // do not report as usually its not the mirrors fault, but Portal/Proxy
 	 break;
+      case SignatureError:
+	 ErrorText = _("Signature error");
+	 Status = StatError;
+	 break;
+      case NotClearsigned:
+         ErrorText = _("Does not start with a cleartext signature");
+	 Status = StatError;
+	 break;
    }
    return false;
 }
@@ -1307,6 +1315,8 @@ void pkgAcqMetaBase::AbortTransaction()
    if(_config->FindB("Debug::Acquire::Transaction", false) == true)
       std::clog << "AbortTransaction: " << TransactionManager << std::endl;
 
+   // ensure the toplevel is in error state too
+   Status = pkgAcquire::Item::StatError;
    for (std::vector<Item*>::iterator I = Transaction.begin();
         I != Transaction.end(); ++I)
    {
@@ -1407,6 +1417,7 @@ pkgAcqMetaSig::pkgAcqMetaSig(pkgAcquire *Owner,          		/*{{{*/
 			     indexRecords* MetaIndexParser) :
    pkgAcqMetaBase(Owner, HashStringList(), TransactionManager), RealURI(URI), 
    MetaIndexParser(MetaIndexParser), MetaIndexFile(MetaIndexFile),
+   URIDesc(URIDesc), ShortDesc(ShortDesc),
    IndexTargets(IndexTargets), AuthPass(false), IMSHit(false)
 {
    DestFile = _config->FindDir("Dir::State::lists") + "partial/";
@@ -1505,13 +1516,35 @@ void pkgAcqMetaSig::Done(string Message,unsigned long long Size, HashStringList
       DestFile += URItoFileName(RealURI);
    }
 
-   Complete = true;
+   // we parse the MetaIndexFile here because at this point we can
+   // trust the data
+   if(AuthPass == true)
+   {
+      // load indexes and queue further downloads
+      MetaIndexParser->Load(MetaIndexFile);
+      ((pkgAcqMetaIndex*)TransactionManager)->QueueIndexes(true);
+   }
 
+   Complete = true;
 }
 									/*}}}*/
 void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/
 {
    string Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI);
+   
+   // FIXME: meh, this is not really elegant
+   string InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12,
+                                         "InRelease");
+   string FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI);
+
+   if(RealFileExists(Final) || RealFileExists(FinalInRelease))
+   {
+      _error->Error("The repository '%s' is no longer signed.",
+                    URIDesc.c_str());
+      Rename(MetaIndexFile, MetaIndexFile+".FAILED");
+      TransactionManager->AbortTransaction();
+      return;
+   }
 
    // this ensures that any file in the lists/ dir is removed by the
    // transaction
@@ -1527,6 +1560,19 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/
          return;
    }
 
+   // only allow going further if the users explicitely wants it
+   if(_config->FindB("APT::Get::AllowUnauthenticated", false))
+   {
+      _error->Warning("Please use --allow-unauthenticated");
+   } 
+   else 
+   {
+      // we parse the indexes here because at this point the user wanted
+      // a repository that may potentially harm him
+      MetaIndexParser->Load(MetaIndexFile);
+      ((pkgAcqMetaIndex*)TransactionManager)->QueueIndexes(true);
+   }
+
    // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor
    if (Cnf->LocalOnly == true || 
        StringToBool(LookupTag(Message,"Transient-Failure"),false) == false)
@@ -1547,6 +1593,7 @@ pkgAcqMetaIndex::pkgAcqMetaIndex(pkgAcquire *Owner,			/*{{{*/
 				 const vector<IndexTarget*>* IndexTargets,
 				 indexRecords* MetaIndexParser) :
    pkgAcqMetaBase(Owner, HashStringList(), TransactionManager), RealURI(URI), IndexTargets(IndexTargets),
+   URIDesc(URIDesc), ShortDesc(ShortDesc),
    MetaIndexParser(MetaIndexParser), AuthPass(false), IMSHit(false),
    MetaIndexSigURI(MetaIndexSigURI), MetaIndexSigURIDesc(MetaIndexSigURIDesc),
    MetaIndexSigShortDesc(MetaIndexSigShortDesc)
@@ -1619,13 +1666,7 @@ void pkgAcqMetaIndex::Done(string Message,unsigned long long Size,HashStringList
          // Still more retrieving to do
          return;
 
-      if (SigFile == "")
-      {
-         // load indexes, the signature will downloaded afterwards
-         MetaIndexParser->Load(DestFile);
-         QueueIndexes(true);
-      }
-      else
+      if (SigFile != "")
       {
          // There was a signature file, so pass it to gpgv for
          // verification
@@ -1952,7 +1993,8 @@ void pkgAcqMetaIndex::Failed(string Message,
 
    /* Always move the meta index, even if gpgv failed. This ensures
     * that PackageFile objects are correctly filled in */
-   if (FileExists(DestFile)) {
+   if (FileExists(DestFile)) 
+   {
       string FinalFile = _config->FindDir("Dir::State::lists");
       FinalFile += URItoFileName(RealURI);
       /* InRelease files become Release files, otherwise
@@ -1970,13 +2012,19 @@ void pkgAcqMetaIndex::Failed(string Message,
       DestFile = FinalFile;
    }
 
-   // warn if the repository is unsinged
-   _error->Warning(_("The data from '%s' is not signed. Packages "
-                     "from that repository can not be authenticated."),
-                   URIDesc.c_str());
    // No Release file was present, or verification failed, so fall
    // back to queueing Packages files without verification
-   QueueIndexes(false);
+   // only allow going further if the users explicitely wants it
+   if(_config->FindB("APT::Get::AllowUnauthenticated", false))
+   {
+      // warn if the repository is unsinged
+      _error->Warning(_("The data from '%s' is not signed. Packages "
+                     "from that repository can not be authenticated."),
+                   URIDesc.c_str());
+      _error->Warning("Please use --allow-unauthenticated");
+   } else {
+      QueueIndexes(false);
+   }
 }
 									/*}}}*/
 
@@ -2060,7 +2108,8 @@ void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long Size,
    if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile))
    {
       pkgAcquire::Item::Failed(Message, Cnf);
-      ErrorText = _("Does not start with a cleartext signature");
+      RenameOnError(NotClearsigned);
+      TransactionManager->AbortTransaction();
       return;
    }
    pkgAcqMetaIndex::Done(Message, Size, Hashes, Cnf);

apt-pkg/acquire-item.h

@@ -312,7 +312,9 @@ class pkgAcquire::Item : public WeakPointable
    enum RenameOnErrorState {
       HashSumMismatch,
       SizeMismatch,
-      InvalidFormat
+      InvalidFormat,
+      SignatureError,
+      NotClearsigned,
    };
 
    /** \brief Rename failed file and set error
@@ -367,6 +369,67 @@ class pkgAcqMetaBase  : public pkgAcquire::Item
       : Item(Owner, ExpectedHashes, TransactionManager) {};
 };
 
+/** \brief An acquire item that downloads the detached signature	{{{
+ *  of a meta-index (Release) file, then queues up the release
+ *  file itself.
+ *
+ *  \todo Why protected members?
+ *
+ *  \sa pkgAcqMetaIndex
+ */
+class pkgAcqMetaSig : public pkgAcqMetaBase
+{
+   void *d;
+
+   protected:
+
+   /** \brief The URI of the signature file.  Unlike Desc.URI, this is
+    *  never modified; it is used to determine the file that is being
+    *  downloaded.
+    */
+   std::string RealURI;
+
+   std::string URIDesc;
+   std::string ShortDesc;
+
+   /** \brief A package-system-specific parser for the meta-index file. */
+   indexRecords* MetaIndexParser;
+
+   /** \brief The file we need to verify */
+   std::string MetaIndexFile;
+
+   /** \brief The index files which should be looked up in the meta-index
+    *  and then downloaded.
+    *
+    *  \todo Why a list of pointers instead of a list of structs?
+    */
+   const std::vector<IndexTarget*>* IndexTargets;
+
+   /** \brief If we are in fetching or download state */
+   bool AuthPass;
+
+   /** \brief Was this file already on disk */
+   bool IMSHit;
+
+   public:
+   
+   // Specialized action members
+   virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf);
+   virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes,
+		     pkgAcquire::MethodConfig *Cnf);
+   virtual std::string Custom600Headers() const;
+   virtual std::string DescURI() const {return RealURI; };
+
+   /** \brief Create a new pkgAcqMetaSig. */
+   pkgAcqMetaSig(pkgAcquire *Owner,
+                 pkgAcqMetaBase *TransactionManager,
+                 std::string URI,std::string URIDesc, std::string ShortDesc,
+                 std::string MetaIndexFile,
+		 const std::vector<IndexTarget*>* IndexTargets,
+		 indexRecords* MetaIndexParser);
+   virtual ~pkgAcqMetaSig();
+};
+									/*}}}*/
 
 /** \brief An item that is responsible for downloading the meta-index	{{{
  *  file (i.e., Release) itself and verifying its signature.
@@ -436,15 +499,8 @@ class pkgAcqMetaIndex : public pkgAcqMetaBase
     */
    void AuthDone(std::string Message);
 
-   /** \brief Starts downloading the individual index files.
-    *
-    *  \param verify If \b true, only indices whose expected hashsum
-    *  can be determined from the meta-index will be downloaded, and
-    *  the hashsums of indices will be checked (reporting
-    *  #StatAuthError if there is a mismatch).  If verify is \b false,
-    *  no hashsum checking will be performed.
-    */
-   void QueueIndexes(bool verify);
+   std::string URIDesc;
+   std::string ShortDesc;
 
    /** \brief The URI of the meta-index file for the detached signature */
    std::string MetaIndexSigURI;
@@ -459,7 +515,17 @@ class pkgAcqMetaIndex : public pkgAcqMetaBase
    void Init(std::string URIDesc, std::string ShortDesc);
    
    public:
-   
+
+   /** \brief Starts downloading the individual index files.
+    *
+    *  \param verify If \b true, only indices whose expected hashsum
+    *  can be determined from the meta-index will be downloaded, and
+    *  the hashsums of indices will be checked (reporting
+    *  #StatAuthError if there is a mismatch).  If verify is \b false,
+    *  no hashsum checking will be performed.
+    */
+   void QueueIndexes(bool verify);
+
    // Specialized action members
    virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf);
    virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes,
@@ -999,66 +1065,6 @@ class OptionalIndexTarget : public IndexTarget
 };
 									/*}}}*/
 
-/** \brief An acquire item that downloads the detached signature	{{{
- *  of a meta-index (Release) file, then queues up the release
- *  file itself.
- *
- *  \todo Why protected members?
- *
- *  \sa pkgAcqMetaIndex
- */
-class pkgAcqMetaSig : public pkgAcqMetaBase
-{
-   void *d;
-
-   protected:
-   /** \brief The last good signature file */
-   std::string LastGoodSig;
-
-   /** \brief The URI of the signature file.  Unlike Desc.URI, this is
-    *  never modified; it is used to determine the file that is being
-    *  downloaded.
-    */
-   std::string RealURI;
-
-   /** \brief A package-system-specific parser for the meta-index file. */
-   indexRecords* MetaIndexParser;
-
-   /** \brief The file we need to verify */
-   std::string MetaIndexFile;
-
-   /** \brief The index files which should be looked up in the meta-index
-    *  and then downloaded.
-    *
-    *  \todo Why a list of pointers instead of a list of structs?
-    */
-   const std::vector<IndexTarget*>* IndexTargets;
-
-   /** \brief If we are in fetching or download state */
-   bool AuthPass;
-
-   /** \brief Was this file already on disk */
-   bool IMSHit;
-
-   public:
-   
-   // Specialized action members
-   virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf);
-   virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes,
-		     pkgAcquire::MethodConfig *Cnf);
-   virtual std::string Custom600Headers() const;
-   virtual std::string DescURI() const {return RealURI; };
-
-   /** \brief Create a new pkgAcqMetaSig. */
-   pkgAcqMetaSig(pkgAcquire *Owner,
-                 pkgAcqMetaBase *TransactionManager,
-                 std::string URI,std::string URIDesc, std::string ShortDesc,
-                 std::string MetaIndexFile,
-		 const std::vector<IndexTarget*>* IndexTargets,
-		 indexRecords* MetaIndexParser);
-   virtual ~pkgAcqMetaSig();
-};
-									/*}}}*/
 /** \brief An item that is responsible for fetching a package file.	{{{
  *
  *  If the package file already exists in the cache, nothing will be

test/integration/test-apt-update-nofallback

@@ -0,0 +1,207 @@
+#!/bin/sh
+#
+# ensure we never fallback from a signed to a unsigned repo
+# 
+# hash checks are done in 
+# 
+set -e
+
+simulate_mitm_and_inject_evil_package()
+{
+    rm -f $APTARCHIVE/dists/unstable/InRelease
+    rm -f $APTARCHIVE/dists/unstable/Release.gpg
+    inject_evil_package
+}
+
+inject_evil_package()
+{
+    cat > $APTARCHIVE/dists/unstable/main/binary-i386/Packages <<EOF
+Package: evil
+Installed-Size: 29
+Maintainer: Joe Sixpack <joe@example.org>
+Architecture: all
+Version: 1.0
+Filename: pool/evil_1.0_all.deb
+Size: 1270
+Description: an autogenerated evil package
+EOF
+    # avoid ims hit
+    touch -d '+1hour' aptarchive/dists/unstable/main/binary-i386/Packages
+}
+
+assert_update_is_refused_and_last_good_state_used()
+{
+    testequal "E: The repository 'file: unstable Release.gpg' is no longer signed." aptget update -qq
+
+    assert_repo_is_intact
+}
+
+assert_repo_is_intact()
+{
+    testequal "foo/unstable 2.0 all" apt list -q
+    testsuccess "" aptget install -y -s foo
+    testfailure "" aptget install -y evil
+
+    LISTDIR=rootdir/var/lib/apt/lists
+    if ! ( ls $LISTDIR/*InRelease >/dev/null 2>&1 || 
+            ls $LISTDIR/*Release.gpg >/dev/null 2>&1 ); then
+        echo "Can not find InRelease/Release.gpg in $(ls $LISTDIR)"
+        msgfail
+    fi
+}
+
+setupaptarchive_with_lists_clean()
+{
+    setupaptarchive --no-update
+    rm -f rootdir/var/lib/apt/lists/_*
+    #rm -rf rootdir/var/lib/apt/lists
+}
+
+test_from_inrelease_to_unsigned()
+{
+    # setup archive with InRelease file
+    setupaptarchive_with_lists_clean
+    testsuccess aptget update
+
+    simulate_mitm_and_inject_evil_package
+    assert_update_is_refused_and_last_good_state_used
+}
+
+test_from_release_gpg_to_unsigned()
+{
+    # setup archive with Release/Release.gpg (but no InRelease)
+    setupaptarchive_with_lists_clean
+    rm $APTARCHIVE/dists/unstable/InRelease
+    testsuccess aptget update
+
+    simulate_mitm_and_inject_evil_package
+    assert_update_is_refused_and_last_good_state_used
+}
+
+test_cve_2012_0214()
+{
+    # see https://bugs.launchpad.net/ubuntu/+source/apt/+bug/947108
+    #
+    # it was possible to MITM the download so that InRelease/Release.gpg
+    # are not delivered (404) and a altered Release file was send
+    #
+    # apt left the old InRelease file in /var/lib/apt/lists and downloaded
+    # the unauthenticated Release file too giving the false impression that
+    # Release was authenticated
+    #
+    # Note that this is pretty much impossible nowdays because:
+    # a) InRelease is left as is, not split to InRelease/Release as it was 
+    #    in the old days
+    # b) we refuse to go from signed->unsigned
+    #
+    # Still worth having a regression test the simulates the condition
+
+    # setup archive with InRelease
+    setupaptarchive_with_lists_clean
+    testsuccess aptget update
+
+    # do what CVE-2012-0214 did
+    rm $APTARCHIVE/dists/unstable/InRelease
+    rm $APTARCHIVE/dists/unstable/Release.gpg
+    inject_evil_package
+    # build valid Release file
+    aptftparchive -qq release ./aptarchive > aptarchive/dists/unstable/Release 
+
+    assert_update_is_refused_and_last_good_state_used
+
+    # ensure there is no _Release file downloaded
+    testfailure ls rootdir/var/lib/apt/lists/*_Release
+}
+
+test_subvert_inrelease()
+{
+    # setup archive with InRelease
+    setupaptarchive_with_lists_clean
+    testsuccess aptget update
+
+    # replace InRelease with something else
+    mv $APTARCHIVE/dists/unstable/Release $APTARCHIVE/dists/unstable/InRelease
+
+    testequal "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease  Does not start with a cleartext signature
+
+E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq
+
+    # ensure we keep the repo
+    assert_repo_is_intact
+}
+
+test_inrelease_to_invalid_inrelease()
+{
+    # setup archive with InRelease
+    setupaptarchive_with_lists_clean
+    testsuccess aptget update
+
+    # now remove InRelease and subvert Release do no longer verify
+    sed -i 's/Codename.*/Codename: evil!'/ $APTARCHIVE/dists/unstable/InRelease
+    inject_evil_package
+
+    testequal "W: An error occurred during the signature verification. The repository is not updated and the previous index files will be used. GPG error: file: unstable InRelease: The following signatures were invalid: BADSIG 5A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) <joe@example.org>
+
+W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease  
+
+W: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq
+
+    # ensure we keep the repo
+    assert_repo_is_intact
+    testfailure grep "evil" rootdir/var/lib/apt/lists/*InRelease
+}
+
+test_release_gpg_to_invalid_release_release_gpg()
+{
+    # setup archive with InRelease
+    setupaptarchive_with_lists_clean
+    rm $APTARCHIVE/dists/unstable/InRelease
+    testsuccess aptget update
+
+    # now subvert Release do no longer verify
+    echo "Some evil data" >>  $APTARCHIVE/dists/unstable/Release
+    inject_evil_package
+
+    testequal "E: The repository 'file: unstable Release.gpg' is no longer signed." aptget update -qq
+
+    assert_repo_is_intact
+    testfailure grep "evil" rootdir/var/lib/apt/lists/*Release
+}
+
+
+TESTDIR=$(readlink -f $(dirname $0))
+. $TESTDIR/framework
+
+setupenvironment
+configarchitecture "i386"
+
+# a "normal" package with source and binary
+buildsimplenativepackage 'foo' 'all' '2.0'
+
+# setup the archive and ensure we have a single package that installs fine
+setupaptarchive
+APTARCHIVE=$(readlink -f ./aptarchive)
+assert_repo_is_intact
+
+# test the various cases where a repo may go from signed->unsigned
+msgmsg "test_from_inrelease_to_unsigned"
+test_from_inrelease_to_unsigned
+
+msgmsg "test_from_release_gpg_to_unsigned"
+test_from_release_gpg_to_unsigned
+
+# ensure we do not regress on CVE-2012-0214
+msgmsg "test_cve_2012_0214"
+test_cve_2012_0214
+
+# ensure InRelase can not be subverted
+msgmsg "test_subvert_inrelease"
+test_subvert_inrelease
+
+# ensure we revert to last good state if InRelease does not verify
+msgmsg "test_inrelease_to_invalid_inrelease"
+test_inrelease_to_invalid_inrelease
+
+# ensure we revert to last good state if Release/Release.gpg does not verify
+msgmsg "test_release_gpg_to_invalid_release_release_gpg"
+test_release_gpg_to_invalid_release_release_gpg