Skip to content

Commit fcdfd3e

Browse files
fix(server): avoid i64 overflow in SSH session TTL
The SSH session expiry calculation cast a u64 TTL to i64 and multiplied by 1000 without bounds checking. A configured TTL larger than i64::MAX / 1000 overflows and panics in debug builds. Use i64::try_from with saturating_mul and saturating_add so any out-of-range TTL clamps to i64::MAX milliseconds. Signed-off-by: Andrew White <andrewh@cdw.com> Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
1 parent fde96f0 commit fcdfd3e

7 files changed

Lines changed: 247 additions & 11 deletions

File tree

crates/openshell-cli/src/commands/common.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,9 @@ pub fn parse_duration_to_ms(s: &str) -> Result<i64> {
726726
let num: i64 = num_str
727727
.parse()
728728
.map_err(|_| miette::miette!("invalid duration: {s} (expected e.g. 5m, 1h, 30s)"))?;
729+
if num < 0 {
730+
return Err(miette::miette!("duration must not be negative: {s}"));
731+
}
729732
let multiplier = match unit {
730733
"s" => 1_000,
731734
"m" => 60_000,
@@ -736,7 +739,8 @@ pub fn parse_duration_to_ms(s: &str) -> Result<i64> {
736739
));
737740
}
738741
};
739-
Ok(num * multiplier)
742+
num.checked_mul(multiplier)
743+
.ok_or_else(|| miette::miette!("duration value is too large: {s}"))
740744
}
741745

742746
// ---------------------------------------------------------------------------
@@ -975,4 +979,24 @@ mod tests {
975979
let err = parse_duration_to_ms("\u{20ac}").expect_err("missing number should error");
976980
assert!(err.to_string().contains("invalid duration"));
977981
}
982+
983+
#[test]
984+
fn parse_duration_to_ms_rejects_overflow() {
985+
let err = parse_duration_to_ms("100000000000000h").expect_err("overflow should error");
986+
assert!(err.to_string().contains("too large"));
987+
}
988+
989+
#[test]
990+
fn parse_duration_to_ms_rejects_negative() {
991+
let err = parse_duration_to_ms("-5m").expect_err("negative duration should error");
992+
assert!(err.to_string().contains("negative"));
993+
}
994+
995+
#[test]
996+
fn parse_duration_to_ms_accepts_zero_and_valid() {
997+
assert_eq!(parse_duration_to_ms("0s").unwrap(), 0);
998+
assert_eq!(parse_duration_to_ms("30s").unwrap(), 30_000);
999+
assert_eq!(parse_duration_to_ms("5m").unwrap(), 300_000);
1000+
assert_eq!(parse_duration_to_ms("2h").unwrap(), 7_200_000);
1001+
}
9781002
}

crates/openshell-cli/src/run.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6574,7 +6574,10 @@ pub async fn sandbox_logs(
65746574
.as_millis(),
65756575
)
65766576
.into_diagnostic()?;
6577-
now_ms - dur_ms
6577+
// Negative durations are rejected by parse_duration_to_ms. Overlong
6578+
// durations are clamped to zero (show all logs since the epoch) rather
6579+
// than silently producing a future timestamp.
6580+
now_ms.checked_sub(dur_ms).unwrap_or(0).max(0)
65786581
} else {
65796582
0
65806583
};

crates/openshell-driver-docker/src/lib.rs

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2750,7 +2750,9 @@ fn parse_cpu_limit(value: &str) -> Result<Option<i64>, Status> {
27502750
"docker cpu_limit must be greater than zero",
27512751
));
27522752
}
2753-
return Ok(Some(millicores.saturating_mul(1_000_000)));
2753+
return millicores.checked_mul(1_000_000).map(Some).ok_or_else(|| {
2754+
Status::failed_precondition(format!("docker cpu_limit '{value}' is too large"))
2755+
});
27542756
}
27552757

27562758
let cores = value.parse::<f64>().map_err(|_| {
@@ -2764,7 +2766,15 @@ fn parse_cpu_limit(value: &str) -> Result<Option<i64>, Status> {
27642766
));
27652767
}
27662768

