From 5f4816f1d88fe8f835a36eb648661e80960a3ac8 Mon Sep 17 00:00:00 2001 From: arvidn Date: Thu, 10 Aug 2017 19:47:42 +0200 Subject: [PATCH 1/5] fix include in http_connection.cpp --- src/http_connection.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/http_connection.cpp b/src/http_connection.cpp index 5f18b452d..3e30f1631 100644 --- a/src/http_connection.cpp +++ b/src/http_connection.cpp @@ -51,6 +51,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include "libtorrent/aux_/disable_warnings_pop.hpp" From 50c2aee8ec68d0673c06547cf2706be09b970c7f Mon Sep 17 00:00:00 2001 From: Steven Siloti Date: Thu, 10 Aug 2017 15:42:56 -0700 Subject: [PATCH 2/5] don't create web seed connections if the torrent is upload only The fix in 9a63d4696ebec5743418181ef3411371d8e31b04 was imcomplete. It turns out disconnect_if_redundant decrements the connection count which unballances the count if we do end up disconnecting the peer. This change avoids the problem by checking if the torrent is upload only much sooner, before the connection object is even created. We still do a redundancy check just-in-case, but it is not expected to trigger. --- src/torrent.cpp | 1 + src/web_connection_base.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/torrent.cpp b/src/torrent.cpp index 2d97b0b20..3aacf237a 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -6605,6 +6605,7 @@ namespace { if (is_paused()) return; if (m_ses.is_aborted()) return; + if (is_upload_only()) return; boost::shared_ptr s = boost::make_shared(boost::ref(m_ses.get_io_service())); diff --git a/src/web_connection_base.cpp b/src/web_connection_base.cpp index 9ad70b513..63e18744b 100644 --- a/src/web_connection_base.cpp +++ b/src/web_connection_base.cpp @@ -77,6 +77,8 @@ namespace libtorrent TORRENT_ASSERT(is_outgoing()); + TORRENT_ASSERT(!m_torrent.lock()->is_upload_only()); + // we only want left-over bandwidth // TODO: introduce a web-seed default class which has a low download priority @@ -117,9 +119,10 @@ namespace libtorrent // which fails because the m_num_connecting count is not consistent until // after we call peer_connection::start m_upload_only = true; - disconnect_if_redundant(); - if (is_disconnecting()) return; peer_connection::start(); + // disconnect_if_redundant must be called after start to keep + // m_num_connecting consistent + disconnect_if_redundant(); } web_connection_base::~web_connection_base() From fcb9c7b6f37acb18627779f4367cef85e27c159b Mon Sep 17 00:00:00 2001 From: arvidn Date: Mon, 14 Aug 2017 12:14:41 +0200 Subject: [PATCH 3/5] fix invalid read in parse_int() in bdecode_node() and lazy_bdecode() --- ChangeLog | 1 + src/bdecode.cpp | 20 ++++++++++++-------- src/lazy_bdecode.cpp | 4 ++++ test/test_bdecode.cpp | 24 ++++++++++++++++++++---- test/test_bencoding.cpp | 7 ++++--- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/ChangeLog b/ChangeLog index c540d4a80..d5d7d8f20 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * fix invalid read in parse_int in bdecoder * fix issue with very long tracker- and web seed URLs * don't attempt to create empty files on startup, if they already exist * fix force-recheck issue (new files would not be picked up) diff --git a/src/bdecode.cpp b/src/bdecode.cpp index d428f3e4f..072697ee9 100644 --- a/src/bdecode.cpp +++ b/src/bdecode.cpp @@ -126,10 +126,12 @@ namespace libtorrent } // anonymous namespace - // fills in 'val' with what the string between start and the - // first occurance of the delimiter is interpreted as an int. - // return the pointer to the delimiter, or 0 if there is a - // parse error. val should be initialized to zero + // reads the string between start and end, or up to the first occurrance of + // 'delimiter', whichever comes first. This string is interpreted as an + // integer which is assigned to 'val'. If there's a non-delimiter and + // non-digit in the range, a parse error is reported in 'ec'. If the value + // cannot be represented by the variable 'val' and overflow error is reported + // by 'ec'. char const* parse_int(char const* start, char const* end, char delimiter , boost::int64_t& val, bdecode_errors::error_code_enum& ec) { @@ -155,8 +157,6 @@ namespace libtorrent val += digit; ++start; } - if (*start != delimiter) - ec = bdecode_errors::expected_colon; return start; } @@ -618,16 +618,18 @@ namespace libtorrent bdecode_token const& t = m_root_tokens[m_token_idx]; int size = m_root_tokens[m_token_idx + 1].offset - t.offset; TORRENT_ASSERT(t.type == bdecode_token::integer); - + // +1 is to skip the 'i' char const* ptr = m_buffer + t.offset + 1; boost::int64_t val = 0; bool negative = false; if (*ptr == '-') negative = true; bdecode_errors::error_code_enum ec = bdecode_errors::no_error; - parse_int(ptr + negative + char const* end = parse_int(ptr + negative , ptr + size, 'e', val, ec); if (ec) return 0; + TORRENT_UNUSED(end); + TORRENT_ASSERT(end < ptr + size); if (negative) val = -val; return val; } @@ -844,6 +846,8 @@ namespace libtorrent start = parse_int(start, end, ':', len, e); if (e) TORRENT_FAIL_BDECODE(e); + if (start == end) + TORRENT_FAIL_BDECODE(bdecode_errors::expected_colon); // remaining buffer size excluding ':' const ptrdiff_t buff_size = end - start - 1; diff --git a/src/lazy_bdecode.cpp b/src/lazy_bdecode.cpp index fc3461bec..9438cc2e2 100644 --- a/src/lazy_bdecode.cpp +++ b/src/lazy_bdecode.cpp @@ -131,6 +131,8 @@ namespace libtorrent start = parse_int(start, end, ':', len, e); if (e) TORRENT_FAIL_BDECODE(e); + if (start == end) + TORRENT_FAIL_BDECODE(bdecode_errors::expected_colon); // remaining buffer size excluding ':' const ptrdiff_t buff_size = end - start - 1; @@ -203,6 +205,8 @@ namespace libtorrent start = parse_int(start, end, ':', len, e); if (e) TORRENT_FAIL_BDECODE(e); + if (start == end) + TORRENT_FAIL_BDECODE(bdecode_errors::expected_colon); // remaining buffer size excluding ':' const ptrdiff_t buff_size = end - start - 1; diff --git a/test/test_bdecode.cpp b/test/test_bdecode.cpp index cf4ef0857..c33c7d946 100644 --- a/test/test_bdecode.cpp +++ b/test/test_bdecode.cpp @@ -715,8 +715,9 @@ TORRENT_TEST(parse_int) { char b[] = "1234567890e"; boost::int64_t val = 0; - bdecode_errors::error_code_enum ec; + bdecode_errors::error_code_enum ec = bdecode_errors::no_error; char const* e = parse_int(b, b + sizeof(b)-1, 'e', val, ec); + TEST_EQUAL(ec, bdecode_errors::no_error); TEST_EQUAL(val, 1234567890); TEST_EQUAL(e, b + sizeof(b) - 2); } @@ -759,7 +760,7 @@ TORRENT_TEST(parse_length_overflow) bdecode_node e; int ret = bdecode(b[i], b[i] + strlen(b[i]), e, ec); TEST_EQUAL(ret, -1); - TEST_CHECK(ec == error_code(bdecode_errors::unexpected_eof)); + TEST_EQUAL(ec, error_code(bdecode_errors::unexpected_eof)); } } @@ -768,9 +769,9 @@ TORRENT_TEST(expected_colon_string) { char b[] = "928"; boost::int64_t val = 0; - bdecode_errors::error_code_enum ec; + bdecode_errors::error_code_enum ec = bdecode_errors::no_error; char const* e = parse_int(b, b + sizeof(b)-1, ':', val, ec); - TEST_EQUAL(ec, bdecode_errors::expected_colon); + TEST_EQUAL(ec, bdecode_errors::no_error); TEST_EQUAL(e, b + 3); } @@ -1253,6 +1254,21 @@ TORRENT_TEST(partial_parse4) TEST_EQUAL(print_entry(e), "{ 'a': 1, 'b': 'foo', 'c': [ 1 ] }"); } +TORRENT_TEST(partial_parse_string) +{ + // it's important to not have a null terminator here + // to allow address sanitizer to trigger in case the decoder reads past the + // end + char b[] = { '5', '5'}; + + bdecode_node e; + error_code ec; + int pos; + int ret = bdecode(b, b + sizeof(b), e, ec, &pos); + TEST_EQUAL(ret, -1); + TEST_EQUAL(pos, 2); +} + // test switch_underlying_buffer TORRENT_TEST(switch_buffer) { diff --git a/test/test_bencoding.cpp b/test/test_bencoding.cpp index 25661d486..f94f953d0 100644 --- a/test/test_bencoding.cpp +++ b/test/test_bencoding.cpp @@ -587,8 +587,9 @@ TORRENT_TEST(lazy_entry) { char b[] = "1234567890e"; boost::int64_t val = 0; - bdecode_errors::error_code_enum ec; + bdecode_errors::error_code_enum ec = bdecode_errors::no_error; char const* e = parse_int(b, b + sizeof(b)-1, 'e', val, ec); + TEST_CHECK(ec == bdecode_errors::no_error); TEST_EQUAL(val, 1234567890); TEST_EQUAL(e, b + sizeof(b) - 2); } @@ -615,9 +616,9 @@ TORRENT_TEST(lazy_entry) { char b[] = "928"; boost::int64_t val = 0; - bdecode_errors::error_code_enum ec; + bdecode_errors::error_code_enum ec = bdecode_errors::no_error; char const* e = parse_int(b, b + sizeof(b)-1, ':', val, ec); - TEST_CHECK(ec == bdecode_errors::expected_colon); + TEST_CHECK(ec == bdecode_errors::no_error); TEST_EQUAL(e, b + 3); } From b70d3efba9e6b534772df73963e74ff796dca5ac Mon Sep 17 00:00:00 2001 From: arvidn Date: Tue, 15 Aug 2017 11:59:13 +0200 Subject: [PATCH 4/5] fix infinite loop when parsing torrents whose filenames have zeroes. #2247 --- AUTHORS | 2 + ChangeLog | 1 + include/libtorrent/string_util.hpp | 12 ++++++ src/string_util.cpp | 43 +++++++++++++++++++ src/torrent_info.cpp | 59 -------------------------- test/test_string.cpp | 67 ++++++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 59 deletions(-) diff --git a/AUTHORS b/AUTHORS index a02387442..7c2005c15 100644 --- a/AUTHORS +++ b/AUTHORS @@ -14,6 +14,8 @@ Stas Khirman Ryan Norton Andrew Resch +Thanks to (github user) nervoir for bug reports + Building and maintainance of the autotools scripts: Michael Wojciechowski Peter Koeleman diff --git a/ChangeLog b/ChangeLog index d5d7d8f20..938c6eef8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * fix infinite loop when parsing maliciously crafted torrents * fix invalid read in parse_int in bdecoder * fix issue with very long tracker- and web seed URLs * don't attempt to create empty files on startup, if they already exist diff --git a/include/libtorrent/string_util.hpp b/include/libtorrent/string_util.hpp index e8d90d427..6babe8147 100644 --- a/include/libtorrent/string_util.hpp +++ b/include/libtorrent/string_util.hpp @@ -103,6 +103,18 @@ namespace libtorrent bool is_i2p_url(std::string const& url); #endif + + // this can be used as the hash function in std::unordered_* + struct TORRENT_EXTRA_EXPORT string_hash_no_case + { size_t operator()(std::string const& s) const; }; + + // these can be used as the comparison functions in std::map and std::set + struct TORRENT_EXTRA_EXPORT string_eq_no_case + { bool operator()(std::string const& lhs, std::string const& rhs) const; }; + + struct TORRENT_EXTRA_EXPORT string_less_no_case + { bool operator()(std::string const& lhs, std::string const& rhs) const; }; + } #endif diff --git a/src/string_util.cpp b/src/string_util.cpp index 5cfca912d..00d90be18 100644 --- a/src/string_util.cpp +++ b/src/string_util.cpp @@ -277,5 +277,48 @@ namespace libtorrent #endif + std::size_t string_hash_no_case::operator()(std::string const& s) const + { + size_t ret = 5381; + for (std::string::const_iterator i = s.begin(); i != s.end(); ++i) + ret = (ret * 33) ^ to_lower(*i); + return ret; + } + + bool string_eq_no_case::operator()(std::string const& lhs, std::string const& rhs) const + { + if (lhs.size() != rhs.size()) return false; + + std::string::const_iterator s1 = lhs.begin(); + std::string::const_iterator s2 = rhs.begin(); + + while (s1 != lhs.end() && s2 != rhs.end()) + { + if (to_lower(*s1) != to_lower(*s2)) return false; + ++s1; + ++s2; + } + return true; + } + + bool string_less_no_case::operator()(std::string const& lhs, std::string const& rhs) const + { + std::string::const_iterator s1 = lhs.begin(); + std::string::const_iterator s2 = rhs.begin(); + + while (s1 != lhs.end() && s2 != rhs.end()) + { + char const c1 = to_lower(*s1); + char const c2 = to_lower(*s2); + if (c1 < c2) return true; + if (c1 > c2) return false; + ++s1; + ++s2; + } + + // this is the tie-breaker + return lhs.size() < rhs.size(); + } + } diff --git a/src/torrent_info.cpp b/src/torrent_info.cpp index 92053c745..a7124226a 100644 --- a/src/torrent_info.cpp +++ b/src/torrent_info.cpp @@ -497,65 +497,6 @@ namespace libtorrent return true; } -#if TORRENT_HAS_BOOST_UNORDERED - struct string_hash_no_case - { - size_t operator()(std::string const& s) const - { - char const* s1 = s.c_str(); - size_t ret = 5381; - int c; - - while ((c = *s1++)) - ret = (ret * 33) ^ to_lower(c); - - return ret; - } - }; - - struct string_eq_no_case - { - bool operator()(std::string const& lhs, std::string const& rhs) const - { - char c1, c2; - char const* s1 = lhs.c_str(); - char const* s2 = rhs.c_str(); - - while (*s1 != 0 && *s2 != 0) - { - c1 = to_lower(*s1); - c2 = to_lower(*s2); - if (c1 != c2) return false; - ++s1; - ++s2; - } - return *s1 == *s2; - } - }; - -#else - struct string_less_no_case - { - bool operator()(std::string const& lhs, std::string const& rhs) const - { - char c1, c2; - char const* s1 = lhs.c_str(); - char const* s2 = rhs.c_str(); - - while (*s1 != 0 || *s2 != 0) - { - c1 = to_lower(*s1); - c2 = to_lower(*s2); - if (c1 < c2) return true; - if (c1 > c2) return false; - ++s1; - ++s2; - } - return false; - } - }; -#endif - // root_dir is the name of the torrent, unless this is a single file // torrent, in which case it's empty. bool extract_files(bdecode_node const& list, file_storage& target diff --git a/test/test_string.cpp b/test/test_string.cpp index 6c74457dd..8c5e30822 100644 --- a/test/test_string.cpp +++ b/test/test_string.cpp @@ -310,3 +310,70 @@ TORRENT_TEST(string) , convert_to_native("foo") + convert_to_native("bar")); } +TORRENT_TEST(string_hash_no_case) +{ + string_hash_no_case hsh; + + // make sure different strings yield different hashes + TEST_CHECK(hsh("ab") != hsh("ba")); + + // make sure case is ignored + TEST_EQUAL(hsh("Ab"), hsh("ab")); + TEST_EQUAL(hsh("Ab"), hsh("aB")); + + // make sure zeroes in strings are supported + TEST_CHECK(hsh(std::string("\0a", 2)) != hsh(std::string("\0b", 2))); + TEST_EQUAL(hsh(std::string("\0a", 2)), hsh(std::string("\0a", 2))); +} + +TORRENT_TEST(string_eq_no_case) +{ + string_eq_no_case cmp; + TEST_CHECK(cmp("ab", "ba") == false); + TEST_CHECK(cmp("", "")); + TEST_CHECK(cmp("abc", "abc")); + + // make sure different lengths are correctly treated as different + TEST_CHECK(cmp("abc", "ab") == false); + + // make sure case is ignored + TEST_CHECK(cmp("Ab", "ab")); + TEST_CHECK(cmp("Ab", "aB")); + + // make sure zeros are supported + TEST_CHECK(cmp(std::string("\0a", 2), std::string("\0b", 2)) == false); + TEST_CHECK(cmp(std::string("\0a", 2), std::string("\0a", 2))); +} + +TORRENT_TEST(string_less_no_case) +{ + string_less_no_case cmp; + // ab < ba + TEST_CHECK(cmp("ab", "ba")); + TEST_CHECK(cmp("ba", "ab") == false); + + TEST_CHECK(cmp("", "") == false); + TEST_CHECK(cmp("", "a")); + TEST_CHECK(cmp("abc", "abc") == false); + + // shorter strings come before longer ones + TEST_CHECK(cmp("abc", "ab") == false); + TEST_CHECK(cmp("ab", "abc")); + + // make sure case is ignored + TEST_CHECK(cmp("Ab", "ba")); + TEST_CHECK(cmp("Ba", "ab") == false); + + TEST_CHECK(cmp("", "") == false); + TEST_CHECK(cmp("", "a")); + TEST_CHECK(cmp("abc", "Abc") == false); + + // shorter strings come before longer ones + TEST_CHECK(cmp("Abc", "ab") == false); + TEST_CHECK(cmp("Ab", "abc")); + + // make sure zeros are supported + TEST_CHECK(cmp(std::string("\0a", 2), std::string("\0b", 2))); + TEST_CHECK(cmp(std::string("\0a", 2), std::string("\0a", 2)) == false); +} + From b5fe0f95a2a5b335416f0602896886ca57472489 Mon Sep 17 00:00:00 2001 From: arvidn Date: Tue, 15 Aug 2017 13:37:12 +0200 Subject: [PATCH 5/5] fix issue in UTF-8 encoding validation --- ChangeLog | 1 + src/ConvertUTF.cpp | 2 +- src/torrent_info.cpp | 10 +++++++--- test/test_torrent_info.cpp | 22 ++++++++++++++++++++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 938c6eef8..15e648238 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * fix utf-8 encoding check in torrent parser * fix infinite loop when parsing maliciously crafted torrents * fix invalid read in parse_int in bdecoder * fix issue with very long tracker- and web seed URLs diff --git a/src/ConvertUTF.cpp b/src/ConvertUTF.cpp index 60ba17df1..49bcc3dc3 100644 --- a/src/ConvertUTF.cpp +++ b/src/ConvertUTF.cpp @@ -300,7 +300,7 @@ Boolean isLegalUTF8(const UTF8 *source, int length) { /* Everything else falls through when "true"... */ case 4: if ((a = (*--srcptr)) < 0x80 || a > 0xBF) return false; case 3: if ((a = (*--srcptr)) < 0x80 || a > 0xBF) return false; - case 2: if ((a = (*--srcptr)) > 0xBF) return false; + case 2: if ((a = (*--srcptr)) < 0x80 || a > 0xBF) return false; switch (*source) { /* no fall-through in this inner switch */ diff --git a/src/torrent_info.cpp b/src/torrent_info.cpp index a7124226a..b3f521057 100644 --- a/src/torrent_info.cpp +++ b/src/torrent_info.cpp @@ -255,8 +255,7 @@ namespace libtorrent continue; } - if (code_point < 0 - || !valid_path_character(code_point)) + if (code_point < 0 || !valid_path_character(code_point)) { // invalid utf8 sequence, replace with "_" path += '_'; @@ -265,9 +264,14 @@ namespace libtorrent continue; } + TORRENT_ASSERT(isLegalUTF8(reinterpret_cast(element + i), seq_len)); + // validation passed, add it to the output string for (int k = i; k < i + seq_len; ++k) + { + TORRENT_ASSERT(element[k] != 0); path.push_back(element[k]); + } if (code_point == '.') ++num_dots; @@ -618,7 +622,7 @@ namespace libtorrent { // as long as this file already exists // increase the counter - boost::uint32_t h = m_files.file_path_hash(i, empty_str); + boost::uint32_t const h = m_files.file_path_hash(i, empty_str); if (!files.insert(h).second) { // This filename appears to already exist! diff --git a/test/test_torrent_info.cpp b/test/test_torrent_info.cpp index 5cb081cf3..2311750b2 100644 --- a/test/test_torrent_info.cpp +++ b/test/test_torrent_info.cpp @@ -326,6 +326,11 @@ TORRENT_TEST(sanitize_path_trailing_spaces) TORRENT_TEST(sanitize_path) { std::string path; + + sanitize_append_path_element(path, "\0\0\xed\0\x80", 5); + TEST_EQUAL(path, "_"); + + path.clear(); sanitize_append_path_element(path, "/a/", 3); sanitize_append_path_element(path, "b", 1); sanitize_append_path_element(path, "c", 1); @@ -506,6 +511,17 @@ TORRENT_TEST(sanitize_path) TEST_EQUAL(path, "foobar"); } +TORRENT_TEST(sanitize_path_zeroes) +{ + std::string path; + sanitize_append_path_element(path, "\0foo", 4); + TEST_EQUAL(path, "foo"); + + path.clear(); + sanitize_append_path_element(path, "\0\0\0\0", 4); + TEST_EQUAL(path, ""); +} + TORRENT_TEST(verify_encoding) { // verify_encoding @@ -585,6 +601,12 @@ TORRENT_TEST(verify_encoding) TEST_CHECK(!verify_encoding(test)); fprintf(stdout, "%s\n", test.c_str()); TEST_CHECK(test == "filename____"); + + // missing byte header + test = "filename\xed\0\x80"; + TEST_CHECK(!verify_encoding(test)); + fprintf(stdout, "%s\n", test.c_str()); + TEST_CHECK(test == "filename_"); } TORRENT_TEST(parse_torrents)