Skip to content

Commit

Permalink
Remove PartialEq and Eq implementations for Input.
Browse files Browse the repository at this point in the history
These implementations are less useful given that most users do not have
`Input` in their public APIs (any more). I verified that *ring* and
webpki can be changed to work with this change.

This eliminates one potential place where secret data could leak through
a side channel. `untrusted` didn't make a promise about this previously,
but going forward we should err more on the side of caution.
  • Loading branch information
briansmith committed Jul 13, 2021
1 parent cb3bd9d commit 36f6dff
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 29 deletions.
29 changes: 4 additions & 25 deletions src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ use crate::{no_panic, Reader};
/// A wrapper around `&'a [u8]` that helps in writing panic-free code.
///
/// No methods of `Input` will ever panic.
#[derive(Clone, Copy, Debug, Eq)]
///
/// Intentionally avoids implementing `PartialEq` and `Eq` to avoid implicit
/// non-constant-time comparisons.
#[derive(Clone, Copy, Debug)]
pub struct Input<'a> {
value: no_panic::Slice<'a>,
}
Expand Down Expand Up @@ -88,27 +91,3 @@ impl<'a> From<no_panic::Slice<'a>> for Input<'a> {
Self { value }
}
}

// #[derive(PartialEq)] would result in lifetime bounds that are
// unnecessarily restrictive; see
// https://github.com/rust-lang/rust/issues/26925.
impl PartialEq<Input<'_>> for Input<'_> {
#[inline]
fn eq(&self, other: &Input) -> bool {
self.as_slice_less_safe() == other.as_slice_less_safe()
}
}

impl PartialEq<[u8]> for Input<'_> {
#[inline]
fn eq(&self, other: &[u8]) -> bool {
self.as_slice_less_safe() == other
}
}

impl PartialEq<Input<'_>> for [u8] {
#[inline]
fn eq(&self, other: &Input) -> bool {
other.as_slice_less_safe() == self
}
}
5 changes: 4 additions & 1 deletion src/no_panic.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
/// A wrapper around a slice that exposes no functions that can panic.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
///
/// Intentionally avoids implementing `PartialEq` and `Eq` to avoid implicit
/// non-constant-time comparisons.
#[derive(Clone, Copy, Debug)]
pub struct Slice<'a> {
bytes: &'a [u8],
}
Expand Down
3 changes: 3 additions & 0 deletions src/reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ use crate::{no_panic, Input};
/// conjunction with `read_all` and `read_all_optional` helps ensure that no
/// byte of the input is accidentally left unprocessed. The methods of `Reader`
/// never panic, so `Reader` also assists the writing of panic-free code.
///
/// Intentionally avoids implementing `PartialEq` and `Eq` to avoid implicit
/// non-constant-time comparisons.
#[derive(Debug)]
pub struct Reader<'a> {
input: no_panic::Slice<'a>,
Expand Down
9 changes: 6 additions & 3 deletions tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ fn test_input_clone_and_copy() {
for input in INPUTS {
let input = untrusted::Input::from(input);
let copy = input;
assert_eq!(input, copy);
assert_eq!(input, input.clone());
assert_eq!(input.as_slice_less_safe(), copy.as_slice_less_safe());
assert_eq!(
input.as_slice_less_safe(),
input.clone().as_slice_less_safe()
);
}
}

Expand Down Expand Up @@ -78,7 +81,7 @@ fn using_reader_after_skip_and_get_error_returns_error_must_not_panic() {
let input = untrusted::Input::from(&[]);
let r = input.read_all(untrusted::EndOfInput, |input| {
let r = input.read_bytes(1);
assert_eq!(r, Err(untrusted::EndOfInput));
assert_eq!(r.unwrap_err(), untrusted::EndOfInput);
Ok(input.read_bytes_to_end())
});
let _ = r; // "Use" r. The value of `r` is undefined here.
Expand Down

0 comments on commit 36f6dff

Please sign in to comment.