2767-
Ok(Some((cores * 1_000_000_000.0).round() as i64))
2769+
let nano_cpus = (cores * 1_000_000_000.0).round();
2770+
#[allow(clippy::cast_precision_loss)]
2771+
if !nano_cpus.is_finite() || nano_cpus < i64::MIN as f64 || nano_cpus >= i64::MAX as f64 {
2772+
return Err(Status::failed_precondition(format!(
2773+
"docker cpu_limit '{value}' is too large",
2774+
)));
2775+
}
2776+
2777+
Ok(Some(nano_cpus as i64))
27682778
}
27692779

27702780
#[allow(clippy::cast_possible_truncation)]
@@ -2810,7 +2820,15 @@ fn parse_memory_limit(value: &str) -> Result<Option<i64>, Status> {
28102820
}
28112821
};
28122822

2813-
Ok(Some((amount * multiplier).round() as i64))
2823+
let bytes = (amount * multiplier).round();
2824+
#[allow(clippy::cast_precision_loss)]
2825+
if !bytes.is_finite() || bytes < i64::MIN as f64 || bytes >= i64::MAX as f64 {
2826+
return Err(Status::failed_precondition(format!(
2827+
"docker memory_limit '{value}' is too large",
2828+
)));
2829+
}
2830+
2831+
Ok(Some(bytes as i64))
28142832
}
28152833

28162834
fn sandbox_from_container_summary(summary: &ContainerSummary) -> Option<DriverSandbox> {

crates/openshell-driver-docker/src/tests.rs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2288,3 +2288,59 @@ fn container_state_needs_resume_matches_startable_states() {
22882288
);
22892289
}
22902290
}
2291+
2292+
#[test]
2293+
fn parse_cpu_limit_rejects_overflow() {
2294+
let err = parse_cpu_limit("1e300").unwrap_err();
2295+
assert!(err.message().contains("too large"));
2296+
}
2297+
2298+
#[test]
2299+
fn parse_memory_limit_rejects_overflow() {
2300+
// 308 nines is finite as f64 (~1e308), but multiplying by Gi overflows to inf.
2301+
let huge = "9".repeat(308) + "Gi";
2302+
let err = parse_memory_limit(&huge).unwrap_err();
2303+
assert!(err.message().contains("too large"));
2304+
}
2305+
2306+
#[test]
2307+
fn parse_cpu_limit_rejects_i64_max_boundary() {
2308+
// 9_223_372_036.854776 cores * 1e9 rounds exactly to 2^63, which used to
2309+
// pass the > check and silently saturate to i64::MAX. It must now be
2310+
// rejected. (9_223_372_037 is already above the boundary and would also
2311+
// pass a > guard, so it does not test the equality case.)
2312+
let err = parse_cpu_limit("9223372036.854776").unwrap_err();
2313+
assert!(err.message().contains("too large"));
2314+
2315+
// One core below the boundary is still valid.
2316+
assert_eq!(
2317+
parse_cpu_limit("9223372036").unwrap(),
2318+
Some(9_223_372_036_000_000_000)
2319+
);
2320+
}
2321+
2322+
#[test]
2323+
fn parse_cpu_limit_rejects_millicore_overflow() {
2324+
// 9_223_372_036_854 millicores * 1_000_000 == 9_223_372_036_854_000_000,
2325+
// which fits in i64. One millicore more overflows and must be rejected.
2326+
assert_eq!(
2327+
parse_cpu_limit("9223372036854m").unwrap(),
2328+
Some(9_223_372_036_854_000_000)
2329+
);
2330+
let err = parse_cpu_limit("9223372036855m").unwrap_err();
2331+
assert!(err.message().contains("too large"));
2332+
}
2333+
2334+
#[test]
2335+
fn parse_memory_limit_rejects_i64_max_boundary() {
2336+
// 8192 PiB = 2^63 bytes, which used to pass the > check and silently
2337+
// saturate to i64::MAX. It must now be rejected.
2338+
let err = parse_memory_limit("8192Pi").unwrap_err();
2339+
assert!(err.message().contains("too large"));
2340+
2341+
// One PiB below the boundary is still valid.
2342+
assert_eq!(
2343+
parse_memory_limit("8191Pi").unwrap(),
2344+
Some(8191 * 1024_i64.pow(5))
2345+
);
2346+
}

