From 97d6c3b2d05fe0d965657197adf56cc78f9edf81 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 10 Jul 2020 00:02:25 +0200 Subject: 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. --- methods/aptmethod.h | 12 ++++++++++ methods/basehttp.cc | 22 +++++++++++++++--- methods/cdrom.cc | 4 ++-- methods/copy.cc | 4 ++-- methods/file.cc | 4 ++-- methods/ftp.cc | 10 ++++---- methods/gpgv.cc | 4 ++-- methods/http.cc | 7 +++--- methods/mirror.cc | 2 +- methods/rred.cc | 4 ++-- methods/rsh.cc | 10 ++++---- methods/store.cc | 4 ++-- .../test-cve-2019-3462-dequote-injection | 27 ++++++++++++++++------ 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 &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 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 const &baseuris, MirrorListInfo const &info, FetchItem *const Itm, std::function 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. -- cgit v1.2.3