Skip to content

Commit da237ad

Browse files
lukekimclaudeCopilot
authored
fix(security): close API key timing-position leak and remote-UDF SSRF (spiceai#10757)
* docs: add product security audit report Workspace-wide audit of auth, secrets, HTTP/Flight surface, AI/LLM tooling, data connectors, and the container/build pipeline. Two critical findings (non-constant-time API key compare; SSRF in remote UDF endpoint validation), four high (MySQL preferred-mode TLS bypass, Vault tls_skip_verify on remote hosts, AWS default credential chain unverified at init, no auth-failure rate limiting), and a set of medium/low hardening items. Each finding cites file:line and a fix. https://claude.ai/code/session_01Xu4b9TS1UvGu4H1a7NxHKj * fix(security): address critical findings from security audit (C1, C2) C1 — runtime-auth API key allowlist iteration The per-comparison code was already constant-time: ApiKey's PartialEq<str> uses subtle::ct_eq, so per-byte timing does not leak key bytes. However, HTTP, Flight basic, Flight bearer, and gRPC paths all used find / any to search the configured key list, which short-circuits on the first matching key. That leaks the *position* of the matching key (1 comparison vs N) and the existence of any match. Replace find / any with a lookup helper that iterates over every configured key on every call without short-circuiting; total verify time is O(N) regardless of which key (if any) matched. Add tests covering matches at first / middle / last positions and empty-credential rejection on the Flight basic-auth path. C2 — remote UDF endpoint SSRF parse_endpoint previously validated only the URL scheme; any operator- or LLM-influenced UDF declaration could target loopback, RFC1918, or the cloud metadata service via 169.254.169.254 — a practical IAM credential exfiltration path on any cloud deployment. parse_endpoint now classifies the host: * IPv4 literals are rejected if loopback, RFC1918, link-local (covers AWS / GCP / Azure IMDS), multicast, broadcast, unspecified, or carrier-grade NAT (100.64.0.0/10). * IPv6 literals are rejected if loopback, unspecified, multicast, link-local (fe80::/10), unique-local (fc00::/7, covers fd00:ec2::254), or IPv4-mapped onto any of the above ranges. * A short list of conventional metadata hostnames (localhost, metadata, metadata.google.internal) is also blocked. Arbitrary hostnames are still allowed; resolving DNS at builder time introduces a TOCTOU/DNS-rebinding hole, so connect-time enforcement is tracked as follow-up work. * A new allow_private_endpoints: true param opts back in for trusted on-host services. Documented in the file header. Tests cover the default rejection across 12 representative private addresses (loopback, RFC1918, IMDS, IPv6 ULA, IPv4-mapped IPv6, etc.), the explicit opt-in path, public-address acceptance, and an invalid-opt-in-value rejection. Existing round-trip tests that bind a mock HTTP server on 127.0.0.1 are routed through a new with_loopback_optin helper so they keep working. Also update docs/security_audit.md to reflect what was fixed and correct the earlier claim that the per-comparison code was not constant-time. https://claude.ai/code/session_01Xu4b9TS1UvGu4H1a7NxHKj * docs: remove security audit report The audit report was a working artifact; the actionable items live in the code changes (constant-time-position API key lookup, remote-UDF SSRF allowlist) plus their tests. Remaining findings can be tracked as issues rather than as in-tree documentation. https://claude.ai/code/session_01Xu4b9TS1UvGu4H1a7NxHKj * fix: address security PR review feedback * fix: improve security checks for remote UDF endpoints and refactor IP validation logic * fix: resolve ownership issues in IP validation logic for disallowed addresses * fix: use CIDR allowlist for remote UDF endpoints * fix: address final security review comments * fix(security): reject pickle model weights by default; enforce RUSTSEC advisories in CI Two follow-ups motivated by CVE-2026-7482 (Ollama heap OOB read in GGUF tensor parsing). The CVE itself does not affect this codebase — Ollama is not a dependency — but the bug class (malformed binary blob handed to a complex parser) does apply to several places spiceai consumes. 1. Pickle weight rejection -------------------------------------------------------------------- The .pt / .pth / .ckpt / .bin extensions are conventionally Python pickle in PyTorch's ecosystem, and pickle deserialization is RCE by design. The local-LLM file pipeline previously accepted whatever the operator pointed at and handed it straight to mistralrs's loader. Operators commonly download model weights from third-party Hugging Face repos, so a malicious .bin in such a repo would have run arbitrary Rust process code at load time. Add `llms::chat::reject_unsafe_weight_formats` and call it from `runtime::model::chat::file` before invoking `create_local_model`. The check refuses pickle-class extensions by default and accepts a new `params.trust_pickle: true` opt-in for operators who control the source. The check is case-insensitive and covers the four conventional pickle extensions. Five unit tests cover rejection, acceptance of safe formats, opt-in, case-insensitivity, and extensionless paths. 2. Enforce RUSTSEC advisories in CI -------------------------------------------------------------------- The `[advisories]` block in deny.toml was schema-pre-v2 and CI only ran `cargo deny check licenses`. Upgrade the block to `version = 2` (cargo-deny v2 default semantics: vulnerability / unmaintained / unsound / notice all fail; yanked already explicitly denied), and add a second step in `license_check.yml` that runs `cargo deny check advisories`. This intentionally turns on enforcement without pre-populating `ignore` — the workspace currently has known findings (yanked crates, unmaintained warnings, and a small number of actual CVEs in transitive deps) that the team will want to triage explicitly rather than silently bypass. https://claude.ai/code/session_01Xu4b9TS1UvGu4H1a7NxHKj * fix: tighten remote UDF endpoint allowlist handling * fix: correct typo in comment regarding PyTorch pickle file extensions * fix: update dependencies in Cargo.lock and enhance ApiKeyAuth implementation * fix: ungate Path import so reject_unsafe_weight_formats builds without local_llm reject_unsafe_weight_formats and the in-module tests use Path / PathBuf unconditionally, but the imports were gated behind feature = "local_llm". The runtime integration test build (no local_llm feature) failed with E0425 "cannot find type `Path` in this scope". Ungate Path, gate PathBuf to local_llm or test so it isn't unused in lib builds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: downgrade getrandom dependency from 0.4.1 to 0.3.4 * fix: filter remote UDF DNS results * fix: update cargo-deny installation command and improve IP address checks for non-public endpoints * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: specify cargo-deny version and improve error messages in remote functions * fix: update global IP address check to allow 192.0.0.0/24 range --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 837c102 commit da237ad

7 files changed

Lines changed: 917 additions & 85 deletions

File tree

.github/workflows/license_check.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ jobs:
5252
uses: ./.github/actions/setup-rust
5353

5454
- name: Install cargo-deny
55-
run: cargo install cargo-deny || true
55+
run: cargo install cargo-deny --version 0.19.6 --locked --force
5656

5757
- name: Check Rust Licenses
5858
run: cargo deny check licenses
59+
60+
- name: Check Rust Advisories
61+
run: cargo deny check advisories

Cargo.lock

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

crates/llms/src/chat/mod.rs

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,8 @@ use secrecy::SecretString;
2424
use snafu::ResultExt;
2525
use snafu::Snafu;
2626
use spicepod::component::model::ModelSource;
27-
#[cfg(feature = "local_llm")]
2827
use std::path::Path;
29-
#[cfg(feature = "local_llm")]
28+
#[cfg(any(feature = "local_llm", test))]
3029
use std::path::PathBuf;
3130
use std::pin::Pin;
3231
#[cfg(feature = "local_llm")]
@@ -157,6 +156,14 @@ pub enum Error {
157156
))]
158157
ModelFileMissing { file_url: String },
159158