crates/openshell-driver-podman/src/container.rs

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,7 +1074,25 @@ pub fn build_container_spec_for_image(
10741074
openshell_core::config::DEFAULT_SSH_PORT
10751075
),
10761076
],
1077-
interval: config.health_check_interval_secs * 1_000_000_000,
1077+
interval: {
1078+
const NS_PER_S: u64 = 1_000_000_000;
1079+
let max_secs = (i64::MAX as u64) / NS_PER_S;
1080+
if config.health_check_interval_secs > max_secs {
1081+
return Err(ComputeDriverError::InvalidArgument(format!(
1082+
"health_check_interval_secs {} exceeds maximum allowed nanoseconds",
1083+
config.health_check_interval_secs
1084+
)));
1085+
}
1086+
config
1087+
.health_check_interval_secs
1088+
.checked_mul(NS_PER_S)
1089+
.ok_or_else(|| {
1090+
ComputeDriverError::InvalidArgument(format!(
1091+
"health_check_interval_secs {} exceeds maximum allowed nanoseconds",
1092+
config.health_check_interval_secs
1093+
))
1094+
})?
1095+
},
10781096
timeout: 2_000_000_000,
10791097
retries: 10,
10801098
start_period: 5_000_000_000,
@@ -1211,8 +1229,13 @@ fn parse_cpu_to_microseconds(quantity: &str) -> Option<u64> {
12111229
if cores <= 0.0 || cores.is_nan() || cores.is_infinite() {
12121230
return None;
12131231
}
1232+
let micros_f = cores * 100_000.0;
1233+
#[allow(clippy::cast_precision_loss)]
1234+
if !micros_f.is_finite() || micros_f >= u64::MAX as f64 {
1235+
return None;
1236+
}
12141237
#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
1215-
let val = (cores * 100_000.0) as u64;
1238+
let val = micros_f as u64;
12161239
val
12171240
};
12181241
// A quota of 0 microseconds is invalid — treat as no limit.
@@ -1287,6 +1310,56 @@ mod tests {
12871310
assert_eq!(parse_cpu_to_microseconds("0.5"), Some(50_000));
12881311
}
12891312

