diff options
author | Neal H. Walfield <neal@pep.foundation> | 2020-03-03 23:20:33 +0100 |
---|---|---|
committer | Neal H. Walfield <neal@pep.foundation> | 2020-03-03 23:52:03 +0100 |
commit | a61bfdab687e9c0b6c5e263ed304f48183059a45 (patch) | |
tree | 545d516b6b2fe513f63caf3bcc81024338459583 /openpgp/src | |
parent | 93dbeb3160262726d4c60b4811cf352c08dfc5b3 (diff) |
openpgp: Only impl Serialize for objects that are normally exported.
- Add two new traits: `Marshal` and `MarshalInto`.
- Implement them instead of `Serialize` and `SerializeInto`.
- Only implement `Serialize` and `SerializeInto` for data structures
that are normally exported.
- This should prevent users from accidentally serializing a bare
signature (`Signature`) when they meant to serialize a signature
packet (`Packet`), for instance.
- Fixes #368.
Diffstat (limited to 'openpgp/src')
24 files changed, 451 insertions, 196 deletions
diff --git a/openpgp/src/armor.rs b/openpgp/src/armor.rs index cb106e03..6f11958b 100644 --- a/openpgp/src/armor.rs +++ b/openpgp/src/armor.rs @@ -38,7 +38,7 @@ use quickcheck::{Arbitrary, Gen}; use crate::vec_truncate; use crate::packet::prelude::*; use crate::packet::header::{BodyLength, CTBNew, CTBOld}; -use crate::serialize::SerializeInto; +use crate::serialize::MarshalInto; /// The encoded output stream must be represented in lines of no more /// than 76 characters each (see (see [RFC 4880, section diff --git a/openpgp/src/crypto/hash.rs b/openpgp/src/crypto/hash.rs index d0ff1c3d..47220da8 100644 --- a/openpgp/src/crypto/hash.rs +++ b/openpgp/src/crypto/hash.rs @@ -245,7 +245,7 @@ impl<P, R> Hash for Key4<P, R> { /// Update the Hash with a hash of the key. fn hash(&self, hash: &mut Context) { - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; // We hash 8 bytes plus the MPIs. But, the len doesn't // include the tag (1 byte) or the length (2 bytes). @@ -303,7 +303,8 @@ impl Hash for Signature4 { impl Hash for signature::Builder { /// Adds the `Signature` to the provided hash context. fn hash(&self, hash: &mut Context) { - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; + // XXX: Annoyingly, we have no proper way of handling errors // here. let hashed_area = self.hashed_area().to_vec() diff --git a/openpgp/src/crypto/mpis.rs b/openpgp/src/crypto/mpis.rs index ace023d4..51374016 100644 --- a/openpgp/src/crypto/mpis.rs +++ b/openpgp/src/crypto/mpis.rs @@ -14,7 +14,7 @@ use crate::types::{ }; use crate::crypto::hash::{self, Hash}; use crate::crypto::mem::{secure_cmp, Protected}; -use crate::serialize::Serialize; +use crate::serialize::Marshal; use crate::Error; use crate::Result; @@ -896,7 +896,6 @@ impl Arbitrary for Signature { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::Serialize; quickcheck! { fn mpi_roundtrip(mpi: MPI) -> bool { @@ -910,7 +909,6 @@ mod tests { fn pk_roundtrip(pk: PublicKey) -> bool { use std::io::Cursor; use crate::PublicKeyAlgorithm::*; - use crate::serialize::Serialize; let buf = Vec::<u8>::default(); let mut cur = Cursor::new(buf); @@ -969,7 +967,6 @@ mod tests { fn sk_roundtrip(sk: SecretKeyMaterial) -> bool { use std::io::Cursor; use crate::PublicKeyAlgorithm::*; - use crate::serialize::Serialize; let buf = Vec::<u8>::default(); let mut cur = Cursor::new(buf); @@ -1009,7 +1006,6 @@ mod tests { fn ct_roundtrip(ct: Ciphertext) -> bool { use std::io::Cursor; use crate::PublicKeyAlgorithm::*; - use crate::serialize::Serialize; let buf = Vec::<u8>::default(); let mut cur = Cursor::new(buf); @@ -1040,7 +1036,6 @@ mod tests { fn signature_roundtrip(sig: Signature) -> bool { use std::io::Cursor; use crate::PublicKeyAlgorithm::*; - use crate::serialize::Serialize; let buf = Vec::<u8>::default(); let mut cur = Cursor::new(buf); diff --git a/openpgp/src/crypto/s2k.rs b/openpgp/src/crypto/s2k.rs index fc29a7e1..d738a37d 100644 --- a/openpgp/src/crypto/s2k.rs +++ b/openpgp/src/crypto/s2k.rs @@ -290,7 +290,6 @@ mod tests { use crate::SymmetricAlgorithm; use crate::Packet; use crate::parse::{Parse, PacketParser}; - use crate::serialize::Serialize; #[test] fn s2k_parser_test() { @@ -423,7 +422,8 @@ mod tests { quickcheck! { fn s2k_roundtrip(s2k: S2K) -> bool { - use crate::serialize::SerializeInto; + use crate::serialize::Marshal; + use crate::serialize::MarshalInto; eprintln!("in {:?}", s2k); use std::io::Cursor; diff --git a/openpgp/src/packet/key/mod.rs b/openpgp/src/packet/key/mod.rs index 1cf16e08..cd184b64 100644 --- a/openpgp/src/packet/key/mod.rs +++ b/openpgp/src/packet/key/mod.rs @@ -1530,7 +1530,7 @@ pub struct Unencrypted { impl From<mpis::SecretKeyMaterial> for Unencrypted { fn from(mpis: mpis::SecretKeyMaterial) -> Self { - use crate::serialize::Serialize; + use crate::serialize::Marshal; let mut plaintext = Vec::new(); // We need to store the type. plaintext.push( diff --git a/openpgp/src/packet/literal.rs b/openpgp/src/packet/literal.rs index 708a37a4..74123b76 100644 --- a/openpgp/src/packet/literal.rs +++ b/openpgp/src/packet/literal.rs @@ -200,7 +200,7 @@ impl Arbitrary for Literal { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; quickcheck! { fn roundtrip(p: Literal) -> bool { diff --git a/openpgp/src/packet/marker.rs b/openpgp/src/packet/marker.rs index 512cda24..9e1e6718 100644 --- a/openpgp/src/packet/marker.rs +++ b/openpgp/src/packet/marker.rs @@ -55,7 +55,7 @@ impl Arbitrary for Marker { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; #[test] fn roundtrip() { diff --git a/openpgp/src/packet/one_pass_sig.rs b/openpgp/src/packet/one_pass_sig.rs index debb2460..0fc72286 100644 --- a/openpgp/src/packet/one_pass_sig.rs +++ b/openpgp/src/packet/one_pass_sig.rs @@ -197,7 +197,7 @@ impl Arbitrary for OnePassSig3 { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; quickcheck! { fn roundtrip(p: OnePassSig3) -> bool { diff --git a/openpgp/src/packet/pkesk.rs b/openpgp/src/packet/pkesk.rs index 4ab19c8d..ed09bddf 100644 --- a/openpgp/src/packet/pkesk.rs +++ b/openpgp/src/packet/pkesk.rs @@ -207,7 +207,7 @@ mod tests { use crate::PacketPile; use crate::Packet; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; quickcheck! { fn roundtrip(p: PKESK3) -> bool { diff --git a/openpgp/src/packet/signature/subpacket.rs b/openpgp/src/packet/signature/subpacket.rs index b45c74bc..5dfcfa7e 100644 --- a/openpgp/src/packet/signature/subpacket.rs +++ b/openpgp/src/packet/signature/subpacket.rs @@ -77,7 +77,7 @@ use crate::{ Fingerprint, KeyID, SignatureType, - serialize::SerializeInto, + serialize::MarshalInto, }; use crate::types::{ AEADAlgorithm, diff --git a/openpgp/src/packet/skesk.rs b/openpgp/src/packet/skesk.rs index dfe9844e..33055c61 100644 --- a/openpgp/src/packet/skesk.rs +++ b/openpgp/src/packet/skesk.rs @@ -425,7 +425,7 @@ mod test { use super::*; use crate::PacketPile; use crate::parse::Parse; - use crate::serialize::{Serialize, SerializeInto}; + use crate::serialize::{Marshal, MarshalInto}; quickcheck! { fn roundtrip(p: SKESK) -> bool { diff --git a/openpgp/src/packet/trust.rs b/openpgp/src/packet/trust.rs index e4431701..35b45597 100644 --- a/openpgp/src/packet/trust.rs +++ b/openpgp/src/packet/trust.rs @@ -76,7 +76,7 @@ impl Arbitrary for Trust { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; quickcheck! { fn roundtrip(p: Trust) -> bool { diff --git a/openpgp/src/packet/user_attribute.rs b/openpgp/src/packet/user_attribute.rs index cfbe608f..11ea2b0c 100644 --- a/openpgp/src/packet/user_attribute.rs +++ b/openpgp/src/packet/user_attribute.rs @@ -20,8 +20,8 @@ use crate::packet::{ header::BodyLength, }; use crate::Packet; -use crate::serialize::Serialize; -use crate::serialize::SerializeInto; +use crate::serialize::Marshal; +use crate::serialize::MarshalInto; /// Holds a UserAttribute packet. /// @@ -270,7 +270,6 @@ impl Arbitrary for Image { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; quickcheck! { fn roundtrip(p: UserAttribute) -> bool { diff --git a/openpgp/src/packet/userid/mod.rs b/openpgp/src/packet/userid/mod.rs index 4751b981..abc04b9e 100644 --- a/openpgp/src/packet/userid/mod.rs +++ b/openpgp/src/packet/userid/mod.rs @@ -832,7 +832,7 @@ impl Arbitrary for UserID { mod tests { use super::*; use crate::parse::Parse; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; quickcheck! { fn roundtrip(p: UserID) -> bool { diff --git a/openpgp/src/parse/mpis.rs b/openpgp/src/parse/mpis.rs index 0dc6de4b..efc4c893 100644 --- a/openpgp/src/parse/mpis.rs +++ b/openpgp/src/parse/mpis.rs @@ -155,7 +155,7 @@ impl mpis::SecretKeyMaterial { pub fn parse_chksumd<T: Read>(algo: PublicKeyAlgorithm, cur: T) -> Result<Self> { use std::io::Cursor; - use crate::serialize::Serialize; + use crate::serialize::Marshal; // read mpis let bio = buffered_reader::Generic::with_cookie( @@ -483,7 +483,7 @@ impl mpis::Signature { fn mpis_parse_test() { use super::Parse; use crate::PublicKeyAlgorithm::*; - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; // Dummy RSA public key. { diff --git a/openpgp/src/parse/parse.rs b/openpgp/src/parse/parse.rs index c7e8338b..7d899fcc 100644 --- a/openpgp/src/parse/parse.rs +++ b/openpgp/src/parse/parse.rs @@ -1172,7 +1172,7 @@ impl_parse_generic_packet!(Signature); #[test] fn signature_parser_test () { - use crate::serialize::SerializeInto; + use crate::serialize::MarshalInto; let data = crate::tests::message("sig.gpg"); { @@ -1790,7 +1790,7 @@ impl Key4<key::UnspecifiedParts, key::UnspecifiedRole> /// secret subkey packet. fn parse<'a, T: 'a + BufferedReader<Cookie>>(mut php: PacketHeaderParser<T>) -> Result<PacketParser<'a>> { use std::io::Cursor; - use crate::serialize::Serialize; + use crate::serialize::Marshal; make_php_try!(php); let tag = php.header.ctb().tag(); diff --git a/openpgp/src/parse/stream.rs b/openpgp/src/parse/stream.rs index 4b057a22..af64d709 100644 --- a/openpgp/src/parse/stream.rs +++ b/openpgp/src/parse/stream.rs @@ -44,7 +44,7 @@ use crate::{ packet::Signature, cert::prelude::*, crypto::SessionKey, - serialize::Serialize, + serialize::Marshal, policy::Policy, }; use crate::parse::{ diff --git a/openpgp/src/serialize/cert.rs b/openpgp/src/serialize/cert.rs index 689fcfcf..1c47da44 100644 --- a/openpgp/src/serialize/cert.rs +++ b/openpgp/src/serialize/cert.rs @@ -2,19 +2,11 @@ use crate::Result; use crate::cert::prelude::*; use crate::packet::{key, Signature, Tag}; use crate::serialize::{ - PacketRef, Serialize, SerializeInto, + PacketRef, + Marshal, MarshalInto, generic_serialize_into, generic_export_into, }; -impl Serialize for Cert { - fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> { - self.serialize_common(o, false) - } - - fn export(&self, o: &mut dyn std::io::Write) -> Result<()> { - self.serialize_common(o, true) - } -} impl Cert { /// Serializes or exports the Cert. @@ -170,7 +162,21 @@ impl Cert { } } -impl SerializeInto for Cert { +impl crate::serialize::Serialize for Cert {} + +impl Marshal for Cert { + fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> { + self.serialize_common(o, false) + } + + fn export(&self, o: &mut dyn std::io::Write) -> Result<()> { + self.serialize_common(o, true) + } +} + +impl crate::serialize::SerializeInto for Cert {} + +impl MarshalInto for Cert { fn serialized_len(&self) -> usize { let mut l = 0; let primary = self.primary_key(); @@ -521,7 +527,9 @@ impl<'a> TSK<'a> { } } -impl<'a> Serialize for TSK<'a> { +impl<'a> crate::serialize::Serialize for TSK<'a> {} + +impl<'a> Marshal for TSK<'a> { fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> { self.serialize_common(o, false) } @@ -531,7 +539,9 @@ impl<'a> Serialize for TSK<'a> { } } -impl<'a> SerializeInto for TSK<'a> { +impl<'a> crate::serialize::SerializeInto for TSK<'a> {} + +impl<'a> MarshalInto for TSK<'a> { fn serialized_len(&self) -> usize { let mut l = 0; @@ -664,7 +674,6 @@ mod test { use super::*; use crate::vec_truncate; use crate::parse::Parse; - use crate::serialize::Serialize; use crate::packet::key; use crate::policy::StandardPolicy as P; diff --git a/openpgp/src/serialize/cert_armored.rs b/openpgp/src/serialize/cert_armored.rs index 5baaa8b6..32146cec 100644 --- a/openpgp/src/serialize/cert_armored.rs +++ b/openpgp/src/serialize/cert_armored.rs @@ -7,7 +7,8 @@ use crate::cert::{Cert, amalgamation::ValidAmalgamation}; use crate::Result; use crate::types::RevocationStatus; use crate::serialize::{ - Serialize, SerializeInto, generic_serialize_into, generic_export_into, + Marshal, MarshalInto, + generic_serialize_into, generic_export_into, }; use crate::policy::StandardPolicy as P; @@ -83,7 +84,9 @@ impl Cert { /// assert!(armored.contains("Mr. Pink ☮☮☮")); /// # Ok(()) } /// ``` - pub fn armored<'a>(&'a self) -> impl Serialize + SerializeInto + 'a { + pub fn armored<'a>(&'a self) + -> impl crate::serialize::Serialize + crate::serialize::SerializeInto + 'a + { Encoder::new(self) } } @@ -124,7 +127,9 @@ impl<'a> Encoder<'a> { } } -impl<'a> Serialize for Encoder<'a> { +impl<'a> crate::serialize::Serialize for Encoder<'a> {} + +impl<'a> Marshal for Encoder<'a> { fn serialize(&self, o: &mut dyn io::Write) -> Result<()> { self.serialize_common(o, false) } @@ -134,7 +139,9 @@ impl<'a> Serialize for Encoder<'a> { } } -impl<'a> SerializeInto for Encoder<'a> { +impl<'a> crate::serialize::SerializeInto for Encoder<'a> {} + +impl<'a> MarshalInto for Encoder<'a> { fn serialized_len(&self) -> usize { let h = self.cert.armor_headers(); let headers_len = diff --git a/openpgp/src/serialize/mod.rs b/openpgp/src/serialize/mod.rs index da10d7d9..97fe667f 100644 --- a/openpgp/src/serialize/mod.rs +++ b/openpgp/src/serialize/mod.rs @@ -49,7 +49,107 @@ use crate::types::{ const TRACE : bool = false; /// Serializes OpenPGP data structures. -pub trait Serialize { +/// +/// This trait provides the same interface as the `Marshal` trait (in +/// fact, it is just a wrapper around that trait), but only data +/// structures that it makes sense to export implement it. +/// +/// Having a separate trait for data structures that it makes sense to +/// export avoids an easy-to-make and hard-to-debug bug: inadvertently +/// exporting an OpenPGP data structure without any framing +/// information. +/// +/// This bug is easy to make, because Rust infers types, which means +/// that it is often not clear from the immediate context exactly what +/// is being serialized. This bug is hard to debug, because errors +/// parsing data that has been incorrectly exported, are removed from +/// the serialization code. +/// +/// The following example shows how to correctly export a revocation +/// certificate. It should make clear how easy it is to forget to +/// convert a bare signature into an OpenPGP packet before serializing +/// it: +/// +/// ``` +/// # extern crate sequoia_openpgp as openpgp; +/// # use openpgp::Result; +/// use openpgp::cert::prelude::*; +/// use openpgp::Packet; +/// use openpgp::serialize::Serialize; +/// +/// # fn main() { f().unwrap(); } +/// # fn f() -> Result<()> { +/// let (_cert, rev) = +/// CertBuilder::general_purpose(None, Some("alice@example.org")) +/// .generate()?; +/// let rev : Packet = rev.into(); +/// # let output = &mut Vec::new(); +/// rev.serialize(output)?; +/// # Ok(()) +/// # } +/// ``` +/// +/// Note: if you `use` both `Serialize` and `Marshal`, then, because +/// they both have the same methods, and all data structures that +/// implement `Serialize` also implement `Marshal`, you will have to +/// use the Universal Function Call Syntax (UFCS) to call the methods +/// on those objects, for example: +/// +/// ``` +/// # extern crate sequoia_openpgp as openpgp; +/// # use openpgp::Result; +/// # use openpgp::cert::prelude::*; +/// # use openpgp::Packet; +/// # use openpgp::serialize::Serialize; +/// # +/// # fn main() { f().unwrap(); } +/// # fn f() -> Result<()> { +/// # let (_cert, rev) = +/// # CertBuilder::general_purpose(None, Some("alice@example.org")) +/// # .generate()?; +/// # let rev : Packet = rev.into(); +/// # let output = &mut Vec::new(); +/// Serialize::serialize(&rev, output)?; +/// # Ok(()) +/// # } +/// ``` +/// +/// If you really needed `Marshal`, we strongly recommend importing it +/// in as small a scope as possible to avoid this, and to avoid +/// accidentally exporting data without the required framing. +pub trait Serialize : Marshal { + /// Writes a serialized version of the object to `o`. + fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> { + Marshal::serialize(self, o) + } + + /// Exports a serialized version of the object to `o`. + /// + /// This is similar to [`serialize(..)`], with these exceptions: + /// + /// - It is an error to export a [`Signature`] if it is marked + /// as non-exportable. + /// - When exporting a [`Cert`], non-exportable signatures are + /// not exported, and any component bound merely by + /// non-exportable signatures is not exported. + /// + /// [`serialize(..)`]: #tymethod.serialize + /// [`Signature`]: ../packet/enum.Signature.html + /// [`Cert`]: ../struct.Cert.html + fn export(&self, o: &mut dyn std::io::Write) -> Result<()> { + Marshal::export(self, o) + } +} + +/// Serializes OpenPGP data structures. +/// +/// This trait provides the same interface as `Serialize`, but is +/// implemented for all data structures that can be serialized. +/// +/// In general, you should prefer the `Serialize` trait, as it is only +/// implemented for data structures that are normally exported. See +/// the documentation for `Serialize` for more details. +pub trait Marshal { /// Writes a serialized version of the object to `o`. fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()>; @@ -72,7 +172,95 @@ pub trait Serialize { } /// Serializes OpenPGP data structures into pre-allocated buffers. -pub trait SerializeInto { +/// +/// This trait provides the same interface as `MarshalInto`, but is +/// only implemented for data structures that can be serialized. +/// +/// In general, you should prefer this trait to `MarshalInto`, as it +/// is only implemented for data structures that are normally +/// exported. See the documentation for `Serialize` for more details. +pub trait SerializeInto : MarshalInto { + /// Computes the maximal length of the serialized representation. + /// + /// # Errors + /// + /// If serialization would fail, this function underestimates the + /// length. + fn serialized_len(&self) -> usize { + MarshalInto::serialized_len(self) + } + + /// Serializes into the given buffer. + /// + |