commit a78957888957d0465a774ac938b71d680bec800f
parent 1afdb5b6cc1d07d49d5c8f369e1493dd844acad9
Author: Nick Mathewson <nickm@torproject.org>
Date: Tue, 22 May 2018 12:33:49 -0400
Merge branch 'trove-2018-005_034'
Diffstat:
7 files changed, 163 insertions(+), 16 deletions(-)
diff --git a/changes/TROVE-2018-005 b/changes/TROVE-2018-005
@@ -0,0 +1,6 @@
+ o Major bugfixes (security, directory authority, denial-of-service):
+ - Fix a bug that could have allowed an attacker to force a
+ directory authority to use up all its RAM by passing it a
+ maliciously crafted protocol versions string. Fixes bug 25517;
+ bugfix on 0.2.9.4-alpha. This issue is also tracked as
+ TROVE-2018-005.
diff --git a/src/or/dirauth/dirvote.c b/src/or/dirauth/dirvote.c
@@ -4358,6 +4358,12 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key,
microdescriptors = smartlist_new();
SMARTLIST_FOREACH_BEGIN(routers, routerinfo_t *, ri) {
+ /* If it has a protover list and contains a protocol name greater than
+ * MAX_PROTOCOL_NAME_LENGTH, skip it. */
+ if (ri->protocol_list &&
+ protover_contains_long_protocol_names(ri->protocol_list)) {
+ continue;
+ }
if (ri->cache_info.published_on >= cutoff) {
routerstatus_t *rs;
vote_routerstatus_t *vrs;
diff --git a/src/or/protover.c b/src/or/protover.c
@@ -53,6 +53,11 @@ static const struct {
#define N_PROTOCOL_NAMES ARRAY_LENGTH(PROTOCOL_NAMES)
+/* Maximum allowed length of any single subprotocol name. */
+// C_RUST_COUPLED: src/rust/protover/protover.rs
+// `MAX_PROTOCOL_NAME_LENGTH`
+static const uint MAX_PROTOCOL_NAME_LENGTH = 100;
+
/**
* Given a protocol_type_t, return the corresponding string used in
* descriptors.
@@ -198,6 +203,15 @@ parse_single_entry(const char *s, const char *end_of_entry)
if (equals == s)
goto error;
+ /* The name must not be longer than MAX_PROTOCOL_NAME_LENGTH. */
+ if (equals - s > MAX_PROTOCOL_NAME_LENGTH) {
+ log_warn(LD_NET, "When parsing a protocol entry, I got a very large "
+ "protocol name. This is possibly an attack or a bug, unless "
+ "the Tor network truly supports protocol names larger than "
+ "%ud characters. The offending string was: %s",
+ MAX_PROTOCOL_NAME_LENGTH, escaped(out->name));
+ goto error;
+ }
out->name = tor_strndup(s, equals-s);
tor_assert(equals < end_of_entry);
@@ -263,6 +277,18 @@ parse_protocol_list(const char *s)
}
/**
+ * Return true if the unparsed protover in <b>s</b> would contain a protocol
+ * name longer than MAX_PROTOCOL_NAME_LENGTH, and false otherwise.
+ */
+bool
+protover_contains_long_protocol_names(const char *s)
+{
+ if (!parse_protocol_list(s))
+ return true;
+ return false;
+}
+
+/**
* Given a protocol type and version number, return true iff we know
* how to speak that protocol.
*/
@@ -439,6 +465,14 @@ expand_protocol_list(const smartlist_t *protos)
SMARTLIST_FOREACH_BEGIN(protos, const proto_entry_t *, ent) {
const char *name = ent->name;
+ if (strlen(name) > MAX_PROTOCOL_NAME_LENGTH) {
+ log_warn(LD_NET, "When expanding a protocol entry, I got a very large "
+ "protocol name. This is possibly an attack or a bug, unless "
+ "the Tor network truly supports protocol names larger than "
+ "%ud characters. The offending string was: %s",
+ MAX_PROTOCOL_NAME_LENGTH, escaped(name));
+ continue;
+ }
SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) {
uint32_t u;
for (u = range->low; u <= range->high; ++u) {
diff --git a/src/or/protover.h b/src/or/protover.h
@@ -42,6 +42,7 @@ typedef enum protocol_type_t {
PRT_CONS,
} protocol_type_t;
+bool protover_contains_long_protocol_names(const char *s);
int protover_all_supported(const char *s, char **missing);
int protover_is_supported_here(protocol_type_t pr, uint32_t ver);
const char *protover_get_supported_protocols(void);
diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs
@@ -56,7 +56,8 @@ pub extern "C" fn protover_all_supported(
Err(_) => return 1,
};
- let relay_proto_entry: UnvalidatedProtoEntry = match relay_version.parse() {
+ let relay_proto_entry: UnvalidatedProtoEntry =
+ match UnvalidatedProtoEntry::from_str_any_len(relay_version) {
Ok(n) => n,
Err(_) => return 1,
};
@@ -167,6 +168,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char {
pub extern "C" fn protover_compute_vote(
list: *const Stringlist,
threshold: c_int,
+ allow_long_proto_names: bool,
) -> *mut c_char {
if list.is_null() {
@@ -181,9 +183,13 @@ pub extern "C" fn protover_compute_vote(
let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new();
for datum in data {
- let entry: UnvalidatedProtoEntry = match datum.parse() {
- Ok(x) => x,
- Err(_) => continue,
+ let entry: UnvalidatedProtoEntry = match allow_long_proto_names {
+ true => match UnvalidatedProtoEntry::from_str_any_len(datum.as_str()) {
+ Ok(n) => n,
+ Err(_) => continue},
+ false => match datum.parse() {
+ Ok(n) => n,
+ Err(_) => continue},
};
proto_entries.push(entry);
}
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
@@ -28,6 +28,9 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha";
/// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND`
const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16);
+/// The maximum size an `UnknownProtocol`'s name may be.
+pub(crate) const MAX_PROTOCOL_NAME_LENGTH: usize = 100;
+
/// Known subprotocols in Tor. Indicates which subprotocol a relay supports.
///
/// C_RUST_COUPLED: src/or/protover.h `protocol_type_t`
@@ -90,6 +93,18 @@ impl FromStr for UnknownProtocol {
type Err = ProtoverError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if s.len() <= MAX_PROTOCOL_NAME_LENGTH {
+ Ok(UnknownProtocol(s.to_string()))
+ } else {
+ Err(ProtoverError::ExceedsNameLimit)
+ }
+ }
+}
+
+impl UnknownProtocol {
+ /// Create an `UnknownProtocol`, ignoring whether or not it
+ /// exceeds MAX_PROTOCOL_NAME_LENGTH.
+ fn from_str_any_len(s: &str) -> Result<Self, ProtoverError> {
Ok(UnknownProtocol(s.to_string()))
}
}
@@ -417,6 +432,49 @@ impl UnvalidatedProtoEntry {
};
supported_versions.iter().any(|v| v.1 >= *vers)
}
+
+ /// Split a string containing (potentially) several protocols and their
+ /// versions into a `Vec` of tuples of string in `(protocol, versions)`
+ /// form.
+ ///
+ /// # Inputs
+ ///
+ /// A &str in the form `"Link=3-4 Cons=5"`.
+ ///
+ /// # Returns
+ ///
+ /// A `Result` whose `Ok` variant is a `Vec<(&str, &str)>` of `(protocol,
+ /// versions)`, or whose `Err` variant is a `ProtoverError`.
+ ///
+ /// # Errors
+ ///
+ /// This will error with a `ProtoverError::Unparseable` if any of the
+ /// following are true:
+ ///
+ /// * If a protocol name is an empty string, e.g. `"Cons=1,3 =3-5"`.
+ /// * If a protocol name cannot be parsed as utf-8.
+ /// * If the version numbers are an empty string, e.g. `"Cons="`.
+ fn parse_protocol_and_version_str<'a>(protocol_string: &'a str)
+ -> Result<Vec<(&'a str, &'a str)>, ProtoverError>
+ {
+ let mut protovers: Vec<(&str, &str)> = Vec::new();
+
+ for subproto in protocol_string.split(' ') {
+ let mut parts = subproto.splitn(2, '=');
+
+ let name = match parts.next() {
+ Some("") => return Err(ProtoverError::Unparseable),
+ Some(n) => n,
+ None => return Err(ProtoverError::Unparseable),
+ };
+ let vers = match parts.next() {
+ Some(n) => n,
+ None => return Err(ProtoverError::Unparseable),
+ };
+ protovers.push((name, vers));
+ }
+ Ok(protovers)
+ }
}
impl FromStr for UnvalidatedProtoEntry {
@@ -449,19 +507,10 @@ impl FromStr for UnvalidatedProtoEntry {
/// * If the version string is malformed. See `impl FromStr for ProtoSet`.
fn from_str(protocol_string: &str) -> Result<UnvalidatedProtoEntry, ProtoverError> {
let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default();
+ let parts: Vec<(&str, &str)> =
+ UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?;
- for subproto in protocol_string.split(' ') {
- let mut parts = subproto.splitn(2, '=');
-
- let name = match parts.next() {
- Some("") => return Err(ProtoverError::Unparseable),
- Some(n) => n,
- None => return Err(ProtoverError::Unparseable),
- };
- let vers = match parts.next() {
- Some(n) => n,
- None => return Err(ProtoverError::Unparseable),
- };
+ for &(name, vers) in parts.iter() {
let versions = ProtoSet::from_str(vers)?;
let protocol = UnknownProtocol::from_str(name)?;
@@ -471,6 +520,26 @@ impl FromStr for UnvalidatedProtoEntry {
}
}
+impl UnvalidatedProtoEntry {
+ /// Create an `UnknownProtocol`, ignoring whether or not it
+ /// exceeds MAX_PROTOCOL_NAME_LENGTH.
+ pub(crate) fn from_str_any_len(protocol_string: &str)
+ -> Result<UnvalidatedProtoEntry, ProtoverError>
+ {
+ let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default();
+ let parts: Vec<(&str, &str)> =
+ UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?;
+
+ for &(name, vers) in parts.iter() {
+ let versions = ProtoSet::from_str(vers)?;
+ let protocol = UnknownProtocol::from_str_any_len(name)?;
+
+ parsed.insert(protocol, versions);
+ }
+ Ok(parsed)
+ }
+}
+
/// Pretend a `ProtoEntry` is actually an `UnvalidatedProtoEntry`.
impl From<ProtoEntry> for UnvalidatedProtoEntry {
fn from(proto_entry: ProtoEntry) -> UnvalidatedProtoEntry {
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
@@ -125,6 +125,13 @@ test_protover_parse_fail(void *arg)
/* Broken range */
elts = parse_protocol_list("Link=1,9-8,3");
tt_ptr_op(elts, OP_EQ, NULL);
+
+ /* Protocol name too long */
+ elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+ tt_ptr_op(elts, OP_EQ, NULL);
+
#endif
done:
;
@@ -219,6 +226,15 @@ test_protover_vote(void *arg)
tt_str_op(result, OP_EQ, "");
tor_free(result);
+ /* Protocol name too long */
+ smartlist_clear(lst);
+ smartlist_add(lst, (void*) "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+ result = protover_compute_vote(lst, 1);
+ tt_str_op(result, OP_EQ, "");
+ tor_free(result);
+
done:
tor_free(result);
smartlist_free(lst);
@@ -300,6 +316,15 @@ test_protover_all_supported(void *arg)
tt_assert(protover_all_supported("Sleen=0-4294967295", &msg));
tor_end_capture_bugs_();
+ /* Protocol name too long */
+ tor_capture_bugs_(1);
+ tt_assert(protover_all_supported(
+ "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaa=1-65536", &msg));
+ tor_end_capture_bugs_();
+
done:
tor_end_capture_bugs_();
tor_free(msg);
