denoland / fastwebsockets

A fast RFC6455 WebSocket implementation
https://docs.rs/fastwebsockets/
Apache License 2.0
845 stars 61 forks source link

Fix header parsing for 32bits architecture #52

Closed erebe closed 11 months ago

erebe commented 11 months ago

When using a 32 bit CPU architecture if payload length is 8 bytes wide, the library crash with

thread 'tokio-runtime-worker' panicked at /cargo/git/checkouts/fastwebsockets-0824f4774dd6468b/6433d9d/src/lib.rs:680:58:
called `Result::unwrap()` on an `Err` value: TryFromSliceError(())

The reason is that usize is machine size dependent and on a 32Bit architecture, it is only 4 bytes wide. When the lib is trying to stuff 8 bytes into the usize the convertion fails. As https://datatracker.ietf.org/doc/html/rfc6455#page-29 it should be considered a u64 length on any architecture.

Sadly, while the patch avoid the immediate crash on 32bit arch if the payload length is 8 bytes wide, it does not solve the potential overflow if the payload length goes beyonf usize::MAX.

I tried to change the length from usize to u64, but as all Rust struct use usize and not u64 for their capacity/indexing it was not tractable.

If you prefer, I can do a checked conversion and return an error but, as there are a lot of unwraps and still some potential overflow, I was not sure if you want to bother with 32 bit CPU arch and correctness.

      match extra {
        2 => u16::from_be_bytes(head[2..4].try_into().unwrap()) as usize,
        8 => match usize::try_from(u64::from_be_bytes(head[2..10].try_into().unwrap())) {
          Ok(len) => len,
          Err(_) => return Err(WebSocketError::FrameTooLarge),
        },
        _ => unreachable!(),
      }
    } else {
      usize::from(length_code)
    };

Other part of the code tha can overflow, if length == usize::MAX

let required = 2 + extra + mask.map(|_| 4).unwrap_or(0) + length;
littledivy commented 11 months ago

Thanks for the PR. I think the checked conversion sounds like a better option, maybe with a conditional compilation block:

match extra {
  // ..
  #[cfg(target_pointer_width = "32")]
  8 => match usize::try_from(u64::from_be_bytes(...)) {
    Ok(size) => size,
    Err(_) => return Err(WebSocketError::FrameTooLarge),
  }
}
erebe commented 11 months ago

I have updated the PR, let me know if it feels right to you.

erebe commented 11 months ago

Thank you :)