undf: assign 954-955; veloren-0001 TradePricing O(N^2) CWE-407, veloren-0002 auth token logged CWE-312
This commit is contained in:
parent
dafb9a87ec
commit
b7340f0b9b
5 changed files with 349 additions and 1 deletions
|
|
@ -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"
|
||||
}
|
||||
|
|
|
|||
135
defects/veloren-0001/patch/veloren-0001.patch
Normal file
135
defects/veloren-0001/patch/veloren-0001.patch
Normal file
|
|
@ -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<ItemDefinitionIdOwned, usize>,
|
||||
equality_set: EqualitySet,
|
||||
}
|
||||
|
||||
@@ -165,17 +166,21 @@ struct FreqEntry {
|
||||
|
||||
#[derive(Default, Debug)]
|
||||
struct PriceEntries(Vec<PriceEntry>);
|
||||
+#[derive(Default, Debug)]
|
||||
+struct PriceIndex(HashMap<ItemDefinitionIdOwned, usize>);
|
||||
#[derive(Default, Debug)]
|
||||
struct FreqEntries(Vec<FreqEntry>);
|
||||
+#[derive(Default, Debug)]
|
||||
+struct FreqIndex(HashMap<ItemDefinitionIdOwned, usize>);
|
||||
|
||||
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::<TradingPriceFile>::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
|
||||
}
|
||||
112
defects/veloren-0001/test/veloren-0001-test.rs
Normal file
112
defects/veloren-0001/test/veloren-0001-test.rs
Normal file
|
|
@ -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<ItemDefinitionIdOwned, usize> 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<String, usize>,
|
||||
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<String> = (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");
|
||||
}
|
||||
31
defects/veloren-0002/patch/veloren-0002.patch
Normal file
31
defects/veloren-0002/patch/veloren-0002.patch
Normal file
|
|
@ -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<AuthClient>,
|
||||
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!"
|
||||
);
|
||||
68
defects/veloren-0002/test/veloren-0002-test.py
Normal file
68
defects/veloren-0002/test/veloren-0002-test.py
Normal file
|
|
@ -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)")
|
||||
Loading…
Add table
Add a link
Reference in a new issue