Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions der/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ der_derive = { version = "=0.6.0-pre.1", optional = true, path = "derive" }
flagset = { version = "0.4.3", optional = true }
pem-rfc7468 = { version = "=0.4.0-pre.0", optional = true, path = "../pem-rfc7468" }
time = { version = "0.3", optional = true, default-features = false }
zeroize = { version = "1", optional = true, default-features = false }

[dev-dependencies]
hex-literal = "0.3"
Expand Down
194 changes: 151 additions & 43 deletions der/src/document.rs
Original file line number Diff line number Diff line change
@@ -1,67 +1,92 @@
//! ASN.1 DER-encoded documents stored on the heap.

use crate::{Decodable, Encodable, Error, Result};
use alloc::{boxed::Box, vec::Vec};
use alloc::vec::Vec;
use core::{
fmt::{self, Debug},
marker::PhantomData,
};

#[cfg(feature = "pem")]
use {crate::pem, alloc::string::String};
use {
crate::pem::{self, PemLabel},
alloc::string::String,
core::str::FromStr,
};

#[cfg(feature = "std")]
use std::{fs, path::Path};

/// ASN.1 DER-encoded document.
///
/// This trait is intended to impl on types which contain an ASN.1 DER-encoded
/// document which is guaranteed to encode as the associated `Message` type.
/// This type wraps an encoded ASN.1 DER message which is guaranteed to
/// infallibly decode as type `T`.
///
/// It implements common functionality related to encoding/decoding such
/// documents, such as PEM encapsulation as well as reading/writing documents
/// from/to the filesystem.
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
pub trait Document<'a>: AsRef<[u8]> + Sized + TryFrom<Vec<u8>, Error = Error> {
/// ASN.1 message type this document decodes to.
type Message: Decodable<'a> + Encodable + Sized;
pub struct Document<T, const SENSITIVE: bool> {
/// ASN.1 DER-encoded document guaranteed to decode to `T` infallibly.
der_bytes: Vec<u8>,

/// Does this type contain potentially sensitive data?
///
/// This enables hardened file permissions when persisting data to disk.
const SENSITIVE: bool;
/// Rust type corresponding to the ASN.1 DER message the bytes can be
/// infallibly deserialized as.
msg_type: PhantomData<T>,
}

