I think it would be better to do the check just before doing the offensive operation. Consider:
0void f()
1{
2 if (x >= a.size()) {
3 return or throw (dont continue below)
4 }
5 a[x] = 0;
vs
0void f()
1{
2 a[x] = 0; // it is not immediately obvious if x has been checked
3
4...
5
6// somewhere else in the code
7if (x >= a.size()) {
8 // make sure f() is not called
9}
10// but new code or a refactor could end up calling f() nevertheless
The following is shorter and I find it safer and it also avoids using magic numbers (+ unit test):
0diff --git i/src/i2p.cpp w/src/i2p.cpp
1index 05a5dde396..685b43ba18 100644
2--- i/src/i2p.cpp
3+++ w/src/i2p.cpp
4@@ -381,17 +381,32 @@ Binary Session::MyDestination() const
5 // "They are 387 bytes plus the certificate length specified at bytes 385-386, which may be
6 // non-zero"
7 static constexpr size_t DEST_LEN_BASE = 387;
8 static constexpr size_t CERT_LEN_POS = 385;
9
10 uint16_t cert_len;
11+
12+ if (m_private_key.size() < CERT_LEN_POS + sizeof(cert_len)) {
13+ throw std::runtime_error(strprintf("The private key is too short (%d < %d)",
14+ m_private_key.size(),
15+ CERT_LEN_POS + sizeof(cert_len)));
16+ }
17+
18 memcpy(&cert_len, &m_private_key.at(CERT_LEN_POS), sizeof(cert_len));
19 cert_len = be16toh(cert_len);
20
21 const size_t dest_len = DEST_LEN_BASE + cert_len;
22
23+ if (dest_len > m_private_key.size()) {
24+ throw std::runtime_error(strprintf("Certificate length (%d) designates that the private key should "
25+ "be %d bytes, but it is only %d bytes",
26+ cert_len,
27+ dest_len,
28+ m_private_key.size()));
29+ }
30+
31 return Binary{m_private_key.begin(), m_private_key.begin() + dest_len};
32 }
33
34 void Session::CreateIfNotCreatedAlready()
35 {
36 std::string errmsg;
37diff --git i/src/test/i2p_tests.cpp w/src/test/i2p_tests.cpp
38index 5b8b0e9215..d6bbe4fa8f 100644
39--- i/src/test/i2p_tests.cpp
40+++ w/src/test/i2p_tests.cpp
41@@ -6,12 +6,13 @@
42 #include <i2p.h>
43 #include <logging.h>
44 #include <netaddress.h>
45 #include <test/util/logging.h>
46 #include <test/util/net.h>
47 #include <test/util/setup_common.h>
48+#include <util/readwritefile.h>
49 #include <util/threadinterrupt.h>
50
51 #include <boost/test/unit_test.hpp>
52
53 #include <memory>
54 #include <string>
55@@ -122,7 +123,51 @@ BOOST_AUTO_TEST_CASE(listen_ok_accept_fail)
56 ASSERT_DEBUG_LOG("Destroying SAM session");
57 BOOST_REQUIRE(session.Listen(conn));
58 BOOST_REQUIRE(!session.Accept(conn));
59 }
60 }
61
62+BOOST_AUTO_TEST_CASE(damaged_private_key)
63+{
64+ const auto CreateSockOrig = CreateSock;
65+
66+ CreateSock = [](const CService&) {
67+ return std::make_unique<StaticContentsSock>("HELLO REPLY RESULT=OK VERSION=3.1\n"
68+ "SESSION STATUS RESULT=OK DESTINATION=\n");
69+ };
70+
71+ const auto i2p_private_key_file = gArgs.GetDataDirNet() / "test_i2p_private_key_damaged";
72+
73+ for (const auto& [file_contents, expected_error] : std::vector<std::tuple<std::string, std::string>>{
74+ {"", "The private key is too short (0 < 387)"},
75+
76+ {"abcd", "The private key is too short (4 < 387)"},
77+
78+ {std::string(386, '\0'), "The private key is too short (386 < 387)"},
79+
80+ {std::string(385, '\0') + '\0' + '\1',
81+ "Certificate length (1) designates that the private key should be 388 bytes, but it is only "
82+ "387 bytes"},
83+
84+ {std::string(385, '\0') + '\0' + '\5' + "abcd",
85+ "Certificate length (5) designates that the private key should be 392 bytes, but it is only "
86+ "391 bytes"}}) {
87+
88+ BOOST_REQUIRE(WriteBinaryFile(i2p_private_key_file, file_contents));
89+
90+ CThreadInterrupt interrupt;
91+ i2p::sam::Session session(i2p_private_key_file, CService{}, &interrupt);
92+
93+ {
94+ ASSERT_DEBUG_LOG("Creating persistent SAM session");
95+ ASSERT_DEBUG_LOG(expected_error);
96+
97+ i2p::Connection conn;
98+ bool proxy_error;
99+ BOOST_CHECK(!session.Connect(CService{}, conn, proxy_error));
100+ }
101+ }
102+
103+ CreateSock = CreateSockOrig;
104+}
105+
106 BOOST_AUTO_TEST_SUITE_END()
