From b7340f0b9ba87961bc03db74474c33c6645f973e Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Tue, 31 Mar 2026 11:48:12 -0400 Subject: [PATCH] undf: assign 954-955; veloren-0001 TradePricing O(N^2) CWE-407, veloren-0002 auth token logged CWE-312 --- UNDF-REGISTRY.json | 4 +- defects/veloren-0001/patch/veloren-0001.patch | 135 ++++++++++++++++++ .../veloren-0001/test/veloren-0001-test.rs | 112 +++++++++++++++ defects/veloren-0002/patch/veloren-0002.patch | 31 ++++ .../veloren-0002/test/veloren-0002-test.py | 68 +++++++++ 5 files changed, 349 insertions(+), 1 deletion(-) create mode 100644 defects/veloren-0001/patch/veloren-0001.patch create mode 100644 defects/veloren-0001/test/veloren-0001-test.rs create mode 100644 defects/veloren-0002/patch/veloren-0002.patch create mode 100644 defects/veloren-0002/test/veloren-0002-test.py diff --git a/UNDF-REGISTRY.json b/UNDF-REGISTRY.json index e267fbfca..7c6a78a07 100644 --- a/UNDF-REGISTRY.json +++ b/UNDF-REGISTRY.json @@ -951,5 +951,7 @@ "tiled-0002-0002": "UNDF-2026-000000950", "tiled-0003-0003": "UNDF-2026-000000951", "jellyfin-0001-0001": "UNDF-2026-000000952", - "jellyfin-0002-0002": "UNDF-2026-000000953" + "jellyfin-0002-0002": "UNDF-2026-000000953", + "veloren-0001-0001": "UNDF-2026-000000954", + "veloren-0002-0002": "UNDF-2026-000000955" } diff --git a/defects/veloren-0001/patch/veloren-0001.patch b/defects/veloren-0001/patch/veloren-0001.patch new file mode 100644 index 000000000..445bc83bc --- /dev/null +++ b/defects/veloren-0001/patch/veloren-0001.patch @@ -0,0 +1,135 @@ +# UNDF: UNDF-2026-000000954 +--- a/common/src/comp/inventory/trade_pricing.rs ++++ b/common/src/comp/inventory/trade_pricing.rs +@@ -28,6 +28,7 @@ const PRICING_DEBUG: bool = false; + #[derive(Default, Debug)] + pub struct TradePricing { + items: PriceEntries, ++ items_index: HashMap, + equality_set: EqualitySet, + } + +@@ -165,17 +166,21 @@ struct FreqEntry { + + #[derive(Default, Debug)] + struct PriceEntries(Vec); ++#[derive(Default, Debug)] ++struct PriceIndex(HashMap); + #[derive(Default, Debug)] + struct FreqEntries(Vec); ++#[derive(Default, Debug)] ++struct FreqIndex(HashMap); + + impl PriceEntries { +- fn add_alternative(&mut self, b: PriceEntry) { ++ fn add_alternative(&mut self, index: &mut PriceIndex, b: PriceEntry) { + // alternatives are added in frequency (gets more frequent) +- let already = self.0.iter_mut().find(|i| i.name == b.name); +- if let Some(entry) = already { ++ if let Some(&idx) = index.0.get(&b.name) { ++ let entry = &mut self.0[idx]; + let entry_freq: MaterialFrequency = std::mem::take(&mut entry.price).into(); + let b_freq: MaterialFrequency = b.price.into(); + let result = entry_freq + b_freq; + entry.price = result.into(); + } else { ++ index.0.insert(b.name.clone(), self.0.len()); + self.0.push(b); + } + } +@@ -183,16 +188,14 @@ impl PriceEntries { + + impl FreqEntries { + fn add( + &mut self, ++ index: &mut FreqIndex, + eqset: &EqualitySet, + item_name: &ItemDefinitionIdOwned, + good: Good, + probability: f32, + can_sell: bool, + ) { + let canonical_itemname = eqset.canonical(item_name); +- let old = self +- .0 +- .iter_mut() +- .find(|elem| elem.name == *canonical_itemname); ++ let old_idx = index.0.get(canonical_itemname).copied(); + let new_freq = MaterialFrequency(vec![(probability, good)]); + // Increase probability if already in entries, or add new entry +- if let Some(FreqEntry { ++ if let Some(FreqEntry { + name: asset, + freq: old_probability, + sell: old_can_sell, + stackable: _, +- }) = old ++ }) = old_idx.map(|i| &mut self.0[i]) + { + if PRICING_DEBUG { + info!("Update {:?} {:?}+{:?}", asset, old_probability, probability); +@@ -218,6 +221,7 @@ impl FreqEntries { + if PRICING_DEBUG { + info!("New {:?}", new_mat_prob); + } ++ index.0.insert(canonical_itemname.to_owned(), self.0.len()); + self.0.push(new_mat_prob); + } + +@@ -225,6 +229,7 @@ impl FreqEntries { + // It will have infinity as its price, but it's fine, + // because we determine all prices based on canonical value + if canonical_itemname != item_name && !self.0.iter().any(|elem| elem.name == *item_name) { ++ index.0.insert(item_name.to_owned(), self.0.len()); + self.0.push(FreqEntry { + name: item_name.to_owned(), + freq: Default::default(), +@@ -674,10 +679,10 @@ impl TradePricing { + // look up price (inverse frequency) of an item + fn price_lookup(&self, requested_name: &ItemDefinitionIdOwned) -> Option<&MaterialUse> { + let canonical_name = self.equality_set.canonical(requested_name); +- self.items +- .0 +- .iter() +- .find(|e| &e.name == canonical_name) ++ self.items_index ++ .get(canonical_name) ++ .and_then(|&idx| self.items.0.get(idx)) ++ .filter(|e| &e.name == canonical_name) + .map(|e| &e.price) + } + +@@ -740,8 +745,10 @@ impl TradePricing { + fn read() -> Self { + let mut result = Self::default(); + let mut freq = FreqEntries::default(); ++ let mut freq_index = FreqIndex::default(); + let price_config = + Ron::::load_expect("common.trading.item_price_calculation").read(); ++ let mut price_index = PriceIndex::default(); + result.equality_set = EqualitySet::load_expect("common.trading.item_price_equality") + .read() + .clone(); +@@ -757,6 +764,7 @@ impl TradePricing { + freq.add( ++ &mut freq_index, + &result.equality_set, + item_asset, + good, +@@ -766,6 +774,7 @@ impl TradePricing { + } + } + freq.add( ++ &mut freq_index, + &result.equality_set, + &ItemDefinitionIdOwned::Simple(Self::COIN_ITEM.into()), + Good::Coin, +@@ -937,7 +946,7 @@ impl TradePricing { + result.items.add_alternative(new_entry); ++ result.items.add_alternative(&mut price_index, new_entry); + } else { + error!("Recipe {:?} incomplete confusion", recipe); + } ++ result.items_index = price_index.0; + result + } diff --git a/defects/veloren-0001/test/veloren-0001-test.rs b/defects/veloren-0001/test/veloren-0001-test.rs new file mode 100644 index 000000000..bb4feb310 --- /dev/null +++ b/defects/veloren-0001/test/veloren-0001-test.rs @@ -0,0 +1,112 @@ +// Unit test for veloren-0001: TradePricing O(N^2) linear scan in PriceEntries/FreqEntries +// +// DEFECT: PriceEntries::add_alternative() and FreqEntries::add() use +// Vec::iter().find() to check for duplicate items before inserting. +// With N = number of unique items (1300+ in Veloren), initialization +// performs O(N^2) linear scans. +// +// FIX: Add HashMap index alongside Vec +// for O(1) lookup by item name during dedup. + +use std::collections::HashMap; +use std::time::Instant; + +/// Simulates the defective pattern: Vec linear scan for dedup +fn vec_dedup_add(entries: &mut Vec<(String, f32)>, name: String, value: f32) { + if let Some(entry) = entries.iter_mut().find(|(n, _)| *n == name) { + entry.1 += value; + } else { + entries.push((name, value)); + } +} + +/// Simulates the fixed pattern: HashMap index for O(1) dedup +fn hashmap_dedup_add( + entries: &mut Vec<(String, f32)>, + index: &mut HashMap, + name: String, + value: f32, +) { + if let Some(&idx) = index.get(&name) { + entries[idx].1 += value; + } else { + index.insert(name.clone(), entries.len()); + entries.push((name, value)); + } +} + +#[test] +fn test_correctness() { + // Both patterns should produce identical results + let items: Vec<(String, f32)> = (0..100) + .map(|i| (format!("item_{}", i % 50), i as f32)) + .collect(); + + let mut vec_entries = Vec::new(); + for (name, value) in items.iter() { + vec_dedup_add(&mut vec_entries, name.clone(), *value); + } + + let mut hm_entries = Vec::new(); + let mut hm_index = HashMap::new(); + for (name, value) in items.iter() { + hashmap_dedup_add(&mut hm_entries, &mut hm_index, name.clone(), *value); + } + + assert_eq!(vec_entries.len(), hm_entries.len()); + for (v, h) in vec_entries.iter().zip(hm_entries.iter()) { + assert_eq!(v.0, h.0); + assert!((v.1 - h.1).abs() < f32::EPSILON); + } +} + +#[test] +fn test_performance_ratio() { + // Simulate N=1300 unique items (Veloren item count) with repeated adds + let n = 1300; + let names: Vec = (0..n).map(|i| format!("common.items.item_{}", i)).collect(); + + // Defective: Vec linear scan + let start = Instant::now(); + let mut vec_entries = Vec::new(); + for name in names.iter() { + vec_dedup_add(&mut vec_entries, name.clone(), 1.0); + } + // Second pass (simulating recipe processing) + for name in names.iter() { + vec_dedup_add(&mut vec_entries, name.clone(), 0.5); + } + let vec_time = start.elapsed(); + + // Fixed: HashMap index + let start = Instant::now(); + let mut hm_entries = Vec::new(); + let mut hm_index = HashMap::new(); + for name in names.iter() { + hashmap_dedup_add(&mut hm_entries, &mut hm_index, name.clone(), 1.0); + } + for name in names.iter() { + hashmap_dedup_add(&mut hm_entries, &mut hm_index, name.clone(), 0.5); + } + let hm_time = start.elapsed(); + + let ratio = vec_time.as_nanos() as f64 / hm_time.as_nanos().max(1) as f64; + println!( + "veloren-0001: Vec O(N^2) = {:?}, HashMap O(N) = {:?}, ratio = {:.1}x", + vec_time, hm_time, ratio + ); + + // Vec scan should be measurably slower at N=1300 + assert!( + ratio > 2.0, + "Expected significant speedup with HashMap index, got ratio {:.1}x", + ratio + ); + assert_eq!(vec_entries.len(), hm_entries.len()); +} + +fn main() { + test_correctness(); + test_performance_ratio(); + println!("PASS"); +} diff --git a/defects/veloren-0002/patch/veloren-0002.patch b/defects/veloren-0002/patch/veloren-0002.patch new file mode 100644 index 000000000..40793c501 --- /dev/null +++ b/defects/veloren-0002/patch/veloren-0002.patch @@ -0,0 +1,31 @@ +# UNDF: UNDF-2026-000000955 +--- a/server/src/login_provider.rs ++++ b/server/src/login_provider.rs +@@ -185,7 +185,7 @@ impl LoginProvider { + srv: Arc, + username_or_token: &str, + ) -> Result<(String, Uuid), RegisterError> { +- info!(?username_or_token, "Validating token"); ++ info!(token_len = username_or_token.len(), "Validating token"); + // Parse token + let token = AuthToken::from_str(username_or_token) + .map_err(|e| RegisterError::AuthError(e.to_string()))?; +--- a/server/src/sys/msg/register.rs ++++ b/server/src/sys/msg/register.rs +@@ -137,7 +137,7 @@ impl<'a> System<'a> for Sys { + + let _ = super::try_recv_all(client, 0, |_, msg: ClientRegister| { +- trace!(?msg.token_or_username, "defer auth lockup"); ++ trace!(token_len = msg.token_or_username.len(), "defer auth lockup"); + let pending = read_data.login_provider.verify(&msg.token_or_username); + locale = msg.locale; + let _ = pending_logins.insert(entity, pending); +--- a/network/src/scheduler.rs ++++ b/network/src/scheduler.rs +@@ -480,7 +480,6 @@ impl Scheduler { + warn!( + ?cid, + ?pid, +- ?secret, + "Detected incompatible Secret!, this is probably an attack!" + ); diff --git a/defects/veloren-0002/test/veloren-0002-test.py b/defects/veloren-0002/test/veloren-0002-test.py new file mode 100644 index 000000000..dc1a70457 --- /dev/null +++ b/defects/veloren-0002/test/veloren-0002-test.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 +""" +Unit test for veloren-0002: Auth token and network secret logged verbatim (CWE-312) + +DEFECT: Three logging sites expose credentials: + 1. server/src/login_provider.rs:188 - logs username_or_token at info! level + 2. server/src/sys/msg/register.rs:140 - logs token_or_username at trace! level + 3. network/src/scheduler.rs:483 - logs network secret (u128) at warn! level + +FIX: Replace credential values with metadata (token_len) and remove secret +from attack detection log. +""" + +import re +import os +import sys + +VELOREN_ROOT = os.path.expanduser("~/git/veloren") + +def read_file(path): + with open(os.path.join(VELOREN_ROOT, path), "r") as f: + return f.read() + + +def test_login_provider_logs_token(): + """login_provider.rs should NOT log the raw token value.""" + src = read_file("server/src/login_provider.rs") + # The defective line logs ?username_or_token which prints the full token + matches = re.findall(r'info!\(.*\?username_or_token.*"Validating token"', src) + assert len(matches) > 0, ( + "DEFECT PRESENT: login_provider.rs logs raw auth token at info! level " + "(line ~188: info!(?username_or_token, \"Validating token\"))" + ) + print("PASS login_provider: defect confirmed (raw token logged at info! level)") + + +def test_register_logs_token(): + """register.rs should NOT log the raw token value.""" + src = read_file("server/src/sys/msg/register.rs") + matches = re.findall(r'trace!\(.*\?msg\.token_or_username.*"defer auth lockup"', src) + assert len(matches) > 0, ( + "DEFECT PRESENT: register.rs logs raw token at trace! level " + "(line ~140: trace!(?msg.token_or_username, \"defer auth lockup\"))" + ) + print("PASS register: defect confirmed (raw token logged at trace! level)") + + +def test_scheduler_logs_secret(): + """scheduler.rs should NOT log the network secret.""" + src = read_file("network/src/scheduler.rs") + # Find the warn! block that logs ?secret + matches = re.findall(r'warn!\([^)]*\?secret[^)]*"Detected incompatible Secret', src, re.DOTALL) + assert len(matches) > 0, ( + "DEFECT PRESENT: scheduler.rs logs network secret at warn! level " + "(line ~483: ?secret in warn! macro)" + ) + print("PASS scheduler: defect confirmed (network secret logged at warn! level)") + + +if __name__ == "__main__": + if not os.path.isdir(VELOREN_ROOT): + print(f"SKIP: {VELOREN_ROOT} not found") + sys.exit(0) + + test_login_provider_logs_token() + test_register_logs_token() + test_scheduler_logs_secret() + print("PASS (3/3 defect sites confirmed)")