impl<T, const SENSITIVE: bool> Document<T, SENSITIVE> {
/// Borrow the inner serialized bytes of this document.
fn as_der(&self) -> &[u8] {
self.as_ref()
}

/// Return an allocated ASN.1 DER serialization as a boxed slice.
fn to_der(&self) -> Box<[u8]> {
self.as_ref().to_vec().into_boxed_slice()
pub fn as_der(&self) -> &[u8] {
self.der_bytes.as_slice()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually know if the bytes are der because we haven't decoded them yet. Perhaps we should just let AsRef or even Deref do the heavy lifting here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take a look at the definition of from_der, you'll see it checks the document decodes using doc.try_decode() before returning it to the caller. The type definitely does maintain the invariant that self.der_bytes will always decode successfully, and as I noted in #421, it may be possible to store the parsed message to prevent repeat decoding (definitely not going to attempt that in this PR).

Also for some reason GitHub isn't letting me reply to your comment about the PEM label, but there's already a PemLabel trait for that.

And FWIW locally I'm trying to replace the trait with a const generic parameter, and liking that better for now.


/// Decode this document as ASN.1 DER.
fn decode(&'a self) -> Self::Message {
Self::Message::from_der(self.as_ref()).expect("ASN.1 DER document malformed")
pub fn decode<'a>(&'a self) -> T
where
T: Decodable<'a> + Sized,
{
self.try_decode().expect("ASN.1 DER document malformed")
}

/// Create a new document from the provided ASN.1 DER bytes.
fn from_der(bytes: &[u8]) -> Result<Self> {
bytes.to_vec().try_into()
pub fn from_der(bytes: impl Into<Vec<u8>>) -> Result<Self>
where
T: for<'a> Decodable<'a> + Sized,
{
let doc = Self {
der_bytes: bytes.into(),
msg_type: PhantomData,
};

// Ensure document parses successfully
doc.try_decode()?;
Ok(doc)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should try decoding the der. This has performance and security implications. I think the Document type should only have the invariant that it contains bytes successfully decoded from a PEM file. The important bit is that T is linked via the type system to the PEM document "name" and that we know the sensitivity of the bytes.


/// Return an allocated ASN.1 DER serialization as a byte vector.
pub fn to_der(&self) -> Vec<u8> {
self.der_bytes.clone()
}

/// Encode the provided type as ASN.1 DER.
fn from_msg(msg: &Self::Message) -> Result<Self> {
pub fn from_msg(msg: &T) -> Result<Self>
where
T: for<'a> Decodable<'a> + Encodable + Sized,
{
msg.to_vec()?.try_into()
}

/// Decode ASN.1 DER document from PEM.
#[cfg(feature = "pem")]
#[cfg_attr(docsrs, doc(cfg(feature = "pem")))]
fn from_pem(s: &str) -> Result<Self>
pub fn from_pem(s: &str) -> Result<Self>
where
Self: pem::PemLabel,
T: for<'a> Decodable<'a> + PemLabel + Sized,
{
let (label, der_bytes) = pem::decode_vec(s.as_bytes())?;

if label != Self::TYPE_LABEL {
if label != T::TYPE_LABEL {
return Err(pem::Error::Label.into());
}

Expand All @@ -71,49 +96,132 @@ pub trait Document<'a>: AsRef<[u8]> + Sized + TryFrom<Vec<u8>, Error = Error> {
/// Encode ASN.1 DER document as a PEM string.
#[cfg(feature = "pem")]
#[cfg_attr(docsrs, doc(cfg(feature = "pem")))]
fn to_pem(&self, line_ending: pem::LineEnding) -> Result<String>
pub fn to_pem(&self, line_ending: pem::LineEnding) -> Result<String>
where
Self: pem::PemLabel,
T: PemLabel,
{
Ok(pem::encode_string(
Self::TYPE_LABEL,
T::TYPE_LABEL,
line_ending,
self.as_ref(),
self.as_der(),
)?)
}

/// Read ASN.1 DER document from a file.
#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
fn read_der_file(path: impl AsRef<Path>) -> Result<Self> {
pub fn read_der_file(path: impl AsRef<Path>) -> Result<Self>
where
T: for<'a> Decodable<'a> + Sized,
{
fs::read(path)?.try_into()
}

/// Write ASN.1 DER document to a file.
#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
pub fn write_der_file(&self, path: impl AsRef<Path>) -> Result<()> {
write_file(path, self.as_der(), SENSITIVE)
}

/// Read PEM-encoded ASN.1 DER document from a file.
#[cfg(all(feature = "pem", feature = "std"))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "pem", feature = "std"))))]
fn read_pem_file(path: impl AsRef<Path>) -> Result<Self>
pub fn read_pem_file(path: impl AsRef<Path>) -> Result<Self>
where
Self: pem::PemLabel,
T: for<'a> Decodable<'a> + PemLabel + Sized,
{
Self::from_pem(&fs::read_to_string(path)?)
}

/// Write ASN.1 DER document to a file.
#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
fn write_der_file(&self, path: impl AsRef<Path>) -> Result<()> {
write_file(path, self.as_ref(), Self::SENSITIVE)
}

/// Write PEM-encoded ASN.1 DER document to a file.
#[cfg(all(feature = "pem", feature = "std"))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "pem", feature = "std"))))]
fn write_pem_file(&self, path: impl AsRef<Path>, line_ending: pem::LineEnding) -> Result<()>
pub fn write_pem_file(&self, path: impl AsRef<Path>, line_ending: pem::LineEnding) -> Result<()>
where
T: PemLabel,
{
write_file(path, self.to_pem(line_ending)?.as_bytes(), SENSITIVE)
}

/// Attempt to decode `self.der_bytes` as `T`.
///
/// This method doesn't uphold the invariant that `T` always decodes
/// successfully, but is needed to make the lifetimes for the constructor
/// work.
fn try_decode<'a>(&'a self) -> Result<T>
where
Self: pem::PemLabel,
T: Decodable<'a> + Sized,
{
write_file(path, self.to_pem(line_ending)?.as_bytes(), Self::SENSITIVE)
T::from_der(self.as_der())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should ditch that invariant altogether.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That defeats the whole point. Then it's no better than a Vec<u8>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document combines three functionalities:

  1. Zeroing the Vec<u8>. This has clear value.
  2. Ensuring file permissions on write. This has clear value. I have some minor concern, however, that this is only useful in a narrow set of cases (i.e. where you want to do full, synchronous io). If you want to do partial io (such as a certificate chain incremental io) or asynchronous io, this type drops this value.
  3. Ensuring the bytes will successfully decode to T. The value of this is unclear to me. And it carries a performance and security cost.

Dropping the third guarantee still leaves Document with two features. So it is still better than a Vec<u8>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensuring the bytes will successfully decode to T. The value of this is unclear to me. A

The goal is to have something that's both type safe and owned, as opposed to a newtype for a bag-of-bytes which may or may not parse successfully as DER. See your own comment about that above.

Similar concerns came up with code review on the yubikey crate.

And it carries a performance and security cost.

Security cost? How so?

The performance cost can be obviated by storing the parsed result, although that's tricky due to self-borrowing.

All that said, I'm not sure this will actually work out, but I'll leave a comment about that below.

}

impl<T, const SENSITIVE: bool> AsRef<[u8]> for Document<T, SENSITIVE> {
fn as_ref(&self) -> &[u8] {
self.as_der()
}
}

impl<T> Debug for Document<T, false>
where
T: for<'a> Decodable<'a> + Debug + Sized,
{
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_tuple("Document").field(&self.decode()).finish()
}
}

impl<T> Debug for Document<T, true> {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("Document").finish_non_exhaustive()
}
}

// NOTE: `Drop` is defined unconditionally to ensure bounds do not change based
// on selected cargo features, which would not be a purely additive change
impl<T, const SENSITIVE: bool> Drop for Document<T, SENSITIVE> {
fn drop(&mut self) {
#[cfg(feature = "zeroize")]
if SENSITIVE {
use zeroize::Zeroize;
self.der_bytes.zeroize();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be easier to just unconditionally Zeroize<Vec<u8>>. I presume you're worried about performance of zeroing large certificates. If you're willing to take the performance tradeoffs, zeroing even the non-sensitive bits has security benefits. For example, attackers often search for the public key to correlate the private key's memory position (or the position of some of its non-zeroed fragments). Zeroing both adds one additional hurdle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an option, but the sensitivity information is already there for other purposes, so it seems ok to me to make use of it for this application as well.


impl<T, const SENSITIVE: bool> TryFrom<&[u8]> for Document<T, SENSITIVE>
where
T: for<'a> Decodable<'a> + Sized,
{
type Error = Error;

fn try_from(der_bytes: &[u8]) -> Result<Self> {
Self::from_der(der_bytes)
}
}

impl<T, const SENSITIVE: bool> TryFrom<Vec<u8>> for Document<T, SENSITIVE>
where
T: for<'a> Decodable<'a> + Sized,
{
type Error = Error;

fn try_from(der_bytes: Vec<u8>) -> Result<Self> {
Self::from_der(der_bytes)
}
}

#[cfg(feature = "pem")]
#[cfg_attr(docsrs, doc(cfg(feature = "pem")))]
impl<T, const SENSITIVE: bool> FromStr for Document<T, SENSITIVE>
where
T: for<'a> Decodable<'a> + PemLabel + Sized,
{
type Err = Error;

fn from_str(s: &str) -> Result<Self> {
Self::from_pem(s)
}
}

Expand Down