Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement keysize mismatch #197

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

implement keysize mismatch #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
47addd7
add some failing tests
russelltg Mar 23, 2023
58110a2
add a few more cases
russelltg Mar 23, 2023
479658a
initial work
russelltg Mar 23, 2023
db053b6
finish
russelltg Mar 23, 2023
78f4ed7
don't use co;nflicting addresses
russelltg Mar 23, 2023
cb45645
fmt
russelltg Mar 23, 2023
ea00e43
Merge branch 'main' into key_size_mismatch
robertream May 27, 2024
b4b6d65
Update crypto.rs
robertream May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions srt-protocol/src/protocol/pending_connection/hsv5.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use std::{
time::Instant,
};

use log::warn;

use crate::{connection::ConnectionSettings, options::*, packet::*, settings::*};

use super::{ConnectError, ConnectionReject};
Expand Down Expand Up @@ -65,11 +67,12 @@ pub fn gen_access_control_response(
};

// crypto
let cipher = match (&settings.key_settings, &incoming.ext_km) {
let cipher = match (&mut settings.key_settings, &incoming.ext_km) {
// ok, both sides have crypto
(Some(key_settings), Some(SrtControlPacket::KeyRefreshRequest(km))) => {
if key_settings.key_size != incoming.key_size {
unimplemented!("Key size mismatch");
warn!("Key size mismatch: caller requested {:?}, listener was configured with {:?}. Selecting {:?}", incoming.key_size, key_settings.key_size, incoming.key_size);
key_settings.key_size = incoming.key_size;
Comment on lines +74 to +75
Copy link

@Sculas Sculas Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended behavior? If my understanding is correct, this means the client can change the server's encryption settings to what it wants.
It can even set the key size to 0, but thankfully that is then caught here by defaulting to Bytes16 instead of no encryption at all.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me confirm what the ref impl does...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it seems to base it off SRTO_SENDER
https://github.com/haivision/srt/blob/39822840c506d72cef5a742d28f32ea28e144345/srtcore/core.cpp#L4927

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently implement SRTO_SENDER because it is only really needed for old SRT compatibility (new SRT is always full duplex). However the docs say that this is the only modern use of it https://github.com/Haivision/srt/blob/master/docs/API/API-socket-options.md#SRTO_SENDER

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

$ srt-live-transmit "srt://:2222?pbkeylen=24&passphrase=password123" udp://127.0.0.1:3333 -a:no &
$ srt-live-transmit udp://:1111 "srt://127.0.0.1:2222?passphrase=password123&pbkeylen=16" -a:no
18:55:59.730216/SRT:RcvQ:w1!W:SRT.cn: processConnectResponse: PBKEYLEN conflict - keep 16; peer-advertised PBKEYLEN 24 rejected because Agent is SRTO_SENDER

$ srt-live-transmit udp://:1111 "srt://:2222?passphrase=password123&pbkeylen=16" -a:no &
$ srt-live-transmit "srt://127.0.0.1:2222?pbkeylen=24&passphrase=password123" udp://127.0.0.1:3333 -a:no
18:57:28.338138/SRT:RcvQ:w1!W:SRT.cn: processConnectResponse: PBKEYLEN conflict - OVERRIDDEN 24 by 16 from PEER (as AGENT is not SRTO_SENDER)

}

let cipher = match CipherSettings::new(key_settings, &settings.key_refresh, km) {
Expand Down
29 changes: 17 additions & 12 deletions srt-tokio/tests/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ use futures::{SinkExt, TryStreamExt};
use log::info;
use tokio::{select, spawn, time::sleep};

async fn test_crypto(size: u16) {
async fn test_crypto(size_listen: u16, size_call: u16, port: u16) {
let sender = SrtSocket::builder()
.encryption(size, "password123")
.listen_on(":2000");
.encryption(size_listen, "password123")
.listen_on(port);

let local_addr = format!("127.0.0.1:{port}");

let recvr = SrtSocket::builder()
.encryption(size, "password123")
.call("127.0.0.1:2000", None);
.encryption(size_call, "password123")
.call(local_addr.as_str(), None);

let t = spawn(async move {
let mut sender = sender.await.unwrap();
Expand All @@ -45,11 +47,16 @@ async fn test_crypto(size: u16) {
async fn crypto_exchange() {
let _ = pretty_env_logger::try_init();

test_crypto(16).await;
sleep(Duration::from_millis(100)).await;
test_crypto(24).await;
sleep(Duration::from_millis(100)).await;
test_crypto(32).await;
test_crypto(16, 16, 2000).await;
test_crypto(24, 24, 2001).await;
test_crypto(32, 32, 2002).await;
}

#[tokio::test]
async fn key_size_mismatch() {
test_crypto(32, 16, 2003).await;
test_crypto(32, 0, 2004).await;
test_crypto(0, 32, 2005).await;
}

#[tokio::test]
Expand Down Expand Up @@ -94,5 +101,3 @@ async fn bad_password_rendezvous() {

assert_matches!(result, Err(e) if e.kind() == io::ErrorKind::ConnectionRefused);
}

// TODO: mismatch
74 changes: 73 additions & 1 deletion srt-tokio/tests/stransmit_interop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ async fn receiver(

i += 1;

info!("Got pack!");
info!("Got pack {i}!");

// stransmit does not totally care if it sends 100% of it's packets
// (which is prob fair), just make sure that we got at least 2/3s of it
Expand Down Expand Up @@ -502,6 +502,78 @@ async fn bidirectional_interop_encrypt_rekey() -> Result<(), Error> {
Ok(())
}

#[tokio::test]
#[cfg(not(target_os = "windows"))]
async fn key_size_mismatch_rust_caller() -> Result<(), Error> {
let _ = pretty_env_logger::try_init();

const PACKETS: u32 = 1_000;

let mut child = allow_not_found!(Command::new("srt-live-transmit")
.arg("udp://:2814")
.arg("srt://:2815?passphrase=password123&pbkeylen=24")
.arg("-a:no")
.arg("-loglevel:debug")
.spawn());

let recvr_fut = async move {
let recv = SrtSocket::builder()
.encryption(16, "password123")
.call("127.0.0.1:2815", None)
.await
.unwrap();

try_join(
receiver(PACKETS, recv.map(|f| f.unwrap().1)),
udp_sender(PACKETS, 2814),
)
.await
.unwrap();
};

recvr_fut.await;
child.wait().unwrap();

Ok(())
}

#[tokio::test]
#[cfg(not(target_os = "windows"))]
async fn key_size_mismatch_rust_listener() -> Result<(), Error> {
let _ = pretty_env_logger::try_init();

const PACKETS: u32 = 1_000;

let mut child = allow_not_found!(Command::new("srt-live-transmit")
.arg("srt://127.0.0.1:2816?passphrase=password123&pbkeylen=24")
.arg("udp://:2817")
.arg("-a:no")
.arg("-loglevel:debug")
.spawn());

let sendr = async move {
let mut sender = SrtSocket::builder()
.encryption(16, "password123")
.local_port(2816)
.listen()
.await
.unwrap();

let mut stream =
counting_stream(PACKETS, Duration::from_millis(1)).map(|b| Ok((Instant::now(), b)));
sender.send_all(&mut stream).await.unwrap();
sender.close().await.unwrap();

Ok(())
};

try_join(sendr, udp_recvr(PACKETS, 2817)).await.unwrap();

child.wait().unwrap();

Ok(())
}

type HaiSocket = i32;

const SRTO_SENDER: c_int = 21;
Expand Down
Loading