1313+
#[test]
1314+
fn parse_cpu_huge_value_returns_none_instead_of_overflow() {
1315+
// A finite f64 whose product with 100_000 overflows to infinity.
1316+
assert_eq!(parse_cpu_to_microseconds("1e300"), None);
1317+
}
1318+
1319+
#[test]
1320+
fn parse_cpu_rejects_u64_max_boundary() {
1321+
// 184_467_440_737_095.51616 cores * 100_000 rounds exactly to 2^64,
1322+
// which used to pass the > check and silently saturate to u64::MAX.
1323+
// It must now be rejected. The previous value (184467440737095520)
1324+
// was ~1000x larger and did not exercise the equality boundary.
1325+
assert_eq!(parse_cpu_to_microseconds("184467440737095.51616"), None);
1326+
1327+
// Just below the boundary is still valid.
1328+
assert_eq!(
1329+
parse_cpu_to_microseconds("184467440737095"),
1330+
Some(18_446_744_073_709_500_416)
1331+
);
1332+
}
1333+
1334+
#[test]
1335+
fn container_spec_rejects_health_check_interval_overflow() {
1336+
let sandbox = test_sandbox("test-id", "test-name");
1337+
let mut config = test_config();
1338+
// i64::MAX nanoseconds / 1_000_000_000 ns/s = 9_223_372_036 seconds.
1339+
// One second over must be rejected before it can saturate.
1340+
config.health_check_interval_secs = 9_223_372_037;
1341+
let err = try_build_container_spec_with_token(&sandbox, &config, None).unwrap_err();
1342+
assert!(
1343+
matches!(err, ComputeDriverError::InvalidArgument(_)),
1344+
"expected InvalidArgument, got {err:?}"
1345+
);
1346+
assert!(format!("{err}").contains("health_check_interval_secs"));
1347+
}
1348+
1349+
#[test]
1350+
fn container_spec_accepts_health_check_interval_at_boundary() {
1351+
let sandbox = test_sandbox("test-id", "test-name");
1352+
let mut config = test_config();
1353+
// i64::MAX nanoseconds / 1_000_000_000 ns/s = 9_223_372_036 seconds.
1354+
const NS_PER_S: u64 = 1_000_000_000;
1355+
config.health_check_interval_secs = (i64::MAX as u64) / NS_PER_S;
1356+
let spec = try_build_container_spec_with_token(&sandbox, &config, None).unwrap();
1357+
let interval = spec["healthconfig"]["Interval"]
1358+
.as_u64()
1359+
.expect("healthcheck interval should be a u64");
1360+
assert_eq!(interval, config.health_check_interval_secs * NS_PER_S);
1361+
}
1362+
12901363
#[test]
12911364
fn parse_memory_binary_suffixes() {
12921365
assert_eq!(parse_memory_to_bytes("256Mi"), Some(256 * 1024 * 1024));

crates/openshell-server/src/grpc/sandbox.rs

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1522,7 +1522,14 @@ pub(super) async fn handle_create_ssh_session(
15221522
let token = uuid::Uuid::new_v4().to_string();
15231523
let now_ms = current_time_ms();
15241524
let expires_at_ms = if state.config.ssh_session_ttl_secs > 0 {
1525-
now_ms + (state.config.ssh_session_ttl_secs as i64 * 1000)
1525+
let ttl_secs = state.config.ssh_session_ttl_secs;
1526+
let ttl_ms = i64::try_from(ttl_secs)
1527+
.map_err(|_| Status::invalid_argument("ssh_session_ttl_secs is too large"))?
1528+
.checked_mul(1000)
1529+
.ok_or_else(|| Status::invalid_argument("ssh_session_ttl_secs is too large"))?;
1530+
now_ms
1531+
.checked_add(ttl_ms)
1532+
.ok_or_else(|| Status::invalid_argument("ssh_session_ttl_secs is too large"))?
15261533
} else {
15271534
0
15281535
};
@@ -4382,4 +4389,32 @@ mod tests {
43824389
assert!(session.revoked);
43834390
assert_eq!(session.object_workspace(), "default");
43844391
}
4392+
4393+
#[tokio::test]
4394+
async fn create_ssh_session_rejects_too_large_ttl() {
4395+
let mut state = test_server_state().await;
4396+
state
4397+
.store
4398+
.put_message(&test_sandbox("ttl-test", Vec::new()))
4399+
.await
4400+
.unwrap();
4401+
4402+
// Any value larger than i64::MAX seconds cannot be converted to
4403+
// milliseconds without overflowing.
4404+
Arc::get_mut(&mut state)
4405+
.expect("fresh test state is uniquely held")
4406+
.config
4407+
.ssh_session_ttl_secs = u64::MAX;
4408+
4409+
let err = handle_create_ssh_session(
4410+
&state,
4411+
Request::new(CreateSshSessionRequest {
4412+
sandbox_id: "sandbox-ttl-test".to_string(),
4413+
}),
4414+
)
4415+
.await
4416+
.unwrap_err();
4417+
assert_eq!(err.code(), tonic::Code::InvalidArgument);
4418+
assert!(err.message().contains("ssh_session_ttl_secs is too large"));
4419+
}
43854420
}

crates/openshell-tui/src/lib.rs

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2698,11 +2698,10 @@ fn format_age(epoch_ms: i64) -> String {
26982698
let now = std::time::SystemTime::now()
26992699
.duration_since(std::time::UNIX_EPOCH)
27002700
.map_or(0, |d| d.as_secs().cast_signed());
2701-
let diff = now - created_secs;
2702-
if diff < 0 {
2701+
if created_secs > now {
27032702
return String::from("-");
27042703
}
2705-
let diff = diff.cast_unsigned();
2704+
let diff = (now - created_secs).cast_unsigned();
27062705
if diff < 60 {
27072706
format!("{diff}s")
27082707
} else if diff < 3600 {
@@ -2805,3 +2804,31 @@ mod provider_profile_workspace_tests {
28052804
}
28062805
}
28072806
}
2807+
2808+
// ---------------------------------------------------------------------------
2809+
// Tests
2810+
// ---------------------------------------------------------------------------
2811+
2812+
#[cfg(test)]
2813+
mod tests {
2814+
use super::*;
2815+
2816+
#[test]
2817+
fn format_age_handles_future_timestamp() {
2818+
let future_ms = i64::try_from(
2819+
std::time::SystemTime::now()
2820+
.duration_since(std::time::UNIX_EPOCH)
2821+
.unwrap()
2822+
.as_millis(),
2823+
)
2824+
.unwrap()
2825+
+ 10_000;
2826+
assert_eq!(format_age(future_ms), "-");
2827+
}
2828+
2829+
#[test]
2830+
fn format_age_handles_zero_and_negative() {
2831+
assert_eq!(format_age(0), "-");
2832+
assert_eq!(format_age(-1), "-");
2833+
}
2834+
}

0 commit comments

Comments
 (0)