Skip to content

Commit

Permalink
fix: do not parse base62 strings of unexpected length (#8)
Browse files Browse the repository at this point in the history
Reject parsing base62 strings that would result in unexpectedly
long payloads and potentially large allocations. Without this, it
is possible to use a base62 string to pass in extra bytes of data
that are unused (but do consume memory for a time). I don't know
if this is something people rely on, but it seems unlikely that
anyone would find this desirable. This might represent a possible
way to OOM any code accepting KSUIDs that didn't limit incoming
payload sizes already, but there are probably better options for that.

Add a test for both long and short base62 strings to try to skip
allocating anything for strings of the wrong length by decoding
them. Previously, length checks only happened after decoding and
would only take the tail end of the decoded base62 string.
  • Loading branch information
nilium committed Feb 29, 2024
1 parent cc8ba21 commit f71ac1d
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ pub const KSUID_EPOCH: i64 = 1_400_000_000;
const BASE_62_CHARS: &[u8; 62] = b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

const TOTAL_BYTES: usize = 20;
const TOTAL_BYTES_BASE62: usize = 27;

#[derive(Debug)]
pub struct Error(String);
Expand Down Expand Up @@ -237,6 +238,12 @@ pub trait KsuidLike {
///
/// ```
fn from_base62(s: &str) -> Result<Self::Type, Error> {
if s.len() != TOTAL_BYTES_BASE62 {
return Err(Error(format!(
"Got base62 ksuid of unexpected length {}",
s.len()
)));
}
if let Some(loaded) = base_encode::from_str(s, 62, BASE_62_CHARS) {
// Get the last TOTAL_BYTES
let loaded = if loaded.len() > TOTAL_BYTES {
Expand Down
12 changes: 12 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,3 +203,15 @@ fn test_deserialize_from_base62() {
assert_eq!(ksuid_obj.id.to_string(), b62);
assert_eq!(ksuidms_obj.id.to_string(), b62);
}

#[test]
fn test_deserialize_bad_base62_length() {
let short_b62 = "ZBpBUvZwXKQmoEYga2";
let long_b62 = "1srOrx2ZWZBpBUvZwXKQmoEYga21srOrx2ZWZBpBUvZwXKQmoEYga2";

let result = Ksuid::from_base62(short_b62);
assert!(result.is_err(), "Short base62 strings should fail to parse");

let result = Ksuid::from_base62(long_b62);
assert!(result.is_err(), "Long base62 strings should fail to parse");
}

0 comments on commit f71ac1d

Please sign in to comment.