From e550f95e86e6cfbe55a57d9afb715e233b0990bb Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Sun, 18 Feb 2024 14:10:09 -0800 Subject: [PATCH] RSA internals: Make it clearer that serializing the public key won't 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. --- src/io.rs | 6 ++++++ src/io/der_writer.rs | 38 ++++++++++++++++++++++---------------- src/io/toolongerror.rs | 29 +++++++++++++++++++++++++++++ src/io/writer.rs | 29 ++++++++++++++++++++--------- src/rsa/public_key.rs | 7 ++++--- 5 files changed, 81 insertions(+), 28 deletions(-) create mode 100644 src/io/toolongerror.rs diff --git a/src/io.rs b/src/io.rs index e7f7cc36f5..3e73df08d3 100644 --- a/src/io.rs +++ b/src/io.rs @@ -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; diff --git a/src/io/der_writer.rs b/src/io/der_writer.rs index 2b0b8c9f35..01d0632b0d 100644 --- a/src/io/der_writer.rs +++ b/src/io/der_writer.rs @@ -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, 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(output: &mut dyn Accumulator, tag: Tag, write_value: F) +fn write_tlv(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)?; } - output.write_byte(lo); + output.write_byte(lo)?; - write_value(output); + write_value(output) } diff --git a/src/io/toolongerror.rs b/src/io/toolongerror.rs new file mode 100644 index 0000000000..ddecdb244e --- /dev/null +++ b/src/io/toolongerror.rs @@ -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(()) + } +} + +impl From for error::Unspecified { + fn from(_: TooLongError) -> Self { + Self + } +} diff --git a/src/io/writer.rs b/src/io/writer.rs index 1d7b443142..62c81b8cbd 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -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 { @@ -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(()) } } @@ -66,14 +72,19 @@ impl From 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()) } diff --git a/src/rsa/public_key.rs b/src/rsa/public_key.rs index df8b7f2628..fc7eebac16 100644 --- a/src/rsa/public_key.rs +++ b/src/rsa/public_key.rs @@ -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 }) }