159+
#[snafu(display(
160+
"Refusing to load model weight file '{path}': '.{extension}' is a Python pickle format \
161+
that executes arbitrary code on load (CVE-class: untrusted pickle deserialization → RCE). \
162+
Convert the model to `.safetensors` or `.gguf`, or set `params.trust_pickle: true` if the \
163+
file source is fully trusted."
164+
))]
165+
UnsafePickleWeight { path: String, extension: String },
166+
160167
#[snafu(display(
161168
"Invalid parameters for model '{model}': {source} Verify the model parameters, and try again."
162169
))]
@@ -759,3 +766,91 @@ pub async fn create_local_model(
759766
.await
760767
.map(|x| Arc::new(x) as Arc<dyn Chat>)
761768
}
769+
770+
/// File extensions that are conventionally Python pickle by `PyTorch`'s
771+
/// ecosystem and that are unsafe to load from any source the operator
772+
/// does not fully trust. Pickle deserialization is RCE by design.
773+
const PICKLE_WEIGHT_EXTENSIONS: &[&str] = &["bin", "pt", "pth", "ckpt"];
774+
775+
/// Reject any weight path whose extension lands in [`PICKLE_WEIGHT_EXTENSIONS`]
776+
/// unless the caller has explicitly opted in via `trust_pickle = true`.
777+
///
778+
/// `weights` is expected to be the same slice of paths that will be handed
779+
/// to the model loader. Returns [`Error::UnsafePickleWeight`] on the first
780+
/// match, so the message identifies the offending file.
781+
///
782+
/// # Errors
783+
///
784+
/// Returns [`Error::UnsafePickleWeight`] when `trust_pickle` is `false`
785+
/// and any path has a pickle-class extension.
786+
pub fn reject_unsafe_weight_formats<P: AsRef<Path>>(
787+
weights: &[P],
788+
trust_pickle: bool,
789+
) -> Result<()> {
790+
if trust_pickle {
791+
return Ok(());
792+
}
793+
for w in weights {
794+
let path = w.as_ref();
795+
if let Some(ext) = path.extension().and_then(|e| e.to_str()) {
796+
let ext = ext.to_ascii_lowercase();
797+
if PICKLE_WEIGHT_EXTENSIONS.contains(&ext.as_str()) {
798+
return Err(Error::UnsafePickleWeight {
799+
path: path.to_string_lossy().into_owned(),
800+
extension: ext,
801+
});
802+
}
803+
}
804+
}
805+
Ok(())
806+
}
807+
808+
#[cfg(test)]
809+
mod tests {
810+
use super::*;
811+
812+
#[test]
813+
fn rejects_pickle_extensions_by_default() {
814+
for ext in PICKLE_WEIGHT_EXTENSIONS {
815+
let path = PathBuf::from(format!("/models/pytorch_model.{ext}"));
816+
let err = reject_unsafe_weight_formats(&[path], false)
817+
.expect_err(&format!("expected rejection for .{ext}"));
818+
assert!(matches!(err, Error::UnsafePickleWeight { .. }));
819+
}
820+
}
821+
822+
#[test]
823+
fn allows_safe_extensions() {
824+
for ext in ["safetensors", "gguf", "ggml", "onnx"] {
825+
let path = PathBuf::from(format!("/models/weights.{ext}"));
826+
reject_unsafe_weight_formats(&[path], false)
827+
.unwrap_or_else(|e| panic!("expected ok for .{ext}, got {e:?}"));
828+
}
829+
}
830+
831+
#[test]
832+
fn opt_in_allows_pickle_extensions() {
833+
let paths = [
834+
PathBuf::from("/m/a.pt"),
835+
PathBuf::from("/m/pytorch_model.bin"),
836+
];
837+
reject_unsafe_weight_formats(&paths, true).expect("opt-in should allow pickle");
838+
}
839+
840+
#[test]
841+
fn extension_check_is_case_insensitive() {
842+
let path = PathBuf::from("/m/Pytorch_Model.PT");
843+
let err = reject_unsafe_weight_formats(&[path], false)
844+
.expect_err("case-insensitive .PT must be rejected");
845+
match err {
846+
Error::UnsafePickleWeight { extension, .. } => assert_eq!(extension, "pt"),
847+
other => panic!("unexpected error: {other:?}"),
848+
}
849+
}
850+
851+
#[test]
852+
fn extensionless_path_is_ignored() {
853+
let path = PathBuf::from("/m/no_extension_here");
854+
reject_unsafe_weight_formats(&[path], false).expect("no extension → no rejection");
855+
}
856+
}

0 commit comments

Comments
 (0)