Skip to content

Commit 72f7c1c

Browse files
authored
Merge pull request #621 from Nukesor/fix-dos
fix: pueued crash with malformed secret payload
2 parents b74a573 + 221088e commit 72f7c1c

File tree

10 files changed

+204
-9
lines changed

10 files changed

+204
-9
lines changed

.github/ISSUE_TEMPLATE/bug_report.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ body:
3838
Logs helps me to debug a problem, especially if the bug is something that's not clearly visible.
3939
4040
You can get detailed log output by launching `pueue` or `pueued` with the `-vvv` flag directly after the binary name.
41+
42+
In case of a panic/crash, please run the program with the `RUST_BACKTRACE=1` environment variable set.
43+
That way we get a proper stack trace.
4144
placeholder: |
4245
```
4346
Some log output here

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ Upon updating Pueue and restarting the daemon, the previous state will be wiped,
140140
- Callback templating arguments were html escaped by accident. [#564](https://github.com/Nukesor/pueue/pull/564)
141141
- Print incompatible version warning info as a log message instead of plain stdout input, which broke json outputs [#562](https://github.com/Nukesor/pueue/issues/562).
142142
- Fixed `-d` daemon mode on Windows. [#344](https://github.com/Nukesor/pueue/issues/344)
143+
- Fixed a pueued crash when malformed secret exchange messages are sent by a connecting client [#619](https://github.com/Nukesor/pueue/issues/619).
143144

144145
### Remove
145146

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,14 @@ snap = "1.1"
2525
strum = { version = "0.27", features = ["derive"] }
2626
tokio = { version = "1.43", features = ["io-std", "rt-multi-thread", "time"] }
2727
tracing = "0.1.41"
28+
tracing-error = "0.2.1"
29+
tracing-subscriber = { version = "0.3.19", features = [
30+
"chrono",
31+
"env-filter",
32+
"fmt",
33+
"local-time",
34+
] }
35+
2836

2937
[profile.release]
3038
codegen-units = 1

pueue/Cargo.toml

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,8 @@ tempfile = "3"
5151
tokio.workspace = true
5252
toml = "0.8"
5353
tracing.workspace = true
54-
tracing-error = "0.2.1"
55-
tracing-subscriber = { version = "0.3.19", features = [
56-
"chrono",
57-
"env-filter",
58-
"fmt",
59-
"local-time",
60-
] }
54+
tracing-error.workspace = true
55+
tracing-subscriber.workspace = true
6156

6257
[dev-dependencies]
6358
assert_cmd = "2"

pueue/src/daemon/network/socket.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,11 @@ async fn handle_incoming(
5959
secret: Vec<u8>,
6060
) -> Result<()> {
6161
// Receive the secret once and check, whether the client is allowed to connect
62-
let payload_bytes = receive_bytes(&mut stream).await?;
62+
// We only allow max payload sizes of 4MB for this one.
63+
// Daemon's might be exposed publicly and get random traffic, potentially announcing huge
64+
// payloads that would result in an OOM.
65+
let payload_bytes =
66+
receive_bytes_with_max_size(&mut stream, Some(4 * (2usize.pow(20)))).await?;
6367

6468
// Didn't receive any bytes. The client disconnected.
6569
if payload_bytes.is_empty() {

pueue_lib/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ portpicker = "0.1"
5252
pretty_assertions.workspace = true
5353
tempfile = "3"
5454
tokio.workspace = true
55+
tracing-error.workspace = true
56+
tracing-subscriber.workspace = true
5557

5658
# --- Platform specific dependencies ---
5759
# Unix

pueue_lib/src/error.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ pub enum Error {
2626
#[error("Couldn't serialize message:\n{}", .0)]
2727
MessageSerialization(String),
2828

29+
#[error("Requested message size of {} with only {} being allowed.", .0, .1)]
30+
MessageTooBig(usize, usize),
31+
2932
#[error("Error while reading configuration:\n{}", .0)]
3033
ConfigDeserialization(String),
3134

pueue_lib/src/network/protocol.rs

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,20 @@ pub async fn send_bytes(payload: &[u8], stream: &mut GenericStream) -> Result<()
8888
Ok(())
8989
}
9090

91+
pub async fn receive_bytes(stream: &mut GenericStream) -> Result<Vec<u8>, Error> {
92+
receive_bytes_with_max_size(stream, None).await
93+
}
94+
9195
/// Receive a byte stream. \
9296
/// This is part of the basic protocol beneath all communication. \
9397
///
9498
/// 1. First of, the client sends a u64 as a 4byte vector in BigEndian mode, which specifies the
9599
/// length of the payload we're going to receive.
96100
/// 2. Receive chunks of [PACKET_SIZE] bytes until we finished all expected bytes.
97-
pub async fn receive_bytes(stream: &mut GenericStream) -> Result<Vec<u8>, Error> {
101+
pub async fn receive_bytes_with_max_size(
102+
stream: &mut GenericStream,
103+
max_size: Option<usize>,
104+
) -> Result<Vec<u8>, Error> {
98105
// Receive the header with the overall message size
99106
let mut header = vec![0; 8];
100107
stream
@@ -104,6 +111,21 @@ pub async fn receive_bytes(stream: &mut GenericStream) -> Result<Vec<u8>, Error>
104111
let mut header = Cursor::new(header);
105112
let message_size = ReadBytesExt::read_u64::<BigEndian>(&mut header)? as usize;
106113

114+
if let Some(max_size) = max_size {
115+
if message_size > max_size {
116+
error!(
117+
"Client requested message size of {message_size}, but only {max_size} is allowed."
118+
);
119+
return Err(Error::MessageTooBig(message_size, max_size));
120+
}
121+
}
122+
123+
// Show a warning if we see unusually large payloads. In this case payloads that're bigger than
124+
// 20MB, which is pretty large considering pueue is usually only sending a bit of text.
125+
if message_size > (20 * (2usize.pow(20))) {
126+
warn!("Client is sending a large payload: {message_size} bytes.");
127+
}
128+
107129
// Buffer for the whole payload
108130
let mut payload_bytes = Vec::with_capacity(message_size);
109131

@@ -281,4 +303,104 @@ mod test {
281303

282304
Ok(())
283305
}
306+
307+
use tracing::level_filters::LevelFilter;
308+
use tracing_subscriber::{
309+
EnvFilter, Layer, Registry, field::MakeExt, filter::FromEnvError, fmt::time::ChronoLocal,
310+
layer::SubscriberExt, util::SubscriberInitExt,
311+
};
312+
313+
pub fn install_tracing(verbosity: u8) -> Result<(), FromEnvError> {
314+
let mut pretty = false;
315+
let level = match verbosity {
316+
0 => LevelFilter::WARN,
317+
1 => LevelFilter::INFO,
318+
2 => LevelFilter::DEBUG,
319+
3 => LevelFilter::TRACE,
320+
_ => {
321+
pretty = true;
322+
LevelFilter::TRACE
323+
}
324+
};
325+
326+
// tries to find local offset internally
327+
let timer = ChronoLocal::new("%H:%M:%S".into());
328+
329+
type GenericLayer<S> = Box<dyn tracing_subscriber::Layer<S> + Send + Sync>;
330+
let fmt_layer: GenericLayer<_> = match pretty {
331+
false => Box::new(
332+
tracing_subscriber::fmt::layer()
333+
.map_fmt_fields(|f| f.debug_alt())
334+
.with_timer(timer)
335+
.with_writer(std::io::stderr),
336+
),
337+
true => Box::new(
338+
tracing_subscriber::fmt::layer()
339+
.pretty()
340+
.with_timer(timer)
341+
.with_target(true)
342+
.with_thread_ids(false)
343+
.with_thread_names(true)
344+
.with_level(true)
345+
.with_ansi(true)
346+
.with_span_events(tracing_subscriber::fmt::format::FmtSpan::ACTIVE)
347+
.with_writer(std::io::stderr),
348+
),
349+
};
350+
let filter_layer = EnvFilter::builder()
351+
.with_default_directive(level.into())
352+
.from_env()?;
353+
354+
Registry::default()
355+
.with(fmt_layer.with_filter(filter_layer))
356+
.with(tracing_error::ErrorLayer::default())
357+
.init();
358+
359+
Ok(())
360+
}
361+
362+
/// Ensure there's no OOM if a huge payload during the handshake phase is being requested.
363+
///
364+
/// We limit the receiving buffer to ~4MB for the incoming secret to prevent (potentially
365+
/// unintended) DoS attacks when something connect to Pueue and sends a malformed secret
366+
/// payload.
367+
#[tokio::test]
368+
async fn test_restricted_payload_size() -> Result<(), Error> {
369+
install_tracing(3)
370+
.expect("Couldn't init tracing for test, have you initialised tracing twice?");
371+
let listener = TcpListener::bind("127.0.0.1:0").await?;
372+
let addr = listener.local_addr()?;
373+
374+
let listener: GenericListener = Box::new(listener);
375+
376+
// Spawn a sub thread that:
377+
// 1. Accepts a new connection.
378+
// 2. Sends a malformed payload.
379+
task::spawn(async move {
380+
let mut stream = listener.accept().await.unwrap();
381+
382+
// Send a payload of 9 bytes to the daemon receiver.
383+
// The first 8 bytes determine the payload size in BigEndian.
384+
// This payload requests 2^64 bytes of memory for the secret.
385+
stream
386+
.write_all(&[128, 0, 0, 0, 0, 0, 0, 0, 0])
387+
.await
388+
.unwrap();
389+
});
390+
391+
// Create a receiver stream
392+
let mut client: GenericStream = Box::new(TcpStream::connect(&addr).await?);
393+
// Wait for a short time to allow the sender to send the message
394+
tokio::time::sleep(Duration::from_millis(500)).await;
395+
396+
// Get the message while restricting the payload size to 4MB
397+
let result = receive_bytes_with_max_size(&mut client, Some(4 * 2usize.pow(20))).await;
398+
399+
assert!(
400+
result.is_err(),
401+
"The payload should be rejected due to large size"
402+
);
403+
404+
Ok(())
405+
}
284406
}

pueue_lib/tests/helper.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
use portpicker::pick_unused_port;
22
use pueue_lib::settings::*;
33
use tempfile::{Builder, TempDir};
4+
use tracing::level_filters::LevelFilter;
5+
use tracing_subscriber::{
6+
EnvFilter, Layer, Registry, field::MakeExt, filter::FromEnvError, fmt::time::ChronoLocal,
7+
layer::SubscriberExt, util::SubscriberInitExt,
8+
};
49

510
pub fn get_shared_settings(
611
#[cfg_attr(target_os = "windows", allow(unused_variables))] use_unix_socket: bool,
@@ -33,3 +38,53 @@ pub fn get_shared_settings(
3338

3439
(shared_settings, tempdir)
3540
}
41+
42+
#[allow(dead_code)]
43+
pub fn install_tracing(verbosity: u8) -> Result<(), FromEnvError> {
44+
let mut pretty = false;
45+
let level = match verbosity {
46+
0 => LevelFilter::WARN,
47+
1 => LevelFilter::INFO,
48+
2 => LevelFilter::DEBUG,
49+
3 => LevelFilter::TRACE,
50+
_ => {
51+
pretty = true;
52+
LevelFilter::TRACE
53+
}
54+
};
55+
56+
// tries to find local offset internally
57+
let timer = ChronoLocal::new("%H:%M:%S".into());
58+
59+
type GenericLayer<S> = Box<dyn tracing_subscriber::Layer<S> + Send + Sync>;
60+
let fmt_layer: GenericLayer<_> = match pretty {
61+
false => Box::new(
62+
tracing_subscriber::fmt::layer()
63+
.map_fmt_fields(|f| f.debug_alt())
64+
.with_timer(timer)
65+
.with_writer(std::io::stderr),
66+
),
67+
true => Box::new(
68+
tracing_subscriber::fmt::layer()
69+
.pretty()
70+
.with_timer(timer)
71+
.with_target(true)
72+
.with_thread_ids(false)
73+
.with_thread_names(true)
74+
.with_level(true)
75+
.with_ansi(true)
76+
.with_span_events(tracing_subscriber::fmt::format::FmtSpan::ACTIVE)
77+
.with_writer(std::io::stderr),
78+
),
79+
};
80+
let filter_layer = EnvFilter::builder()
81+
.with_default_directive(level.into())
82+
.from_env()?;
83+
84+
Registry::default()
85+
.with(fmt_layer.with_filter(filter_layer))
86+
.with(tracing_error::ErrorLayer::default())
87+
.init();
88+
89+
Ok(())
90+
}

0 commit comments

Comments
 (0)