From 1b3ad3891465f8b72bcedfb4edd513cb74eec7f3 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:35:29 +0100 Subject: Add a simple test for APT::String::DisplayLength References: 2497198e9599a6a8d4d0ad08627bcfc7ea49c644 Gbp-Dch: Ignore --- test/libapt/strutil_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'test/libapt') diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index b7132c35f..349946b5f 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -361,3 +361,20 @@ TEST(StrUtilTest, LookupTag) EXPECT_EQ("Value4", LookupTag(msg, "Field4", "")); EXPECT_EQ("Value5", LookupTag(msg, "Field5", "")); } + +TEST(StrUtilTest, DisplayLength) +{ + EXPECT_EQ(0, APT::String::DisplayLength("")); + EXPECT_EQ(1, APT::String::DisplayLength("a")); + EXPECT_EQ(3, APT::String::DisplayLength("apt")); + EXPECT_EQ(1, APT::String::DisplayLength("@")); + EXPECT_EQ(3, APT::String::DisplayLength("き")); + + EXPECT_EQ(1, APT::String::DisplayLength("$")); + EXPECT_EQ(2, APT::String::DisplayLength("¢")); + EXPECT_EQ(3, APT::String::DisplayLength("ह")); + EXPECT_EQ(3, APT::String::DisplayLength("€")); + EXPECT_EQ(3, APT::String::DisplayLength("한")); + EXPECT_EQ(4, APT::String::DisplayLength("𐍈")); + EXPECT_EQ(16, APT::String::DisplayLength("𐍈한€ह¢$")); +} -- cgit v1.2.3-70-g09d2 From 10f13938bbf1474451fadcd62e1c31c4b5f5b3d7 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:41:43 +0100 Subject: Fix incorrect base64 encoding due to int promotion For \xff and friends with the highest bit set and hence being a negative value on signed char systems the wrong encoding is produced as we run into undefined behaviour accessing negative array indexes. We can avoid this problem simply by using an unsigned data type. --- apt-pkg/contrib/strutl.cc | 2 +- test/libapt/strutil_test.cc | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'test/libapt') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index bd4856526..826b21478 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -597,7 +597,7 @@ string Base64Encode(const string &S) base64. */ for (string::const_iterator I = S.begin(); I < S.end(); I += 3) { - char Bits[3] = {0,0,0}; + uint8_t Bits[3] = {0,0,0}; Bits[0] = I[0]; if (I + 1 < S.end()) Bits[1] = I[1]; diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index 349946b5f..f101d72cf 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -167,6 +167,10 @@ TEST(StrUtilTest,Base64Encode) EXPECT_EQ("ZS4=", Base64Encode("e.")); EXPECT_EQ("Lg==", Base64Encode(".")); EXPECT_EQ("", Base64Encode("")); + EXPECT_EQ("IA==", Base64Encode("\x20")); + EXPECT_EQ("/w==", Base64Encode("\xff")); + EXPECT_EQ("/A==", Base64Encode("\xfc")); + EXPECT_EQ("//8=", Base64Encode("\xff\xff")); } static void ReadMessagesTestWithNewLine(char const * const nl, char const * const ab) { -- cgit v1.2.3-70-g09d2 From ed192f410da36aedf5e54bb3f967e6613ab4bb51 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 3 Dec 2020 10:44:27 +0100 Subject: Don't parse \x and \0 past the end in DeEscapeString MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This has no attack surface though as the loop is to end very soon anyhow and the method only used while reading CD-ROM mountpoints which seems like a very unlikely attack vector… --- apt-pkg/contrib/strutl.cc | 12 ++++++++---- test/libapt/strutil_test.cc | 6 ++++++ 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'test/libapt') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 826b21478..45e475b3e 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -1611,22 +1611,26 @@ string DeEscapeString(const string &input) switch (*it) { case '0': - if (it + 2 <= input.end()) { + if (it + 2 < input.end()) { tmp[0] = it[1]; tmp[1] = it[2]; tmp[2] = 0; output += (char)strtol(tmp, 0, 8); it += 2; - } + } else { + // FIXME: raise exception here? + } break; case 'x': - if (it + 2 <= input.end()) { + if (it + 2 < input.end()) { tmp[0] = it[1]; tmp[1] = it[2]; tmp[2] = 0; output += (char)strtol(tmp, 0, 16); it += 2; - } + } else { + // FIXME: raise exception here? + } break; default: // FIXME: raise exception here? diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index f101d72cf..d477e953c 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -21,6 +21,12 @@ TEST(StrUtilTest,DeEscapeString) // double slashes EXPECT_EQ("foo\\ x", DeEscapeString("foo\\\\ x")); EXPECT_EQ("\\foo\\", DeEscapeString("\\\\foo\\\\")); + + // FIXME: the input is bad, the output as well, but we have no indicator for it + EXPECT_EQ("aa", DeEscapeString("aa\\x")); + EXPECT_EQ("aa0", DeEscapeString("aa\\x0")); + EXPECT_EQ("aa", DeEscapeString("aa\\0")); + EXPECT_EQ("aaa", DeEscapeString("aa\\0a")); } TEST(StrUtilTest,StringStrip) { -- cgit v1.2.3-70-g09d2 From e0743a85c5f5f2f83d91c305450e8ba192194cd8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 13 May 2020 09:07:19 +0200 Subject: Forbid negative values in unsigned StrToNum explicitly strtoul(l) surprises us with parsing negative values which should not exist in the places we use to parse them, so we can just downright refuse them rather than trying to work with them by having them promoted to huge positive values. --- apt-pkg/contrib/strutl.cc | 41 +++++++++++++-------------------- test/libapt/strutil_test.cc | 56 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 25 deletions(-) (limited to 'test/libapt') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index 7beb5f621..84e79122e 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -1139,34 +1140,24 @@ bool FTPMDTMStrToTime(const char* const str,time_t &time) /*}}}*/ // StrToNum - Convert a fixed length string to a number /*{{{*/ // --------------------------------------------------------------------- -/* This is used in decoding the crazy fixed length string headers in +/* This is used in decoding the crazy fixed length string headers in tar and ar files. */ bool StrToNum(const char *Str,unsigned long &Res,unsigned Len,unsigned Base) { - char S[30]; - if (Len >= sizeof(S)) + unsigned long long BigRes; + if (not StrToNum(Str, BigRes, Len, Base)) return false; - memcpy(S,Str,Len); - S[Len] = 0; - - // All spaces is a zero - Res = 0; - unsigned I; - for (I = 0; S[I] == ' '; I++); - if (S[I] == 0) - return true; - - char *End; - Res = strtoul(S,&End,Base); - if (End == S) + + if (std::numeric_limits::max() < BigRes) return false; - + + Res = BigRes; return true; } /*}}}*/ // StrToNum - Convert a fixed length string to a number /*{{{*/ // --------------------------------------------------------------------- -/* This is used in decoding the crazy fixed length string headers in +/* This is used in decoding the crazy fixed length string headers in tar and ar files. */ bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base) { @@ -1175,20 +1166,20 @@ bool StrToNum(const char *Str,unsigned long long &Res,unsigned Len,unsigned Base return false; memcpy(S,Str,Len); S[Len] = 0; - + // All spaces is a zero Res = 0; unsigned I; - for (I = 0; S[I] == ' '; I++); + for (I = 0; S[I] == ' '; ++I); if (S[I] == 0) return true; - + if (S[I] == '-') + return false; + char *End; + errno = 0; Res = strtoull(S,&End,Base); - if (End == S) - return false; - - return true; + return not (End == S || errno != 0); } /*}}}*/ diff --git a/test/libapt/strutil_test.cc b/test/libapt/strutil_test.cc index d477e953c..469de4403 100644 --- a/test/libapt/strutil_test.cc +++ b/test/libapt/strutil_test.cc @@ -1,6 +1,8 @@ #include #include +#include #include +#include #include #include @@ -257,6 +259,60 @@ TEST(StrUtilTest,QuoteString) EXPECT_EQ("Eltville-Erbach", DeQuoteString(QuoteString("Eltville-Erbach", ""))); } +static void EXPECT_STRTONUM(APT::StringView const str, bool const success, unsigned long const expected, unsigned const base) +{ + SCOPED_TRACE(std::string(str.data(), str.length())); + SCOPED_TRACE(base); + unsigned long N1 = 1000; + unsigned long long N2 = 1000; + if (not success) + { + EXPECT_FALSE(StrToNum(str.data(), N1, str.length(), base)); + EXPECT_FALSE(StrToNum(str.data(), N2, str.length(), base)); + return; + } + EXPECT_TRUE(StrToNum(str.data(), N1, str.length(), base)); + EXPECT_EQ(expected, N1); + + EXPECT_TRUE(StrToNum(str.data(), N2, str.length(), base)); + EXPECT_EQ(expected, N2); +} +TEST(StrUtilTest,StrToNum) +{ + EXPECT_STRTONUM("", true, 0, 10); + EXPECT_STRTONUM(" ", true, 0, 10); + EXPECT_STRTONUM("0", true, 0, 10); + EXPECT_STRTONUM("1", true, 1, 10); + EXPECT_STRTONUM(" 1 ", true, 1, 10); + EXPECT_STRTONUM("1", true, 1, 8); + EXPECT_STRTONUM("10", true, 10, 10); + EXPECT_STRTONUM("10", true, 8, 8); + EXPECT_STRTONUM("010", true, 8, 8); + EXPECT_STRTONUM(" 010 ", true, 8, 8); + EXPECT_STRTONUM("-1", false, 0, 10); + EXPECT_STRTONUM(" -1 ", false, 0, 10); + EXPECT_STRTONUM("11", true, 3, 2); + + unsigned long long bigN = 0; + unsigned long smallN = 0; + auto bigLimit = std::to_string(std::numeric_limits::max()); + if (std::numeric_limits::max() < std::numeric_limits::max()) + { + EXPECT_TRUE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), bigN); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10)); + } + bigLimit.append("0"); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), bigN, bigLimit.length(), 10)); + EXPECT_FALSE(StrToNum(bigLimit.c_str(), smallN, bigLimit.length(), 10)); + + auto const smallLimit = std::to_string(std::numeric_limits::max()); + EXPECT_TRUE(StrToNum(smallLimit.c_str(), bigN, smallLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), bigN); + EXPECT_TRUE(StrToNum(smallLimit.c_str(), smallN, smallLimit.length(), 10)); + EXPECT_EQ(std::numeric_limits::max(), smallN); +} + TEST(StrUtilTest,RFC1123StrToTime) { { -- cgit v1.2.3-70-g09d2 From c4da2ff42da55ffc38c77a9170dc151216d75962 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 3 Feb 2021 17:40:05 +0100 Subject: Avoid overstepping bounds in config file parsing Our configuration files are not security relevant, but having a parser which avoids crashing on them even if they are seriously messed up is not a bad idea anyway. It is also a good opportunity to brush up the code a bit avoiding a few small string copies with our string_view. --- apt-pkg/contrib/configuration.cc | 114 +++++++++++++++++++-------------------- apt-pkg/contrib/string_view.h | 26 +++++++++ test/libapt/stringview_test.cc | 8 +++ 3 files changed, 88 insertions(+), 60 deletions(-) (limited to 'test/libapt') diff --git a/apt-pkg/contrib/configuration.cc b/apt-pkg/contrib/configuration.cc index 554307324..a8fced724 100644 --- a/apt-pkg/contrib/configuration.cc +++ b/apt-pkg/contrib/configuration.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -867,83 +868,76 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio // Now strip comments; if the whole line is contained in a // comment, skip this line. + APT::StringView Line{Input.data(), Input.size()}; - // The first meaningful character in the current fragment; will - // be adjusted below as we remove bytes from the front. - std::string::const_iterator Start = Input.begin(); - // The last meaningful character in the current fragment. - std::string::const_iterator End = Input.end(); - - // Multi line comment - if (InComment == true) + // continued Multi line comment + if (InComment) { - for (std::string::const_iterator I = Start; - I != End; ++I) + size_t end = Line.find("*/"); + if (end != APT::StringView::npos) { - if (*I == '*' && I + 1 != End && I[1] == '/') - { - Start = I + 2; - InComment = false; - break; - } + Line.remove_prefix(end + 2); + InComment = false; } - if (InComment == true) + else continue; } // Discard single line comments - bool InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - continue; - - if ((*I == '/' && I + 1 != End && I[1] == '/') || - (*I == '#' && strcmp(string(I,I+6).c_str(),"#clear") != 0 && - strcmp(string(I,I+8).c_str(),"#include") != 0 && - strcmp(string(I,I+strlen("#x-apt-configure-index")).c_str(), "#x-apt-configure-index") != 0)) + size_t start = 0; + while ((start = Line.find("//", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); + break; + } + using APT::operator""_sv; + constexpr std::array magicComments { "clear"_sv, "include"_sv, "x-apt-configure-index"_sv }; + start = 0; + while ((start = Line.find('#', start)) != APT::StringView::npos) { - End = I; + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0 || + std::any_of(magicComments.begin(), magicComments.end(), [&](auto const m) { return Line.compare(start+1, m.length(), m) == 0; })) + { + ++start; + continue; + } + Line.remove_suffix(Line.length() - start); break; } } // Look for multi line comments and build up the // fragment. - Fragment.reserve(End - Start); - InQuote = false; - for (std::string::const_iterator I = Start; - I != End; ++I) + Fragment.reserve(Line.length()); { - if (*I == '"') - InQuote = !InQuote; - if (InQuote == true) - Fragment.push_back(*I); - else if (*I == '/' && I + 1 != End && I[1] == '*') - { - InComment = true; - for (std::string::const_iterator J = I; - J != End; ++J) + size_t start = 0; + while ((start = Line.find("/*", start)) != APT::StringView::npos) + { + if (std::count(Line.begin(), Line.begin() + start, '"') % 2 != 0) { - if (*J == '*' && J + 1 != End && J[1] == '/') - { - // Pretend we just finished walking over the - // comment, and don't add anything to the output - // fragment. - I = J + 1; - InComment = false; - break; - } + start += 2; + continue; } - - if (InComment == true) - break; + Fragment.append(Line.data(), start); + auto const end = Line.find("*/", start + 2); + if (end == APT::StringView::npos) + { + Line.clear(); + InComment = true; + break; + } + else + Line.remove_prefix(end + 2); + start = 0; } - else - Fragment.push_back(*I); + if (not Line.empty()) + Fragment.append(Line.data(), Line.length()); } // Skip blank lines. @@ -951,9 +945,9 @@ bool ReadConfigFile(Configuration &Conf,const string &FName,bool const &AsSectio continue; // The line has actual content; interpret what it means. - InQuote = false; - Start = Fragment.begin(); - End = Fragment.end(); + bool InQuote = false; + auto Start = Fragment.cbegin(); + auto End = Fragment.cend(); for (std::string::const_iterator I = Start; I != End; ++I) { diff --git a/apt-pkg/contrib/string_view.h b/apt-pkg/contrib/string_view.h index 05aad3327..04f6ff115 100644 --- a/apt-pkg/contrib/string_view.h +++ b/apt-pkg/contrib/string_view.h @@ -39,6 +39,10 @@ public: StringView(const char *data) : data_(data), size_(strlen(data)) {} StringView(std::string const & str): data_(str.data()), size_(str.size()) {} + /* Modifiers */ + void remove_prefix(size_t n) { data_ += n; size_ -= n; } + void remove_suffix(size_t n) { size_ -= n; } + void clear() { size_ = 0; } /* Viewers */ constexpr StringView substr(size_t pos, size_t n = npos) const { @@ -76,6 +80,28 @@ public: return found - data_; } + size_t find(APT::StringView const needle) const { + if (needle.empty()) + return npos; + if (needle.length() == 1) + return find(*needle.data()); + size_t found = 0; + while ((found = find(*needle.data(), found)) != npos) { + if (compare(found, needle.length(), needle) == 0) + return found; + ++found; + } + return found; + } + size_t find(APT::StringView const needle, size_t pos) const { + if (pos == 0) + return find(needle); + size_t const found = substr(pos).find(needle); + if (found == npos) + return npos; + return pos + found; + } + /* Conversions */ std::string to_string() const { return std::string(data_, size_); diff --git a/test/libapt/stringview_test.cc b/test/libapt/stringview_test.cc index 03d82517b..9cfaa3b48 100644 --- a/test/libapt/stringview_test.cc +++ b/test/libapt/stringview_test.cc @@ -74,6 +74,14 @@ TEST(StringViewTest,Find) EXPECT_EQ(defString.to_string().find('e',3), defString.find('e',3)); EXPECT_EQ(defString.to_string().find('l',6), defString.find('l',6)); EXPECT_EQ(defString.to_string().find('l',11), defString.find('l',11)); + + EXPECT_EQ(defString.to_string().find("l"), defString.find("l")); + EXPECT_EQ(defString.to_string().find("ll"), defString.find("ll")); + EXPECT_EQ(defString.to_string().find("lo"), defString.find("lo")); + EXPECT_EQ(defString.to_string().find("ll", 1), defString.find("ll", 1)); + EXPECT_EQ(defString.to_string().find("ll", 6), defString.find("ll", 6)); + EXPECT_EQ(defString.to_string().find("or"), defString.find("or")); + EXPECT_EQ(defString.to_string().find("od"), defString.find("od")); } TEST(StringViewTest,RFind) -- cgit v1.2.3-70-g09d2