You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A tunnel fragment that declares zero length is delivered as an I2NP message of zero length. GetPayloadLength () is GetLength () - I2NP_HEADER_SIZE, so it wraps around to (size_t)-16, and the handler that gets the message reads far past the buffer.
Reproduced with AddressSanitizer from a single crafted tunnel data message, no network involved:
TunnelEndpoint endpoint (true);
// padding fills the encrypted area so that the fragment ends exactly at its end,
// which is the branch where the tunnel message itself is passed on
fragment[0] = 0x00; // first fragment, delivery type local, not fragmented
htobe16buf (fragment + 1, 0); // declared size: zero
payload[4] = 1; // first byte of the iv, read as the I2NP type: database store
endpoint.HandleDecryptedTunnelDataMsg (msg);
==30723==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x51a000000b2d
READ of size 32 in i2p::data::Tag<32ul>::Tag(unsigned char const*) libi2pd/Tag.h:36
#1 i2p::data::NetDb::HandleDatabaseStoreMsg libi2pd/NetDb.cpp:832
0x51a000000b30 is located 0 bytes after 1200-byte region
allocated in i2p::tunnel::Tunnels::NewI2NPTunnelMessage libi2pd/Tunnel.cpp:1148
Every byte here comes from the plaintext of the tunnel message, checksum included, so the sender picks all of them, the fake I2NP type byte included. NetDb::HandleDatabaseStoreMsg is only the first handler to read; the wrapped-around length is visible to any of them.
The fix drops such a message where all delivery types funnel through, before the type is read.
With the change the same case logs TunnelMessage: Message of 0 bytes is shorter than I2NP header, dropped and there is no report.
Found by a set of generated fragment chains (first fragment plus follow-on fragments, out of order, duplicated, missing, lying about size). On the current head that set stops on a sanitizer report; with this change 500 chains run to the end with none. A soak with proxy traffic, SAM and config reloads on the same build is clean as well.
make has no new warnings, make -C tests passes, g++ -std=c++17 -fsyntax-only on the changed file passes.
For context: both transports already refuse a too short I2NP block before it becomes a message - NTCP2.cpp:1341 and SSU2Session.cpp:1811 both check size < 9. The tunnel endpoint had no such check, which is the gap this closes; the shape of the fix follows what the transports already do.
One more number: on a soak with real traffic on the same build - a site behind a server tunnel, requests through the http proxy, a SAM transfer and two config reloads - the new check never fired once (grep -c over both routers' logs returns 0), the page still opens after the reloads, and the run is free of sanitizer reports. So it rejects only what could not be a valid message in the first place.
Checked again against the current head (e3d3d7f, after 2537 and 2538 were merged): the single-message recipe still gives heap-buffer-overflow there, and is silent on this branch. The zip and http sets from the two merged changes are clean on that head as well - 3001 su3 cases, 412 zip truncation points and 200 000 mutated http messages, no reports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A tunnel fragment that declares zero length is delivered as an I2NP message of zero length.
GetPayloadLength ()isGetLength () - I2NP_HEADER_SIZE, so it wraps around to(size_t)-16, and the handler that gets the message reads far past the buffer.Reproduced with AddressSanitizer from a single crafted tunnel data message, no network involved:
Every byte here comes from the plaintext of the tunnel message, checksum included, so the sender picks all of them, the fake I2NP type byte included.
NetDb::HandleDatabaseStoreMsgis only the first handler to read; the wrapped-around length is visible to any of them.The fix drops such a message where all delivery types funnel through, before the type is read.
With the change the same case logs
TunnelMessage: Message of 0 bytes is shorter than I2NP header, droppedand there is no report.Found by a set of generated fragment chains (first fragment plus follow-on fragments, out of order, duplicated, missing, lying about size). On the current head that set stops on a sanitizer report; with this change 500 chains run to the end with none. A soak with proxy traffic, SAM and config reloads on the same build is clean as well.
makehas no new warnings,make -C testspasses,g++ -std=c++17 -fsyntax-onlyon the changed file passes.