From 35af71bc026d85aef4af979aa247e837d91dfc1c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 17:50:05 +0100 Subject: Use error reporting instead of assert in rred patching The rest of our code uses return-code errors and it isn't that nice to crash the rred method on bad patches anyway, so we move to properly reporting errors in our usual way which incidently also helps writing a fuzzer for it. This is not really security relevant though as the patches passed hash verification while they were downloaded, so an attacker has to overtake a trusted repository first which gives plenty better options of attack. --- methods/rred.cc | 156 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 88 insertions(+), 68 deletions(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index 981364a9e..f53f05ad5 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -7,12 +7,15 @@ #include +#ifndef APT_EXCLUDE_RRED_METHOD_CODE #include "aptmethod.h" #include +#include +#endif + #include #include #include -#include #include #include @@ -23,7 +26,7 @@ #include #include -#include +#include #include #include #include @@ -33,7 +36,9 @@ #include -#define BLOCK_SIZE (512*1024) +#ifndef APT_MEMBLOCK_SIZE +#define APT_MEMBLOCK_SIZE (512*1024) +#endif static bool ShowHelp(CommandLine &) { @@ -66,9 +71,9 @@ class MemBlock { char *start; size_t size; char *free; - MemBlock *next; + MemBlock *next = nullptr; - explicit MemBlock(size_t size) : size(size), next(NULL) + explicit MemBlock(size_t size) : size(size) { free = start = new char[size]; } @@ -77,11 +82,7 @@ class MemBlock { public: - MemBlock(void) { - free = start = new char[BLOCK_SIZE]; - size = BLOCK_SIZE; - next = NULL; - } + MemBlock() : MemBlock(APT_MEMBLOCK_SIZE) {} ~MemBlock() { delete [] start; @@ -100,11 +101,9 @@ class MemBlock { for (MemBlock *k = this; k; k = k->next) { if (k->free == last) { if (len <= k->avail()) { - char *n = k->add(src, len); - assert(last == n); - if (last == n) - return NULL; - return n; + char * const n = k->add(src, len); + assert(last == n); // we checked already that the block is big enough, so a new one shouldn't be used + return (last == n) ? nullptr : n; } else { break; } @@ -119,10 +118,10 @@ class MemBlock { char *add(char *src, size_t len) { if (len > avail()) { if (!next) { - if (len > BLOCK_SIZE) { + if (len > APT_MEMBLOCK_SIZE) { next = new MemBlock(len); } else { - next = new MemBlock; + next = new MemBlock(); } } return next->add(src, len); @@ -155,24 +154,27 @@ struct Change { } /* actually, don't write lines from */ - void skip_lines(size_t lines) + bool skip_lines(size_t lines) { while (lines > 0) { char *s = (char*) memchr(add, '\n', add_len); - assert(s != NULL); + if (s == nullptr) + return _error->Error("No line left in add_len data to skip (1)"); s++; add_len -= (s - add); add_cnt--; lines--; if (add_len == 0) { - add = NULL; - assert(add_cnt == 0); - assert(lines == 0); + add = nullptr; + if (add_cnt != 0 || lines != 0) + return _error->Error("No line left in add_len data to skip (2)"); } else { add = s; - assert(add_cnt > 0); + if (add_cnt == 0) + return _error->Error("No line left in add_len data to skip (3)"); } } + return true; } }; @@ -183,7 +185,8 @@ class FileChanges { bool pos_is_okay(void) const { -#ifdef POSDEBUG +#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION + // this isn't unsafe, it is just a moderately expensive check we want to avoid normally size_t cpos = 0; std::list::const_iterator x; for (x = changes.begin(); x != where; ++x) { @@ -208,33 +211,40 @@ class FileChanges { std::list::reverse_iterator rbegin(void) { return changes.rbegin(); } std::list::reverse_iterator rend(void) { return changes.rend(); } - void add_change(Change c) { + bool add_change(Change c) { assert(pos_is_okay()); - go_to_change_for(c.offset); - assert(pos + where->offset == c.offset); + if (not go_to_change_for(c.offset) || + pos + where->offset != c.offset) + return false; if (c.del_cnt > 0) - delete_lines(c.del_cnt); - assert(pos + where->offset == c.offset); + if (not delete_lines(c.del_cnt)) + return false; + if (pos + where->offset != c.offset) + return false; if (c.add_len > 0) { assert(pos_is_okay()); if (where->add_len > 0) - new_change(); - assert(where->add_len == 0 && where->add_cnt == 0); + if (not new_change()) + return false; + if (where->add_len != 0 || where->add_cnt != 0) + return false; where->add_len = c.add_len; where->add_cnt = c.add_cnt; where->add = c.add; } assert(pos_is_okay()); - merge(); - assert(pos_is_okay()); + if (not merge()) + return false; + return pos_is_okay(); } private: - void merge(void) + bool merge(void) { while (where->offset == 0 && where != changes.begin()) { - left(); + if (not left()) + return false; } std::list::iterator next = where; ++next; @@ -253,53 +263,56 @@ class FileChanges { ++next; } } + return true; } - void go_to_change_for(size_t line) + bool go_to_change_for(size_t line) { while(where != changes.end()) { if (line < pos) { - left(); + if (not left()) + return false; continue; } if (pos + where->offset + where->add_cnt <= line) { - right(); + if (not right()) + return false; continue; } // line is somewhere in this slot if (line < pos + where->offset) { break; } else if (line == pos + where->offset) { - return; + return true; } else { - split(line - pos); - right(); - return; + if (not split(line - pos)) + return false; + return right(); } } /* it goes before this patch */ - insert(line-pos); + return insert(line-pos); } - void new_change(void) { insert(where->offset); } + bool new_change(void) { return insert(where->offset); } - void insert(size_t offset) + bool insert(size_t offset) { assert(pos_is_okay()); - assert(where == changes.end() || offset <= where->offset); + if (where != changes.end() && offset > where->offset) + return false; if (where != changes.end()) where->offset -= offset; changes.insert(where, Change(offset)); --where; - assert(pos_is_okay()); + return pos_is_okay(); } - void split(size_t offset) + bool split(size_t offset) { assert(pos_is_okay()); - - assert(where->offset < offset); - assert(offset < where->offset + where->add_cnt); + if (where->offset >= offset || offset >= where->offset + where->add_cnt) + return false; size_t keep_lines = offset - where->offset; @@ -307,27 +320,29 @@ class FileChanges { where->del_cnt = 0; where->offset = 0; - where->skip_lines(keep_lines); + if (not where->skip_lines(keep_lines)) + return false; before.add_cnt = keep_lines; before.add_len -= where->add_len; changes.insert(where, before); --where; - assert(pos_is_okay()); + return pos_is_okay(); } - void delete_lines(size_t cnt) + bool delete_lines(size_t cnt) { - std::list::iterator x = where; assert(pos_is_okay()); + std::list::iterator x = where; while (cnt > 0) { size_t del; del = x->add_cnt; if (del > cnt) del = cnt; - x->skip_lines(del); + if (not x->skip_lines(del)) + return false; cnt -= del; ++x; @@ -342,21 +357,21 @@ class FileChanges { where->del_cnt += del; cnt -= del; } - assert(pos_is_okay()); + return pos_is_okay(); } - void left(void) { + bool left(void) { assert(pos_is_okay()); --where; pos -= where->offset + where->add_cnt; - assert(pos_is_okay()); + return pos_is_okay(); } - void right(void) { + bool right(void) { assert(pos_is_okay()); pos += where->offset + where->add_cnt; ++where; - assert(pos_is_okay()); + return pos_is_okay(); } }; @@ -378,7 +393,7 @@ class Patch { static void dump_rest(FileFd &o, FileFd &i, Hashes * const start_hash, Hashes * const end_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; unsigned long long l = 0; while (i.Read(buffer, sizeof(buffer), &l)) { if (l ==0 || !retry_fwrite(buffer, l, o, start_hash, end_hash)) @@ -389,7 +404,7 @@ class Patch { static void dump_lines(FileFd &o, FileFd &i, size_t n, Hashes * const start_hash, Hashes * const end_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; while (n > 0) { if (i.ReadLine(buffer, sizeof(buffer)) == NULL) buffer[0] = '\0'; @@ -402,7 +417,7 @@ class Patch { static void skip_lines(FileFd &i, int n, Hashes * const start_hash) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; while (n > 0) { if (i.ReadLine(buffer, sizeof(buffer)) == NULL) buffer[0] = '\0'; @@ -422,7 +437,7 @@ class Patch { bool read_diff(FileFd &f, Hashes * const h) { - char buffer[BLOCK_SIZE]; + char buffer[APT_MEMBLOCK_SIZE]; bool cmdwanted = true; Change ch(std::numeric_limits::max()); @@ -478,7 +493,8 @@ class Patch { ch.add = NULL; ch.add_cnt = 0; ch.add_len = 0; - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Delete command could not be added to changes", f.Name().c_str()); break; default: return _error->Error("Parsing patchfile %s failed: Unknown command", f.Name().c_str()); @@ -486,7 +502,8 @@ class Patch { } else { /* !cmdwanted */ if (strcmp(buffer, ".\n") == 0) { cmdwanted = true; - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (1)", f.Name().c_str()); } else { char *last = NULL; char *add; @@ -500,7 +517,8 @@ class Patch { ch.add_cnt++; } else { if (ch.add) { - filechanges.add_change(ch); + if (not filechanges.add_change(ch)) + return _error->Error("Parsing patchfile %s failed: Data couldn't be added for command (2)", f.Name().c_str()); ch.del_cnt = 0; } ch.offset += ch.add_cnt; @@ -575,6 +593,7 @@ class Patch { } }; +#ifndef APT_EXCLUDE_RRED_METHOD_CODE class RredMethod : public aptMethod { private: bool Debug; @@ -859,3 +878,4 @@ int main(int argc, const char *argv[]) input.Close(); return DispatchCommandLine(CmdL, {}); } +#endif -- cgit v1.2.3-70-g09d2 From 96dc40b19623621a9cc2c5541fb3adbbceb553b1 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 28 Jun 2020 20:52:09 +0200 Subject: Replace PrintStatus with SendMessage usage varg API is a nightmare as the symbols seems different on ever other arch, but more importantly SendMessage does a few checks on the content of the message and it is all outputted via C++ iostreams and not mixed in FILE* which is handy for overriding the streams. --- apt-pkg/acquire-method.cc | 44 ++++++++++++++++++++++++++++++++++++++------ apt-pkg/acquire-method.h | 2 +- apt-pkg/contrib/gpgv.cc | 15 --------------- apt-pkg/contrib/strutl.cc | 11 +++++------ apt-pkg/contrib/strutl.h | 2 ++ methods/aptmethod.h | 30 ++++++++++++++++++------------ methods/basehttp.cc | 4 +++- methods/gpgv.cc | 8 ++++++-- 8 files changed, 73 insertions(+), 43 deletions(-) (limited to 'methods') diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc index 6e1674f7f..089582561 100644 --- a/apt-pkg/acquire-method.cc +++ b/apt-pkg/acquire-method.cc @@ -482,9 +482,25 @@ void pkgAcqMethod::PrintStatus(char const * const header, const char* Format, void pkgAcqMethod::Log(const char *Format,...) { va_list args; - va_start(args,Format); - PrintStatus("101 Log", Format, args); - va_end(args); + ssize_t size = 400; + std::ostringstream outstr; + while (true) { + bool ret; + va_start(args,Format); + ret = iovprintf(outstr, Format, args, size); + va_end(args); + if (ret == true) + break; + } + std::unordered_map fields; + if (Queue != 0) + try_emplace(fields, "URI", Queue->Uri); + else + try_emplace(fields, "URI", ""); + if (not UsedMirror.empty()) + try_emplace(fields, "UsedMirror", UsedMirror); + try_emplace(fields, "Message", outstr.str()); + SendMessage("101 Log", std::move(fields)); } /*}}}*/ // AcqMethod::Status - Send a status message /*{{{*/ @@ -493,9 +509,25 @@ void pkgAcqMethod::Log(const char *Format,...) void pkgAcqMethod::Status(const char *Format,...) { va_list args; - va_start(args,Format); - PrintStatus("102 Status", Format, args); - va_end(args); + ssize_t size = 400; + std::ostringstream outstr; + while (true) { + bool ret; + va_start(args,Format); + ret = iovprintf(outstr, Format, args, size); + va_end(args); + if (ret == true) + break; + } + std::unordered_map fields; + if (Queue != 0) + try_emplace(fields, "URI", Queue->Uri); + else + try_emplace(fields, "URI", ""); + if (not UsedMirror.empty()) + try_emplace(fields, "UsedMirror", UsedMirror); + try_emplace(fields, "Message", outstr.str()); + SendMessage("102 Status", std::move(fields)); } /*}}}*/ // AcqMethod::Redirect - Send a redirect message /*{{{*/ diff --git a/apt-pkg/acquire-method.h b/apt-pkg/acquire-method.h index e854f5ff1..b4b238c4c 100644 --- a/apt-pkg/acquire-method.h +++ b/apt-pkg/acquire-method.h @@ -101,7 +101,7 @@ class APT_PUBLIC pkgAcqMethod bool MediaFail(std::string Required,std::string Drive); virtual void Exit() {}; - void PrintStatus(char const * const header, const char* Format, va_list &args) const; + APT_DEPRECATED_MSG("Use SendMessage instead") void PrintStatus(char const * const header, const char* Format, va_list &args) const; public: enum CnfFlags diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 28f3150c3..3368ece84 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -121,21 +121,6 @@ static bool operator!=(LineBuffer const &buf, APT::StringView const exp) noexcep And as a cherry on the cake, we use our apt-key wrapper to do part of the lifting in regards to merging keyrings. Fun for the whole family. */ -static bool iovprintf(std::ostream &out, const char *format, - va_list &args, ssize_t &size) { - auto S = make_unique_char(malloc(size)); - ssize_t const n = vsnprintf(S.get(), size, format, args); - if (n > -1 && n < size) { - out << S.get(); - return true; - } else { - if (n > -1) - size = n + 1; - else - size *= 2; - } - return false; -} static void APT_PRINTF(4) apt_error(std::ostream &outterm, int const statusfd, int fd[2], const char *format, ...) { std::ostringstream outstr; diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 84e79122e..3a0a6eaa3 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1414,13 +1415,12 @@ unsigned long RegexChoice(RxChoiceList *Rxs,const char **ListBegin, // --------------------------------------------------------------------- /* This is used to make the internationalization strings easier to translate and to allow reordering of parameters */ -static bool iovprintf(ostream &out, const char *format, +bool iovprintf(std::ostream &out, const char *format, va_list &args, ssize_t &size) { - char *S = (char*)malloc(size); - ssize_t const n = vsnprintf(S, size, format, args); + auto S = std::unique_ptr{static_cast(malloc(size)), &free}; + ssize_t const n = vsnprintf(S.get(), size, format, args); if (n > -1 && n < size) { - out << S; - free(S); + out << S.get(); return true; } else { if (n > -1) @@ -1428,7 +1428,6 @@ static bool iovprintf(ostream &out, const char *format, else size *= 2; } - free(S); return false; } void ioprintf(ostream &out,const char *format,...) diff --git a/apt-pkg/contrib/strutl.h b/apt-pkg/contrib/strutl.h index 8669c97a5..b6e6bfdce 100644 --- a/apt-pkg/contrib/strutl.h +++ b/apt-pkg/contrib/strutl.h @@ -117,6 +117,8 @@ APT_PUBLIC std::vector StringSplit(std::string const &input, std::string const &sep, unsigned int maxsplit=std::numeric_limits::max()) APT_PURE; + +APT_HIDDEN bool iovprintf(std::ostream &out, const char *format, va_list &args, ssize_t &size); APT_PUBLIC void ioprintf(std::ostream &out,const char *format,...) APT_PRINTF(2); APT_PUBLIC void strprintf(std::string &out,const char *format,...) APT_PRINTF(2); APT_PUBLIC char *safe_snprintf(char *Buffer,char *End,const char *Format,...) APT_PRINTF(3); diff --git a/methods/aptmethod.h b/methods/aptmethod.h index 7038131cf..bd50e8078 100644 --- a/methods/aptmethod.h +++ b/methods/aptmethod.h @@ -325,7 +325,11 @@ protected: rc = seccomp_load(ctx); if (rc == -EINVAL) - Warning("aptMethod::Configuration: could not load seccomp policy: %s", strerror(-rc)); + { + std::string msg; + strprintf(msg, "aptMethod::Configuration: could not load seccomp policy: %s", strerror(-rc)); + Warning(std::move(msg)); + } else if (rc != 0) return _error->FatalE("aptMethod::Configuration", "could not load seccomp policy: %s", strerror(-rc)); @@ -380,12 +384,17 @@ protected: return true; } - void Warning(const char *Format,...) + void Warning(std::string &&msg) { - va_list args; - va_start(args,Format); - PrintStatus("104 Warning", Format, args); - va_end(args); + std::unordered_map fields; + if (Queue != 0) + fields.emplace("URI", Queue->Uri); + else + fields.emplace("URI", ""); + if (not UsedMirror.empty()) + fields.emplace("UsedMirror", UsedMirror); + fields.emplace("Message", std::move(msg)); + SendMessage("104 Warning", std::move(fields)); } std::vector methodNames; @@ -560,14 +569,11 @@ class aptAuthConfMethod : public aptMethod result &= MaybeAddAuth(*authconf, uri); } - if (not _error->empty()) + while (not _error->empty()) { std::string message; - while (not _error->empty()) - { - _error->PopMessage(message); - Warning("%s", message.c_str()); - } + _error->PopMessage(message); + Warning(std::move(message)); } _error->RevertToStack(); diff --git a/methods/basehttp.cc b/methods/basehttp.cc index 8aac1090c..b75b450cc 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -762,7 +762,9 @@ int BaseHttpMethod::Loop() // yes, he did! Disable pipelining and rewrite queue if (Server->Pipeline == true) { - Warning(_("Automatically disabled %s due to incorrect response from server/proxy. (man 5 apt.conf)"), "Acquire::http::Pipeline-Depth"); + std::string msg; + strprintf(msg, _("Automatically disabled %s due to incorrect response from server/proxy. (man 5 apt.conf)"), "Acquire::http::Pipeline-Depth"); + Warning(std::move(msg)); Server->Pipeline = false; Server->PipelineAllowed = false; // we keep the PipelineDepth value so that the rest of the queue can be fixed up as well diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 08d030a17..a9da456ec 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -264,7 +264,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, SubKeyMapping[tokens[9]].emplace_back(sig); } else if (strncmp(buffer, APTKEYWARNING, sizeof(APTKEYWARNING)-1) == 0) - Warning("%s", buffer + sizeof(APTKEYWARNING)); + Warning(buffer + sizeof(APTKEYWARNING)); else if (strncmp(buffer, APTKEYERROR, sizeof(APTKEYERROR)-1) == 0) _error->Error("%s", buffer + sizeof(APTKEYERROR)); } @@ -442,8 +442,12 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm) })) { for (auto const & Signer : Signers.SoonWorthless) + { + std::string msg; // TRANSLATORS: The second %s is the reason and is untranslated for repository owners. - Warning(_("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str()); + strprintf(msg, _("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str()); + Warning(std::move(msg)); + } } if (Signers.Good.empty() || !Signers.Bad.empty() || !Signers.NoPubKey.empty()) -- cgit v1.2.3-70-g09d2 From 3e53dbbe758a4e2da378ebf0296d8105d4a5804c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 28 Jun 2020 20:59:27 +0200 Subject: Ensure HTTP status code text has sensible content We use the code in error messages, so at least for that edgecase we should ensure that it has sensible content. Note that the acquire system aborts on non-sensible message content in SendMessage, so you can't really exploit this. --- methods/basehttp.cc | 3 +++ methods/basehttp.h | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/basehttp.cc b/methods/basehttp.cc index b75b450cc..3786e2e6c 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -110,6 +110,9 @@ bool RequestState::HeaderLine(string const &Line) /*{{{*/ if (sscanf(Line.c_str(),"HTTP %3u%359[^\n]",&Result,Code) != 2) return _error->Error(_("The HTTP server sent an invalid reply header")); } + auto const CodeLen = strlen(Code); + auto const CodeEnd = std::remove_if(Code, Code + CodeLen, [](char c) { return isprint(c) == 0; }); + *CodeEnd = '\0'; /* Check the HTTP response header to get the default persistence state. */ diff --git a/methods/basehttp.h b/methods/basehttp.h index 62c9963ea..c0d14d854 100644 --- a/methods/basehttp.h +++ b/methods/basehttp.h @@ -60,7 +60,7 @@ struct RequestState bool AddPartialFileToHashes(FileFd &File); RequestState(BaseHttpMethod * const Owner, ServerState * const Server) : - Owner(Owner), Server(Server) { time(&Date); } + Owner(Owner), Server(Server) { time(&Date); Code[0] = '\0'; } }; struct ServerState { -- cgit v1.2.3-70-g09d2