Out-of-bounds write in parseBenc

[guidovranken]

This code:

transmission/libtransmission/torrent-metainfo.cc

Lines 458 to 461 in 4bc1589

	auto const n = std::size(value) / sizeof(tr_sha1_digest_t);
	tm_.pieces_.resize(n);
	std::copy_n(std::data(value), std::size(value), reinterpret_cast<char*>(std::data(tm_.pieces_)));
	tm_.pieces_offset_ = context.tokenSpan().first;

Allocates enough room for n elements, but then proceeds to copy n elements AND the remaining data. So if std::size(value) is 21 (and sizeof(tr_sha1_digest_t) is 20), then it allocates 1 element (= 20 bytes), and copy 21 bytes to it.

Test for this issue:

diff --git a/tests/libtransmission/torrent-metainfo-test.cc b/tests/libtransmission/torrent-metainfo-test.cc
index 2253d41..ee208ea 100644
--- a/tests/libtransmission/torrent-metainfo-test.cc
+++ b/tests/libtransmission/torrent-metainfo-test.cc
@@ -255,5 +255,11 @@ TEST_F(TorrentMetainfoTest, GetRightStyleWebseedString)
     EXPECT_EQ("http://www.webseed-one.com/"sv, tm.webseed(0));
 }
 
+TEST_F(TorrentMetainfoTest, parseBencOOBWrite)
+{
+    auto tm = tr_torrent_metainfo{};
+    tm.parseBenc(tr_base64_decode("ZGg0OmluZm9kNjpwaWVjZXMzOkFpzQ=="));
+}
+
 } // namespace test
 } // namespace libtransmission

Potential fix (it addresses the issue, but I don't know if it's compliant with the relevant specs):

diff --git a/libtransmission/torrent-metainfo.cc b/libtransmission/torrent-metainfo.cc
index cec210d..1885902 100644
--- a/libtransmission/torrent-metainfo.cc
+++ b/libtransmission/torrent-metainfo.cc
@@ -455,10 +455,14 @@ struct MetainfoHandler final : public transmission::benc::BasicHandler<MaxBencDe
         }
         else if (pathIs(InfoKey, PiecesKey))
         {
-            auto const n = std::size(value) / sizeof(tr_sha1_digest_t);
-            tm_.pieces_.resize(n);
-            std::copy_n(std::data(value), std::size(value), reinterpret_cast<char*>(std::data(tm_.pieces_)));
-            tm_.pieces_offset_ = context.tokenSpan().first;
+            if (std::size(value) % sizeof(tr_sha1_digest_t) == 0) {
+                auto const n = std::size(value) / sizeof(tr_sha1_digest_t);
+                tm_.pieces_.resize(n);
+                std::copy_n(std::data(value), std::size(value), reinterpret_cast<char*>(std::data(tm_.pieces_)));
+                tm_.pieces_offset_ = context.tokenSpan().first;
+            } else {
+                unhandled = true;
+            }
         }
         else if (pathStartsWith(PieceLayersKey))
         {

[guidovranken]

If you're satisfied with both the test and the fix, I'll submit a PR.

[guidovranken]

A more lax approach:

diff --git a/libtransmission/torrent-metainfo.cc b/libtransmission/torrent-metainfo.cc
index cec210d..6470590 100644
--- a/libtransmission/torrent-metainfo.cc
+++ b/libtransmission/torrent-metainfo.cc
@@ -457,7 +457,10 @@ struct MetainfoHandler final : public transmission::benc::BasicHandler<MaxBencDe
         {
             auto const n = std::size(value) / sizeof(tr_sha1_digest_t);
             tm_.pieces_.resize(n);
-            std::copy_n(std::data(value), std::size(value), reinterpret_cast<char*>(std::data(tm_.pieces_)));
+            std::copy_n(
+                    std::data(value),
+                    n * sizeof(tr_sha1_digest_t),
+                    reinterpret_cast<char*>(std::data(tm_.pieces_)));
             tm_.pieces_offset_ = context.tokenSpan().first;
         }
         else if (pathStartsWith(PieceLayersKey))

This simply ignores the excess bytes.

[ckerr]

At a minimum we should log m error. I’m inclined to say refuse to parse a torrent with the wrong length since it’s likely a sign of a corrupt torrent. That array should always be cleanly divisible by 20.

I’d be fine with a PR that checks this to safeguard against corrupt torrents.