Skip to content

Commit a509da1

Browse files
author
audrius
committed
Mapping hash uses fixed DefaultHasher enabling hash-flooding DoS
1 parent 7ea12ec commit a509da1

1 file changed

Lines changed: 125 additions & 88 deletions

File tree

‎src/mapping.rs‎

Lines changed: 125 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
//! A YAML mapping and its iterator types.
22
33
use crate::duplicate_key::DuplicateKeyError;
4-
use crate::{private, Value};
4+
use crate::{Value, private};
55
use indexmap::IndexMap;
66
use serde::{Deserialize, Deserializer, Serialize};
77
use std::cmp::Ordering;
8-
use std::collections::hash_map::DefaultHasher;
98
use std::fmt;
109
use std::hash::{Hash, Hasher};
1110
use std::mem;
@@ -107,11 +106,8 @@ impl Mapping {
107106

108107
/// Convenience method, as key is most often as string and the key itself
109108
/// normally has no anchor, value does.
110-
pub fn set(&mut self, key: &str, value: Value) {
111-
self.insert(
112-
Value::String(key.to_string(), None),
113-
value,
114-
);
109+
pub fn set(&mut self, key: &str, value: Value) {
110+
self.insert(Value::String(key.to_string(), None), value);
115111
}
116112

117113
/// Checks if the map contains the given key.
@@ -444,101 +440,102 @@ where
444440
impl Hash for Mapping {
445441
fn hash<H: Hasher>(&self, state: &mut H) {
446442
// Hash the kv pairs in a way that is not sensitive to their order.
447-
let mut xor = 0;
448-
for (k, v) in self {
449-
let mut hasher = DefaultHasher::new();
450-
k.hash(&mut hasher);
451-
v.hash(&mut hasher);
452-
xor ^= hasher.finish();
443+
//
444+
// We still feed key/value hashes into `state` directly so that callers'
445+
// hash randomization remains effective.
446+
let mut entries = self.iter().collect::<Vec<_>>();
447+
entries.sort_by(|(ak, av), (bk, bv)| {
448+
total_cmp_value(ak, bk).then_with(|| total_cmp_value(av, bv))
449+
});
450+
for (k, v) in entries {
451+
k.hash(state);
452+
v.hash(state);
453453
}
454-
xor.hash(state);
455454
}
456455
}
457456

458-
impl PartialOrd for Mapping {
459-
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
460-
let mut self_entries = self.iter().collect::<Vec<_>>();
461-
let mut other_entries = other.iter().collect::<Vec<_>>();
457+
fn total_cmp_value(a: &Value, b: &Value) -> Ordering {
458+
match (a, b) {
459+
(Value::Null(_), Value::Null(_)) => Ordering::Equal,
460+
(Value::Null(_), _) => Ordering::Less,
461+
(_, Value::Null(_)) => Ordering::Greater,
462462

463-
// Sort in an arbitrary order that is consistent with Value's PartialOrd
464-
// impl.
465-
fn total_cmp(a: &Value, b: &Value) -> Ordering {
466-
match (a, b) {
467-
(Value::Null(_), Value::Null(_)) => Ordering::Equal,
468-
(Value::Null(_), _) => Ordering::Less,
469-
(_, Value::Null(_)) => Ordering::Greater,
470-
471-
(Value::Bool(a, _), Value::Bool(b, _)) => a.cmp(b),
472-
(Value::Bool(_, _), _) => Ordering::Less,
473-
(_, Value::Bool(_, _)) => Ordering::Greater,
474-
475-
(Value::Number(a, _), Value::Number(b, _)) => a.total_cmp(b),
476-
(Value::Number(_, _), _) => Ordering::Less,
477-
(_, Value::Number(_, _)) => Ordering::Greater,
478-
479-
(Value::String(a, _), Value::String(b, _)) => a.cmp(b),
480-
(Value::String(_, _), _) => Ordering::Less,
481-
(_, Value::String(_, _)) => Ordering::Greater,
482-
483-
(Value::Sequence(a), Value::Sequence(b)) => iter_cmp_by(a, b, total_cmp),
484-
(Value::Sequence(_), _) => Ordering::Less,
485-
(_, Value::Sequence(_)) => Ordering::Greater,
486-
487-
(Value::Alias(a), Value::Alias(b)) => a.cmp(b),
488-
(Value::Alias(_), _) => Ordering::Less,
489-
(_, Value::Alias(_)) => Ordering::Greater,
490-
491-
(Value::Mapping(a), Value::Mapping(b)) => a.partial_cmp(b).unwrap_or_else(|| {
492-
iter_cmp_by(a, b, |(ak, av), (bk, bv)| {
493-
total_cmp(ak, bk).then_with(|| total_cmp(av, bv))
494-
})
495-
}),
496-
(Value::Mapping(_), _) => Ordering::Less,
497-
(_, Value::Mapping(_)) => Ordering::Greater,
498-
499-
(Value::Tagged(a), Value::Tagged(b)) => a
500-
.tag
501-
.cmp(&b.tag)
502-
.then_with(|| total_cmp(&a.value, &b.value)),
503-
}
504-
}
463+
(Value::Bool(a, _), Value::Bool(b, _)) => a.cmp(b),
464+
(Value::Bool(_, _), _) => Ordering::Less,
465+
(_, Value::Bool(_, _)) => Ordering::Greater,
505466

506-
fn iter_cmp_by<I, F>(this: I, other: I, mut cmp: F) -> Ordering
507-
where
508-
I: IntoIterator,
509-
F: FnMut(I::Item, I::Item) -> Ordering,
510-
{
511-
let mut this = this.into_iter();
512-
let mut other = other.into_iter();
513-
514-
loop {
515-
let x = match this.next() {
516-
None => {
517-
return if other.next().is_none() {
518-
Ordering::Equal
519-
} else {
520-
Ordering::Less
521-
}
522-
}
523-
Some(val) => val,
524-
};
467+
(Value::Number(a, _), Value::Number(b, _)) => a.total_cmp(b),
468+
(Value::Number(_, _), _) => Ordering::Less,
469+
(_, Value::Number(_, _)) => Ordering::Greater,
525470

526-
let y = match other.next() {
527-
None => return Ordering::Greater,
528-
Some(val) => val,
529-
};
471+
(Value::String(a, _), Value::String(b, _)) => a.cmp(b),
472+
(Value::String(_, _), _) => Ordering::Less,
473+
(_, Value::String(_, _)) => Ordering::Greater,
530474

531-
match cmp(x, y) {
532-
Ordering::Equal => {}
533-
non_eq => return non_eq,
534-
}
475+
(Value::Sequence(a), Value::Sequence(b)) => iter_cmp_by(a, b, total_cmp_value),
476+
(Value::Sequence(_), _) => Ordering::Less,
477+
(_, Value::Sequence(_)) => Ordering::Greater,
478+
479+
(Value::Alias(a), Value::Alias(b)) => a.cmp(b),
480+
(Value::Alias(_), _) => Ordering::Less,
481+
(_, Value::Alias(_)) => Ordering::Greater,
482+
483+
(Value::Mapping(a), Value::Mapping(b)) => a.partial_cmp(b).unwrap_or_else(|| {
484+
iter_cmp_by(a, b, |(ak, av), (bk, bv)| {
485+
total_cmp_value(ak, bk).then_with(|| total_cmp_value(av, bv))
486+
})
487+
}),
488+
(Value::Mapping(_), _) => Ordering::Less,
489+
(_, Value::Mapping(_)) => Ordering::Greater,
490+
491+
(Value::Tagged(a), Value::Tagged(b)) => a
492+
.tag
493+
.cmp(&b.tag)
494+
.then_with(|| total_cmp_value(&a.value, &b.value)),
495+
}
496+
}
497+
498+
fn iter_cmp_by<I, F>(this: I, other: I, mut cmp: F) -> Ordering
499+
where
500+
I: IntoIterator,
501+
F: FnMut(I::Item, I::Item) -> Ordering,
502+
{
503+
let mut this = this.into_iter();
504+
let mut other = other.into_iter();
505+
506+
loop {
507+
let x = match this.next() {
508+
None => {
509+
return if other.next().is_none() {
510+
Ordering::Equal
511+
} else {
512+
Ordering::Less
513+
};
535514
}
515+
Some(val) => val,
516+
};
517+
518+
let y = match other.next() {
519+
None => return Ordering::Greater,
520+
Some(val) => val,
521+
};
522+
523+
match cmp(x, y) {
524+
Ordering::Equal => {}
525+
non_eq => return non_eq,
536526
}
527+
}
528+
}
529+
530+
impl PartialOrd for Mapping {
531+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
532+
let mut self_entries = self.iter().collect::<Vec<_>>();
533+
let mut other_entries = other.iter().collect::<Vec<_>>();
537534

538535
// While sorting by map key, we get to assume that no two keys are
539536
// equal, otherwise they wouldn't both be in the map. This is not a safe
540537
// assumption outside of this situation.
541-
let total_cmp = |&(a, _): &_, &(b, _): &_| total_cmp(a, b);
538+
let total_cmp = |&(a, _): &_, &(b, _): &_| total_cmp_value(a, b);
542539
self_entries.sort_by(total_cmp);
543540
other_entries.sort_by(total_cmp);
544541
self_entries.partial_cmp(&other_entries)
@@ -885,3 +882,43 @@ impl<'de> Deserialize<'de> for Mapping {
885882
deserializer.deserialize_map(Visitor)
886883
}
887884
}
885+
886+
#[cfg(test)]
887+
mod tests {
888+
use super::Mapping;
889+
use crate::Value;
890+
use std::hash::{Hash, Hasher};
891+
892+
#[derive(Default)]
893+
struct CountingHasher {
894+
writes: usize,
895+
}
896+
897+
impl Hasher for CountingHasher {
898+
fn finish(&self) -> u64 {
899+
self.writes as u64
900+
}
901+
902+
fn write(&mut self, _bytes: &[u8]) {
903+
self.writes += 1;
904+
}
905+
906+
fn write_u64(&mut self, _i: u64) {
907+
self.writes += 1;
908+
}
909+
}
910+
911+
#[test]
912+
fn mapping_hash_feeds_entries_into_caller_hasher() {
913+
let mut mapping = Mapping::new();
914+
mapping.insert(
915+
Value::String("k".to_owned(), None),
916+
Value::String("v".to_owned(), None),
917+
);
918+
919+
let mut hasher = CountingHasher::default();
920+
mapping.hash(&mut hasher);
921+
922+
assert!(hasher.writes > 1);
923+
}
924+
}

0 commit comments

Comments
 (0)