Skip to content

Commit

Permalink
RSA internals: Make it clearer that serializing the public key won't …
Browse files Browse the repository at this point in the history
…panic.

RsaKeyPair rejects keys that are so large that they can't be serialized
by the DER writer, so no panic will happen. However, this is far from
obvious, so just make the DER writer fallible instead of having it
panic.
  • Loading branch information
briansmith committed Feb 19, 2024
1 parent 9cb93e0 commit e550f95
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 28 deletions.
6 changes: 6 additions & 0 deletions src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,10 @@ pub(crate) mod der_writer;

pub(crate) mod positive;

#[cfg(feature = "alloc")]
mod toolongerror;

pub use self::positive::Positive;

#[cfg(feature = "alloc")]
pub(crate) use self::toolongerror::TooLongError;
38 changes: 22 additions & 16 deletions src/io/der_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,51 +15,57 @@
use super::{der::*, writer::*, *};
use alloc::boxed::Box;

pub(crate) fn write_positive_integer(output: &mut dyn Accumulator, value: &Positive) {
pub(crate) fn write_positive_integer(
output: &mut dyn Accumulator,
value: &Positive,
) -> Result<(), TooLongError> {
let first_byte = value.first_byte();
let value = value.big_endian_without_leading_zero_as_input();
write_tlv(output, Tag::Integer, |output| {
if (first_byte & 0x80) != 0 {
output.write_byte(0); // Disambiguate negative number.
output.write_byte(0)?; // Disambiguate negative number.
}
write_copy(output, value)
})
}

pub(crate) fn write_all(tag: Tag, write_value: &dyn Fn(&mut dyn Accumulator)) -> Box<[u8]> {
pub(crate) fn write_all(
tag: Tag,
write_value: &dyn Fn(&mut dyn Accumulator) -> Result<(), TooLongError>,
) -> Result<Box<[u8]>, TooLongError> {
let length = {
let mut length = LengthMeasurement::zero();
write_tlv(&mut length, tag, write_value);
write_tlv(&mut length, tag, write_value)?;
length
};

let mut output = Writer::with_capacity(length);
write_tlv(&mut output, tag, write_value);
write_tlv(&mut output, tag, write_value)?;

output.into()
Ok(output.into())
}

fn write_tlv<F>(output: &mut dyn Accumulator, tag: Tag, write_value: F)
fn write_tlv<F>(output: &mut dyn Accumulator, tag: Tag, write_value: F) -> Result<(), TooLongError>
where
F: Fn(&mut dyn Accumulator),
F: Fn(&mut dyn Accumulator) -> Result<(), TooLongError>,
{
let length: usize = {
let mut length = LengthMeasurement::zero();
write_value(&mut length);
write_value(&mut length)?;
length.into()
};
let length: u16 = length.try_into().unwrap();
let length: u16 = length.try_into().map_err(|_| TooLongError::new())?;

output.write_byte(tag.into());
output.write_byte(tag.into())?;

let [lo, hi] = length.to_le_bytes();
if length >= 0x1_00 {
output.write_byte(0x82);
output.write_byte(hi);
output.write_byte(0x82)?;
output.write_byte(hi)?;
} else if length >= 0x80 {
output.write_byte(0x81);
output.write_byte(0x81)?;

Check warning on line 66 in src/io/der_writer.rs

View check run for this annotation

Codecov / codecov/patch

src/io/der_writer.rs#L66

Added line #L66 was not covered by tests
}
output.write_byte(lo);
output.write_byte(lo)?;

write_value(output);
write_value(output)
}
29 changes: 29 additions & 0 deletions src/io/toolongerror.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2024 Brian Smith.
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
// copyright notice and this permission notice appear in all copies.
//
// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHORS DISCLAIM ALL WARRANTIES
// WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
// MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY
// SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use crate::error;

pub struct TooLongError(());

impl TooLongError {
pub fn new() -> Self {
Self(())
}

Check warning on line 22 in src/io/toolongerror.rs

View check run for this annotation

Codecov / codecov/patch

src/io/toolongerror.rs#L20-L22

Added lines #L20 - L22 were not covered by tests
}

impl From<TooLongError> for error::Unspecified {
fn from(_: TooLongError) -> Self {
Self
}

Check warning on line 28 in src/io/toolongerror.rs

View check run for this annotation

Codecov / codecov/patch

src/io/toolongerror.rs#L26-L28

Added lines #L26 - L28 were not covered by tests
}
29 changes: 20 additions & 9 deletions src/io/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use super::TooLongError;
use alloc::{boxed::Box, vec::Vec};

pub trait Accumulator {
fn write_byte(&mut self, value: u8);
fn write_bytes(&mut self, value: &[u8]);
fn write_byte(&mut self, value: u8) -> Result<(), TooLongError>;
fn write_bytes(&mut self, value: &[u8]) -> Result<(), TooLongError>;
}

pub(super) struct LengthMeasurement {
Expand All @@ -36,11 +37,16 @@ impl LengthMeasurement {
}

impl Accumulator for LengthMeasurement {
fn write_byte(&mut self, _value: u8) {
self.len += 1;
fn write_byte(&mut self, _value: u8) -> Result<(), TooLongError> {
self.len = self.len.checked_add(1).ok_or_else(TooLongError::new)?;
Ok(())
}
fn write_bytes(&mut self, value: &[u8]) {
self.len += value.len();
fn write_bytes(&mut self, value: &[u8]) -> Result<(), TooLongError> {
self.len = self
.len
.checked_add(value.len())
.ok_or_else(TooLongError::new)?;
Ok(())
}
}

Expand All @@ -66,14 +72,19 @@ impl From<Writer> for Box<[u8]> {
}

impl Accumulator for Writer {
fn write_byte(&mut self, value: u8) {
fn write_byte(&mut self, value: u8) -> Result<(), TooLongError> {
self.bytes.push(value);
Ok(())
}
fn write_bytes(&mut self, value: &[u8]) {
fn write_bytes(&mut self, value: &[u8]) -> Result<(), TooLongError> {
self.bytes.extend(value);
Ok(())
}
}

pub fn write_copy(accumulator: &mut dyn Accumulator, to_copy: untrusted::Input) {
pub fn write_copy(
accumulator: &mut dyn Accumulator,
to_copy: untrusted::Input,
) -> Result<(), TooLongError> {
accumulator.write_bytes(to_copy.as_slice_less_safe())
}
7 changes: 4 additions & 3 deletions src/rsa/public_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@ impl PublicKey {
let e_bytes = io::Positive::from_be_bytes(e_bytes)
.map_err(|_: error::Unspecified| error::KeyRejected::unexpected_error())?;
let serialized = der_writer::write_all(der::Tag::Sequence, &|output| {
der_writer::write_positive_integer(output, &n_bytes);
der_writer::write_positive_integer(output, &e_bytes);
});
der_writer::write_positive_integer(output, &n_bytes)?;
der_writer::write_positive_integer(output, &e_bytes)
})
.map_err(|_: io::TooLongError| error::KeyRejected::unexpected_error())?;

Ok(Self { inner, serialized })
}
Expand Down

0 comments on commit e550f95

Please sign in to comment.