shadowsocks / shadowsocks/shadowsocks-rust

AEAD-2022 TCP writer reports retry buffer length after Pending, dropping appended data

Open
#2,175 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
10.9k
Forks
1.5k
Avg merge
4d 15h
Merged PRs (30d)
5

Description

The AEAD-2022 TCP writer can report more plaintext bytes written than it actually sends when a retry after Poll::Pending supplies a longer input slice. Tokio's copy and copy_bidirectional can grow that slice while waiting for the writer, so this can silently drop data during TCP forwarding.

Reproduced with shadowsocks 1.25.0, tokio 1.53.1 and Rust 1.98.1 on Linux, using AEAD2022_BLAKE3_AES_128_GCM. The same write-state logic is still present on master at 157cefa96d44de848ff218119dbec2047826c1bb; I have not run the reproducer against master.

What happens
  1. Call poll_write_encrypted with b"a". The writer encrypts it, but the underlying stream returns Pending.
  2. Make the underlying stream writable and retry with b"ab".
  3. The writer sends its cached frame containing only a, then returns Ok(2).

Expected: report 1, leaving b for the caller to submit next, or actually send both bytes before reporting 2.

The Writing state retains the encrypted buffer and its wire offset, but returns the current buf.len() after sending that buffer. It does not retain the original plaintext length. Tokio's copy implementation can append input after a pending write.

Reproducer

This uses a controlled in-memory sink. It needs no server, network, runtime, or sleeps. Run cargo run with these two files.

Cargo.toml and src/main.rs

Cargo.toml:

[package]
name = "ss-pending-write-repro"
version = "0.1.0"
edition = "2024"

[dependencies]
shadowsocks = { version = "=1.25.0", default-features = false, features = ["aead-cipher-2022"] }
tokio = { version = "=1.53.1", features = ["io-util"] }

src/main.rs:

use shadowsocks::{
    config::ServerType,
    context::Context as SsContext,
    crypto::CipherKind,
    relay::tcprelay::crypto_io::{CryptoStream, CryptoWrite, StreamType},
};
use std::{
    io,
    pin::Pin,
    task::{Context, Poll, Waker},
};
use tokio::io::{AsyncRead, AsyncWrite, ReadBuf};

#[derive(Default)]
struct Sink {
    blocked: bool,
    bytes: Vec<u8>,
    waker: Option<Waker>,
}

impl AsyncRead for Sink {
    fn poll_read(
        self: Pin<&mut Self>,
        _: &mut Context<'_>,
        _: &mut ReadBuf<'_>,
    ) -> Poll<io::Result<()>> {
        Poll::Ready(Ok(()))
    }
}

impl AsyncWrite for Sink {
    fn poll_write(
        mut self: Pin<&mut Self>,
        cx: &mut Context<'_>,
        data: &[u8],
    ) -> Poll<io::Result<usize>> {
        if self.blocked {
            self.waker = Some(cx.waker().clone());
            return Poll::Pending;
        }
        self.bytes.extend_from_slice(data);
        Poll::Ready(Ok(data.len()))
    }

    fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<()>> {
        Poll::Ready(Ok(()))
    }

    fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<io::Result<()>> {
        Poll::Ready(Ok(()))
    }
}

fn main() {
    let context = SsContext::new_shared(ServerType::Local);
    let mut stream = CryptoStream::from_stream(
        &context,
        Sink {
            blocked: true,
            ..Sink::default()
        },
        StreamType::Client,
        CipherKind::AEAD2022_BLAKE3_AES_128_GCM,
        &[0; 16],
    );
    let mut cx = Context::from_waker(Waker::noop());

    assert!(
        Pin::new(&mut stream)
            .poll_write_encrypted(&mut cx, b"a")
            .is_pending()
    );

    let sink = stream.get_mut();
    sink.blocked = false;
    sink.waker.take().unwrap().wake();

    let Poll::Ready(Ok(written)) = Pin::new(&mut stream).poll_write_encrypted(&mut cx, b"ab")
    else {
        panic!("write did not complete");
    };
    println!("Reported plaintext bytes: {written}");
    println!("Ciphertext bytes: {}", stream.get_ref().bytes.len());
    // The cached frame contains only 'a'. The appended 'b' has not been sent.
    assert_eq!(written, 1);
}

Actual output:

Reported plaintext bytes: 2
Ciphertext bytes: 60
assertion `left == right` failed
  left: 2
 right: 1

The 60 wire bytes contain the 16-byte salt, 27-byte encrypted fixed header, and 17-byte encrypted one-byte payload. The appended byte is not in the frame.

We encountered this as an intermittent TCP payload timeout through ProxyServerStream, after the server-first greeting had succeeded. Our downstream workaround and duplex regression preserve the original write input across retries. Retaining the original plaintext length in the upstream write state appears to address the incorrect count.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in crates/shadowsocks/src/relay/tcprelay/aead_2022.rs around the Writing state at lines 727-740, then run the supplied two-file reproducer with the pinned shadowsocks and Tokio versions. Done means a retry after Poll::Pending reports only the original plaintext length and a regression test covers the appended-input case.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.