webrtc-rs/sctp

A pure Rust implementation of SCTP

sctp's Issues

Stream::read: partial read OR returning the expected size of the buffer

The problem I'm facing right now is where I'm passing a buffer to Stream::read, which is not big enough (Error::ErrShortBuffer). Note there's no indication of the expected size => you're forced to guess the number.

Also note it's different from TcpStream https://doc.rust-lang.org/std/io/trait.Read.html#tymethod.read, which partially reads the data and does not put any restrictions on the size of the buffer (i.e. you, the caller, is in control of how fast you're consuming data).

I can see that the code in Pion is identical to one here, but I nonetheless think we need to change something.

Either

1. switch to "tcp stream" partial reading model
2. indicate the expected buffer size by changing ErrShortBuffer to be ErrShortBuffer { min: usize }

Comments and suggestions are welcome.

New release?

Are there plans to make a new release of this crate? I'd like to drop the local dependencies from the webrtc-rs crate but I need the changes in #17 because it enables the webrtc-rs <-> usrsctp interoperation.

Checking header.value_length for being too short

While fuzzing src::packet::Packet::unmarshal a lot of panics have shown their face.
The common theme seems to be that the code did check that header.value_length isn't too big.

What seems to have been missed, that it also leads to panics when this value is too small.

Often there are checks that at least check that the buffer has enough bytes, but then the buffer is sliced with a too small upper limit causing panics in all sorts of places.

// example from src/chunk/chunk_forward_tsn.rs
        let header = ChunkHeader::unmarshal(buf)?;

        if header.typ != CT_FORWARD_TSN {
            return Err(Error::ErrChunkTypeNotForwardTsn);
        }

        let mut offset = CHUNK_HEADER_SIZE + NEW_CUMULATIVE_TSN_LENGTH;
        if buf.len() < offset {
            return Err(Error::ErrChunkTooShort);
        }


        // !!!!! header.value_length might be smaller then offset. This is never checked !!!!!!!
        let reader = &mut buf.slice(CHUNK_HEADER_SIZE..CHUNK_HEADER_SIZE + header.value_length());

Many of these can probably be found by just going through each unmarshal impl. Most of them are pretty obvious once you know what you are looking for.

On the other hand, once #25 is merged, just running the fuzzer with cargo +nightly fuzz run packet will find many of them.

The workflow I used for this was:

1. (prerequisites) rustup toolchain add nightly and cargo install cargo-fuzz
2. run cargo +nightly fuzz run packet
3. Once this finds a panic it stops and writes the bad input into a file in fuzz/artifacts/packet
4. run cargo test artifacts
5. The test will panic and give you a backtrace to where it paniced
6. Go there and find the issue and fix it.

association: only reset `self.stored_init` once an ACK is received

What happened

got a weird error while integrating webrtc-rs into substrate:

2022-07-06 12:23:05.090 DEBUG tokio-runtime-worker webrtc_sctp::association::association_internal: [] failed to retransmit init (n_rtos=3): ErrInitNotStoredToSend

basically if the first send_init fails link

all the following attempts (via fn) will fail 100% because send_init "take"s value from Option.

what's the point of doing on_retransmission_timeout then?

Is it different from Go's Pion?

Yes. The go code does not "take" the value. It only "resets" storedInit to nil once it got an ACK link

Proposed solution

Do not take value out of Option.

Complete three timer related unit tests and one FIXME unit test

Figure out item visibilities when fuzzing

As discussed in #25 we have to make certain items visible outside the crate i.e. pub for fuzzing to be possible.

Ideal we'd find a way that let's these items still be pub(crate) when not fuzzing.

Something based on conditional visibility using #[cfg(fuzzing)] seems like a likely solve

Unknown paramtypes not ignored even if possible

Hi,

I am using this webrtc stack for internal testing. On the other side we use usrsctp. In the init chunk usrsctp sends a param for ECN (Explicit Congestion Notification) with ID 0x8000.

If the sctp implementation receiving this parameter does not supprt ECN, it is safe to ignore it, as per this section: of RFC 4960

      00 -  Stop processing this parameter; do not process any further
            parameters within this chunk.

      01 -  Stop processing this parameter, do not process any further
            parameters within this chunk, and report the unrecognized
            parameter in an 'Unrecognized Parameter', as described in
            [Section 3.2.2](https://datatracker.ietf.org/doc/html/rfc4960#section-3.2.2).

      10 -  Skip this parameter and continue processing.

      11 -  Skip this parameter and continue processing but report the
            unrecognized parameter in an 'Unrecognized Parameter', as
            described in [Section 3.2.2](https://datatracker.ietf.org/doc/html/rfc4960#section-3.2.2).

IIUC the current implementation refuses init chunks on ANY unknown parameter.

Possible fix for this:

• Extend ParamType::Unknown to contain two flags: report and stop_processing
• Make the init chunk parsing handle these cases correctly
• ChunkInit probably needs a second params field, for the params that need to be reported back as unknown
• Add them to the ChunkInit in handle_init
• handle_chunk probably also needs some check for chunks that are safe to ignore

Reading is not possible after `close`ing the Stream

Description

The comment here states that only the "writing" side is closed. In fact, after the Stream is closed, one can't read incoming data because of

sctp/src/stream/mod.rs

Line 180 in c20d296

while !self.closed.load(Ordering::SeqCst) {

.

Proposal

Allow reading from remote after the "writing" side is closed.

Refs libp2p/rust-libp2p#2681

Add graceful exit for examples

Fix ping pong examples with ctrl c

state cookie generation

looking at the state cookie generation implementation, shouldn't it include HMAC generation and other things as mentioned here?

test_assoc_unreliable_rexmit_unordered_fragment: range end index 2 out of range for slice of length 0

https://github.com/webrtc-rs/sctp/runs/7287646167?check_suite_focus=true

failures:
---- association::association_test::test_assoc_unreliable_rexmit_unordered_fragment stdout ----
thread 'association::association_test::test_assoc_unreliable_rexmit_unordered_fragment' panicked at 'range end index 2 out of range for slice of length 0', library\core\src\slice\index.rs:73:5
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

CT_CWR Wrongly Set to ECNE as a Return String

I have some observations while reviewing the chunk_type.rs file and the SCTP RFC 4960 document

sctp/src/chunk/chunk_type.rs

Line 21 in 24aca40

pub(crate) const CT_CWR: ChunkType = ChunkType(13);

sctp/src/chunk/chunk_type.rs

Line 42 in 24aca40

CT_CWR => "ECNE", // Explicit Congestion Notification Echo

According to RFC 4960 section 3.2,
12 - Reserved for Explicit Congestion Notification Echo (ECNE)
13 - Reserved for Congestion Window Reduced (CWR)

Line 42 should be: CT_CWR => "CWR",.

So there should be separate entries for ECNE (i.e ID Value 12)

pub(crate) const CT_ECNE: ChunkType = ChunkType(12);
CT_ECNE => "ECNE",

Checking chunk parameter length before unmarshalling

As discorvered by @melekes in #17 many parameters do not validate the length of the body matches the expected length as given by the header. This can lead to panics in the handling code.

This could be fixed by making a check in the build_param function.

A quick look suggests that this also is an issue for chunk unmarshalling. This is probably a pervasive issue in the codebase.

• Parameters have this issue. Fixable by checking size in src/param/mod.rs::build_param Edit: This is already checked in ParamHeader::unmarshal
• Chunks have this issue. Fixable by checking size in Packet::unmarshal Edit: This is already checked in ChunkHeader::unmarshal

After more investigation the original culprits seem to be None-issues because they were already checked, in an unexpected place.