Implement encoded URI handling in all methods

Every method opts in to getting the encoded URI passed along while
keeping compat in case we are operated by an older acquire system.

Effectively this is just a change for the http-based methods as the
others just decode the URI as they work with files directly.

13 files changed, 77 insertions, 37 deletions

diff --git a/methods/aptmethod.h b/methods/aptmethod.h
index 67d5a3a0b..7038131cf 100644
--- a/methods/aptmethod.h
+++ b/methods/aptmethod.h
@@ -475,6 +475,18 @@ protected:
 	 QueueBack = Queue;
       delete Tmp;
    }
+   static std::string URIEncode(std::string const &part)
+   {
+      // The "+" is encoded as a workaround for an S3 bug (LP#1003633 and LP#1086997)
+      return QuoteString(part, _config->Find("Acquire::URIEncode", "+~ ").c_str());
+   }
+
+   static std::string DecodeSendURI(std::string const &part)
+   {
+      if (_config->FindB("Acquire::Send-URI-Encoded", false))
+	 return DeQuoteString(part);
+      return part;
+   }
 
    aptMethod(std::string &&Binary, char const *const Ver, unsigned long const Flags) APT_NONNULL(3)
        : pkgAcqMethod(Ver, Flags), Binary(Binary), SeccompFlags(0), methodNames({Binary})
diff --git a/methods/basehttp.cc b/methods/basehttp.cc
index b8ab73155..8aac1090c 100644
--- a/methods/basehttp.cc
+++ b/methods/basehttp.cc
@@ -283,6 +283,14 @@ void ServerState::Reset()						/*{{{*/
 /* We look at the header data we got back from the server and decide what
    to do. Returns DealWithHeadersResult (see http.h for details).
  */
+static std::string fixURIEncoding(std::string const &part)
+{
+   // if the server sends a space this is not an encoded URI
+   // so other clients seem to encode it and we do it as well
+   if (part.find_first_of(" ") != std::string::npos)
+      return aptMethod::URIEncode(part);
+   return part;
+}
 BaseHttpMethod::DealWithHeadersResult
 BaseHttpMethod::DealWithHeaders(FetchResult &Res, RequestState &Req)
 {
@@ -318,7 +326,10 @@ BaseHttpMethod::DealWithHeaders(FetchResult &Res, RequestState &Req)
             NextURI = URI::SiteOnly(Uri);
 	 else
 	    NextURI.clear();
-	 NextURI.append(DeQuoteString(Req.Location));
+	 if (_config->FindB("Acquire::Send-URI-Encoded", false))
+	    NextURI.append(fixURIEncoding(Req.Location));
+	 else
+	    NextURI.append(DeQuoteString(Req.Location));
 	 if (Queue->Uri == NextURI)
 	 {
 	    SetFailReason("RedirectionLoop");
@@ -331,8 +342,12 @@ BaseHttpMethod::DealWithHeaders(FetchResult &Res, RequestState &Req)
       }
       else
       {
-	 NextURI = DeQuoteString(Req.Location);
-	 URI tmpURI(NextURI);
+	 bool const SendURIEncoded = _config->FindB("Acquire::Send-URI-Encoded", false);
+	 if (not SendURIEncoded)
+	    Req.Location = DeQuoteString(Req.Location);
+	 URI tmpURI(Req.Location);
+	 if (SendURIEncoded)
+	    tmpURI.Path = fixURIEncoding(tmpURI.Path);
 	 if (tmpURI.Access.find('+') != std::string::npos)
 	 {
 	    _error->Error("Server tried to trick us into using a specific implementation: %s", tmpURI.Access.c_str());
@@ -340,6 +355,7 @@ BaseHttpMethod::DealWithHeaders(FetchResult &Res, RequestState &Req)
 	       return ERROR_WITH_CONTENT_PAGE;
 	    return ERROR_UNRECOVERABLE;
 	 }
+	 NextURI = tmpURI;
 	 URI Uri(Queue->Uri);
 	 if (Binary.find('+') != std::string::npos)
 	 {
diff --git a/methods/cdrom.cc b/methods/cdrom.cc
index d836e1315..f534a06f5 100644
--- a/methods/cdrom.cc
+++ b/methods/cdrom.cc
@@ -57,7 +57,7 @@ class CDROMMethod : public aptMethod
 /* */
 CDROMMethod::CDROMMethod() : aptMethod("cdrom", "1.0",SingleInstance | LocalOnly |
 					  SendConfig | NeedsCleanup |
-					  Removable),
+					  Removable | SendURIEncoded),
                                           DatabaseLoaded(false),
 					  Debug(false),
                                           MountedByApt(false)
@@ -175,7 +175,7 @@ bool CDROMMethod::Fetch(FetchItem *Itm)
    FetchResult Res;
 
    URI Get(Itm->Uri);
-   string File = Get.Path;
+   std::string const File = DecodeSendURI(Get.Path);
    Debug = DebugEnabled();
 
    if (Debug)
diff --git a/methods/copy.cc b/methods/copy.cc
index 9a8665446..82eed150c 100644
--- a/methods/copy.cc
+++ b/methods/copy.cc
@@ -29,7 +29,7 @@ class CopyMethod : public aptMethod
    virtual bool Fetch(FetchItem *Itm) APT_OVERRIDE;
 
    public:
-   CopyMethod() : aptMethod("copy", "1.0", SingleInstance | SendConfig)
+   CopyMethod() : aptMethod("copy", "1.0", SingleInstance | SendConfig | SendURIEncoded)
    {
       SeccompFlags = aptMethod::BASE;
    }
@@ -41,7 +41,7 @@ class CopyMethod : public aptMethod
 bool CopyMethod::Fetch(FetchItem *Itm)
 {
    // this ensures that relative paths work in copy
-   std::string const File = Itm->Uri.substr(Itm->Uri.find(':')+1);
+   std::string const File = DecodeSendURI(Itm->Uri.substr(Itm->Uri.find(':')+1));
 
    // Stat the file and send a start message
    struct stat Buf;
diff --git a/methods/file.cc b/methods/file.cc
index 80e47f1ad..b2fe133f2 100644
--- a/methods/file.cc
+++ b/methods/file.cc
@@ -32,7 +32,7 @@ class FileMethod : public aptMethod
    virtual bool Fetch(FetchItem *Itm) APT_OVERRIDE;
 
    public:
-   FileMethod() : aptMethod("file", "1.0", SingleInstance | SendConfig | LocalOnly)
+   FileMethod() : aptMethod("file", "1.0", SingleInstance | SendConfig | LocalOnly | SendURIEncoded)
    {
       SeccompFlags = aptMethod::BASE;
    }
@@ -44,7 +44,7 @@ class FileMethod : public aptMethod
 bool FileMethod::Fetch(FetchItem *Itm)
 {
    URI Get(Itm->Uri);
-   std::string File = Get.Path;
+   std::string const File = DecodeSendURI(Get.Path);
    FetchResult Res;
    if (Get.Host.empty() == false)
       return _error->Error(_("Invalid URI, local URIS must not start with //"));
diff --git a/methods/ftp.cc b/methods/ftp.cc
index 98398341e..16236232a 100644
--- a/methods/ftp.cc
+++ b/methods/ftp.cc
@@ -988,7 +988,7 @@ bool FTPConn::Get(const char *Path,FileFd &To,unsigned long long Resume,
 // FtpMethod::FtpMethod - Constructor					/*{{{*/
 // ---------------------------------------------------------------------
 /* */
-FtpMethod::FtpMethod() : aptAuthConfMethod("ftp", "1.0", SendConfig)
+FtpMethod::FtpMethod() : aptAuthConfMethod("ftp", "1.0", SendConfig | SendURIEncoded)
 {
    SeccompFlags = aptMethod::BASE | aptMethod::NETWORK;
    signal(SIGTERM,SigTerm);
@@ -1038,7 +1038,7 @@ bool FtpMethod::Configuration(string Message)
 bool FtpMethod::Fetch(FetchItem *Itm)
 {
    URI Get(Itm->Uri);
-   const char *File = Get.Path.c_str();
+   auto const File = DecodeSendURI(Get.Path);
    FetchResult Res;
    Res.Filename = Itm->DestFile;
    Res.IMSHit = false;
@@ -1070,8 +1070,8 @@ bool FtpMethod::Fetch(FetchItem *Itm)
    // Get the files information
    Status(_("Query"));
    unsigned long long Size;
-   if (Server->Size(File,Size) == false ||
-       Server->ModTime(File,FailTime) == false)
+   if (not Server->Size(File.c_str(), Size) ||
+       not Server->ModTime(File.c_str(), FailTime))
    {
       Fail(true);
       return true;
@@ -1119,7 +1119,7 @@ bool FtpMethod::Fetch(FetchItem *Itm)
       FailFd = Fd.Fd();
       
       bool Missing;
-      if (Server->Get(File,Fd,Res.ResumePoint,Hash,Missing,Itm->MaximumSize,this) == false)
+      if (not Server->Get(File.c_str(), Fd, Res.ResumePoint, Hash, Missing, Itm->MaximumSize, this))
       {
 	 Fd.Close();
 
diff --git a/methods/gpgv.cc b/methods/gpgv.cc
index 5597e7cff..08d030a17 100644
--- a/methods/gpgv.cc
+++ b/methods/gpgv.cc
@@ -123,7 +123,7 @@ class GPGVMethod : public aptMethod
    protected:
    virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE;
    public:
-   GPGVMethod() : aptMethod("gpgv", "1.1", SingleInstance | SendConfig){};
+   GPGVMethod() : aptMethod("gpgv", "1.1", SingleInstance | SendConfig | SendURIEncoded){};
 };
 static void PushEntryWithKeyID(std::vector<std::string> &Signers, char * const buffer, bool const Debug)
 {
@@ -419,7 +419,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile,
 bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm)
 {
    URI const Get(Itm->Uri);
-   string const Path = Get.Host + Get.Path; // To account for relative paths
+   std::string const Path = DecodeSendURI(Get.Host + Get.Path); // To account for relative paths
    SignersStorage Signers;
 
    std::vector<std::string> keyFpts, keyFiles;
diff --git a/methods/http.cc b/methods/http.cc
index 0ef695166..b6d754037 100644
--- a/methods/http.cc
+++ b/methods/http.cc
@@ -901,9 +901,8 @@ void HttpMethod::SendReq(FetchItem *Itm)
    else
       requesturi = Uri;
 
-   // The "+" is encoded as a workaround for a amazon S3 bug
-   // see LP bugs #1003633 and #1086997.
-   requesturi = QuoteString(requesturi, "+~ ");
+   if (not _config->FindB("Acquire::Send-URI-Encoded", false))
+      requesturi = URIEncode(requesturi);
 
    /* Build the request. No keep-alive is included as it is the default
       in 1.1, can cause problems with proxies, and we are an HTTP/1.1
@@ -1022,7 +1021,7 @@ BaseHttpMethod::DealWithHeadersResult HttpMethod::DealWithHeaders(FetchResult &R
    return FILE_IS_OPEN;
 }
 									/*}}}*/
-HttpMethod::HttpMethod(std::string &&pProg) : BaseHttpMethod(std::move(pProg), "1.2", Pipeline | SendConfig) /*{{{*/
+HttpMethod::HttpMethod(std::string &&pProg) : BaseHttpMethod(std::move(pProg), "1.2", Pipeline | SendConfig | SendURIEncoded) /*{{{*/
 {
    SeccompFlags = aptMethod::BASE | aptMethod::NETWORK;
 
diff --git a/methods/mirror.cc b/methods/mirror.cc
index 3e382e497..5c4d341c2 100644
--- a/methods/mirror.cc
+++ b/methods/mirror.cc
@@ -97,7 +97,7 @@ class MirrorMethod : public aptMethod /*{{{*/
    void DealWithPendingItems(std::vector<std::string> const &baseuris, MirrorListInfo const &info, FetchItem *const Itm, std::function<void()> handler);
 
    public:
-   explicit MirrorMethod(std::string &&pProg) : aptMethod(std::move(pProg), "2.0", SingleInstance | Pipeline | SendConfig | AuxRequests), genrng(clock())
+   explicit MirrorMethod(std::string &&pProg) : aptMethod(std::move(pProg), "2.0", SingleInstance | Pipeline | SendConfig | AuxRequests | SendURIEncoded), genrng(clock())
    {
       SeccompFlags = aptMethod::BASE | aptMethod::DIRECTORY;
    }
diff --git a/methods/rred.cc b/methods/rred.cc
index 2164cd19e..981364a9e 100644
--- a/methods/rred.cc
+++ b/methods/rred.cc
@@ -604,7 +604,7 @@ class RredMethod : public aptMethod {
       virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE {
 	 Debug = DebugEnabled();
 	 URI Get(Itm->Uri);
-	 std::string Path = Get.Host + Get.Path; // rred:/path - no host
+	 std::string Path = DecodeSendURI(Get.Host + Get.Path); // rred:/path - no host
 
 	 FetchResult Res;
 	 Res.Filename = Itm->DestFile;
@@ -750,7 +750,7 @@ class RredMethod : public aptMethod {
       }
 
    public:
-   RredMethod() : aptMethod("rred", "2.0", SendConfig), Debug(false)
+   RredMethod() : aptMethod("rred", "2.0", SendConfig | SendURIEncoded), Debug(false)
    {
       SeccompFlags = aptMethod::BASE | aptMethod::DIRECTORY;
    }
diff --git a/methods/rsh.cc b/methods/rsh.cc
index cc42b43e7..09d72cbc5 100644
--- a/methods/rsh.cc
+++ b/methods/rsh.cc
@@ -382,7 +382,7 @@ bool RSHConn::Get(const char *Path,FileFd &To,unsigned long long Resume,
 									/*}}}*/
 
 // RSHMethod::RSHMethod - Constructor					/*{{{*/
-RSHMethod::RSHMethod(std::string &&pProg) : aptMethod(std::move(pProg),"1.0",SendConfig)
+RSHMethod::RSHMethod(std::string &&pProg) : aptMethod(std::move(pProg),"1.0",SendConfig | SendURIEncoded)
 {
    signal(SIGTERM,SigTerm);
    signal(SIGINT,SigTerm);
@@ -434,7 +434,7 @@ void RSHMethod::SigTerm(int)
 bool RSHMethod::Fetch(FetchItem *Itm)
 {
    URI Get(Itm->Uri);
-   const char *File = Get.Path.c_str();
+   auto const File = DecodeSendURI(Get.Path);
    FetchResult Res;
    Res.Filename = Itm->DestFile;
    Res.IMSHit = false;
@@ -458,8 +458,8 @@ bool RSHMethod::Fetch(FetchItem *Itm)
 
    // Get the files information
    unsigned long long Size;
-   if (Server->Size(File,Size) == false ||
-       Server->ModTime(File,FailTime) == false)
+   if (not Server->Size(File.c_str(), Size) ||
+       not Server->ModTime(File.c_str(), FailTime))
    {
       //Fail(true);
       //_error->Error(_("File not found")); // Will be handled by Size
@@ -505,7 +505,7 @@ bool RSHMethod::Fetch(FetchItem *Itm)
       FailFd = Fd.Fd();
 
       bool Missing;
-      if (Server->Get(File,Fd,Res.ResumePoint,Hash,Missing,Res.Size) == false)
+      if (not Server->Get(File.c_str(), Fd, Res.ResumePoint, Hash, Missing, Res.Size))
       {
 	 Fd.Close();
 
diff --git a/methods/store.cc b/methods/store.cc
index 1b0f07947..8ffc7b93d 100644
--- a/methods/store.cc
+++ b/methods/store.cc
@@ -36,7 +36,7 @@ class StoreMethod : public aptMethod
 
    public:
 
-   explicit StoreMethod(std::string &&pProg) : aptMethod(std::move(pProg),"1.2",SingleInstance | SendConfig)
+   explicit StoreMethod(std::string &&pProg) : aptMethod(std::move(pProg),"1.2",SingleInstance | SendConfig | SendURIEncoded)
    {
       SeccompFlags = aptMethod::BASE;
       if (Binary != "store")
@@ -64,7 +64,7 @@ static bool OpenFileWithCompressorByName(FileFd &fileFd, std::string const &File
 bool StoreMethod::Fetch(FetchItem *Itm)					/*{{{*/
 {
    URI Get(Itm->Uri);
-   std::string Path = Get.Host + Get.Path; // To account for relative paths
+   std::string Path = DecodeSendURI(Get.Host + Get.Path); // To account for relative paths
    
    FetchResult Res;
    Res.Filename = Itm->DestFile;
diff --git a/test/integration/test-cve-2019-3462-dequote-injection b/test/integration/test-cve-2019-3462-dequote-injection
index 74ab03ba5..23cef4fae 100755
--- a/test/integration/test-cve-2019-3462-dequote-injection
+++ b/test/integration/test-cve-2019-3462-dequote-injection
@@ -16,9 +16,16 @@ SHA256="DEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF"
 changetowebserver
 
 runwithbaduri() {
-	webserverconfig aptwebserver::redirect::replace::alpha_1_all.deb "$1"
+	local BADURI="$1"
+	local ERRMSG="$2"
+	shift 2
+	local BADFETCH="http://localhost:${APTHTTPPORT}/pool/alpha_1_all.deb"
+	if [ "$#" = '0' ]; then
+		BADFETCH="http://localhost:${APTHTTPPORT}/pool/$BADURI"
+	fi
+	webserverconfig aptwebserver::redirect::replace::alpha_1_all.deb "$BADURI"
 
-	testsuccess apt update -o debug::http=1 -o debug::pkgacquire::worker=1
+	testsuccess apt update -o debug::http=1 -o debug::pkgacquire::worker=1 "$@"
 
 	testfailureequal "Reading package lists...
 Building dependency tree...
@@ -28,13 +35,19 @@ The following NEW packages will be installed:
 Need to get 20.7 kB of archives.
 After this operation, 11.3 kB of additional disk space will be used.
 Err:1 http://localhost:${APTHTTPPORT} unstable/main all alpha all 1
-  SECURITY: URL redirect target contains control characters, rejecting.
-E: Failed to fetch http://localhost:${APTHTTPPORT}/pool/alpha_1_all.deb  SECURITY: URL redirect target contains control characters, rejecting.
-E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?" aptget install alpha
+  $ERRMSG
+E: Failed to fetch $BADFETCH  $ERRMSG
+E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?" aptget install alpha "$@"
 }
-runwithbaduri "beeta_1_all.deb%0a%0a201%20URI%20Done%0aURI:%20http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%0aFilename:%20${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%0aSize:%2020672%0aLast-Modified:%20Fri,%2018%20Jan%202019%2009:52:02%20+0000%0aSHA256-Hash:%20${SHA256}%0aChecksum-FileSize-Hash:%2012345%0a%0a%0a"
+runwithbaduri "beeta_1_all.deb%0a%0a201%20URI%20Done%0aURI:%20http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%0aFilename:%20${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%0aSize:%2020672%0aLast-Modified:%20Fri,%2018%20Jan%202019%2009:52:02%20+0000%0aSHA256-Hash:%20${SHA256}%0aChecksum-FileSize-Hash:%2012345%0a%0a%0a" 'SECURITY: URL redirect target contains control characters, rejecting.' -o Acquire::Send-URI-Encoded=false
+rm -rf rootdir/var/lib/apt/lists
+runwithbaduri "beeta_1_all.deb%250a%250a201%2520URI%2520Done%250aURI:%2520http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%250aFilename:%2520${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%250aSize:%252020672%250aLast-Modified:%2520Fri,%252018%2520Jan%25202019%252009:52:02%2520+0000%250aSHA256-Hash:%2520${SHA256}%250aChecksum-FileSize-Hash:%252012345%250a%250a%0a" 'SECURITY: URL redirect target contains control characters, rejecting.' -o Acquire::Send-URI-Encoded=false
+
+# without de- and reencoding, we just trigger an error in our webserver as it refuses URIs containing '//'
+rm -rf rootdir/var/lib/apt/lists
+runwithbaduri "beeta_1_all.deb%0a%0a201%20URI%20Done%0aURI:%20http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%0aFilename:%20${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%0aSize:%2020672%0aLast-Modified:%20Fri,%2018%20Jan%202019%2009:52:02%20+0000%0aSHA256-Hash:%20${SHA256}%0aChecksum-FileSize-Hash:%2012345%0a%0a%0a" '400  Bad Request'
 rm -rf rootdir/var/lib/apt/lists
-runwithbaduri "beeta_1_all.deb%250a%250a201%2520URI%2520Done%250aURI:%2520http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%250aFilename:%2520${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%250aSize:%252020672%250aLast-Modified:%2520Fri,%252018%2520Jan%25202019%252009:52:02%2520+0000%250aSHA256-Hash:%2520${SHA256}%250aChecksum-FileSize-Hash:%252012345%250a%250a%0a"
+runwithbaduri "beeta_1_all.deb%250a%250a201%2520URI%2520Done%250aURI:%2520http://localhost:${APTHTTPPORT}/pool/beeta_1_all.deb%250aFilename:%2520${TMPWORKINGDIRECTORY}/rootdir/var/cache/apt/archives/partial/alpha_1_all.deb%250aSize:%252020672%250aLast-Modified:%2520Fri,%252018%2520Jan%25202019%252009:52:02%2520+0000%250aSHA256-Hash:%2520${SHA256}%250aChecksum-FileSize-Hash:%252012345%250a%250a%0a" '400  Bad Request'
 
 # For reference, the following is the original reproducer/bug. It has
 # been disabled using exit 0, as it will fail in fixed versions.