openpgp: Fix serialization of UserAttribute subpackets.

2 files changed, 93 insertions, 20 deletions

diff --git a/openpgp/src/packet/user_attribute.rs b/openpgp/src/packet/user_attribute.rs
index 0e1505d3..03703c8c 100644
--- a/openpgp/src/packet/user_attribute.rs
+++ b/openpgp/src/packet/user_attribute.rs
@@ -266,12 +266,56 @@ mod tests {
 
     quickcheck! {
         fn roundtrip(p: UserAttribute) -> bool {
-            let q = UserAttribute::from_bytes(&p.to_vec().unwrap()).unwrap();
+            let buf = p.to_vec().unwrap();
+            assert_eq!(p.serialized_len(), buf.len());
+            let q = UserAttribute::from_bytes(&buf).unwrap();
             assert_eq!(p, q);
             true
         }
     }
 
+    quickcheck! {
+        fn roundtrip_subpacket(sp: Subpacket) -> bool {
+            let value = sp.to_vec().unwrap();
+            assert_eq!(sp.serialized_len(), value.len());
+            let ua = UserAttribute {
+                common: Default::default(),
+                value,
+            };
+            let buf = ua.to_vec().unwrap();
+            let q = UserAttribute::from_bytes(&buf).unwrap();
+            let subpackets = q.subpackets().collect::<Vec<_>>();
+            assert_eq!(subpackets.len(), 1);
+            assert_eq!(&sp, subpackets[0].as_ref().unwrap());
+            true
+        }
+    }
+
+    quickcheck! {
+        fn roundtrip_image(img: Image) -> bool {
+            let mut body = img.to_vec().unwrap();
+            assert_eq!(img.serialized_len(), body.len());
+            let mut value =
+                BodyLength::Full(1 + body.len() as u32).to_vec().unwrap();
+            value.push(1); // Image subpacket tag.
+            value.append(&mut body);
+            let ua = UserAttribute {
+                common: Default::default(),
+                value,
+            };
+            let buf = ua.to_vec().unwrap();
+            let q = UserAttribute::from_bytes(&buf).unwrap();
+            let subpackets = q.subpackets().collect::<Vec<_>>();
+            assert_eq!(subpackets.len(), 1);
+            if let Ok(Subpacket::Image(i)) = &subpackets[0] {
+                assert_eq!(&img, i);
+            } else {
+                panic!("expected image subpacket, got {:?}", subpackets[0]);
+            }
+            true
+        }
+    }
+
     #[test]
     fn image() {
         use crate::Packet;
@@ -300,11 +344,27 @@ ABQQAQAAAAAAAAAAAAAAAAAAAAD/2gAIAQEAAT8Qf//Z
         };
         assert_eq!(subpackets.len(), 1);
         if let Ok(Subpacket::Image(Image::JPEG(img))) = &subpackets[0] {
-            assert_eq!(img.len(), 539);
+            assert_eq!(img.len(), 539 /* Image data */);
             assert_eq!(&img[6..10], b"JFIF");
             assert_eq!(&img[24..41], b"Created with GIMP");
         } else {
             panic!("Expected JPEG, got {:?}", &subpackets[0]);
         }
+
+        if let Ok(Subpacket::Image(img)) = &subpackets[0] {
+            let buf = img.to_vec().unwrap();
+            assert_eq!(buf.len(), 539 + 16 /* Image header */);
+            assert_eq!(img.serialized_len(), 539 + 16 /* Image header */);
+        } else {
+            unreachable!("decomposed fine before");
+        }
+
+        if let Ok(img) = &subpackets[0] {
+            let buf = img.to_vec().unwrap();
+            assert_eq!(buf.len(), 539 + 16 + 3 /* Subpacket header */);
+            assert_eq!(img.serialized_len(), 539 + 16 + 3 /* Subpacket header */);
+        } else {
+            unreachable!("decomposed fine before");
+        }
     }
 }
diff --git a/openpgp/src/serialize.rs b/openpgp/src/serialize.rs
index 3b04a085..7e49fbb2 100644
--- a/openpgp/src/serialize.rs
+++ b/openpgp/src/serialize.rs
@@ -1938,12 +1938,19 @@ impl MarshalInto for UserAttribute {
 
 impl Marshal for user_attribute::Subpacket {
     fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> {
-        match self {
+        let body_len = match self {
             user_attribute::Subpacket::Image(image) =>
-                image.serialize(o)?,
+                image.serialized_len(),
+            user_attribute::Subpacket::Unknown(_tag, data) =>
+                data.len(),
+        };
+        BodyLength::Full(1 + body_len as u32).serialize(o)?;
+        match self {
+            user_attribute::Subpacket::Image(image) => {
+                write_byte(o, 1)?;
+                image.serialize(o)?;
+            },
             user_attribute::Subpacket::Unknown(tag, data) => {
-                BodyLength::Full(1 + data.len() as u32)
-                    .serialize(o)?;
                 write_byte(o, *tag)?;
                 o.write_all(&data[..])?;
             }
@@ -1955,15 +1962,15 @@ impl Marshal for user_attribute::Subpacket {
 
 impl MarshalInto for user_attribute::Subpacket {
     fn serialized_len(&self) -> usize {
-        match self {
+        let body_len = match self {
             user_attribute::Subpacket::Image(image) =>
                 image.serialized_len(),
-            user_attribute::Subpacket::Unknown(_tag, data) => {
-                let header_len = BodyLength::Full(1 + data.len() as u32)
-                    .serialized_len();
-                header_len + 1 + data.len()
-            }
-        }
+            user_attribute::Subpacket::Unknown(_tag, data) =>
+                data.len(),
+        };
+        let header_len =
+            BodyLength::Full(1 + body_len as u32).serialized_len();
+        header_len + 1 + body_len
     }
 
     fn serialize_into(&self, buf: &mut [u8]) -> Result<usize> {
@@ -1973,18 +1980,20 @@ impl MarshalInto for user_attribute::Subpacket {
 
 impl Marshal for user_attribute::Image {
     fn serialize(&self, o: &mut dyn std::io::Write) -> Result<()> {
+        const V1HEADER_TOP: [u8; 3] = [0x10, 0x00, 0x01];
+        const V1HEADER_PAD: [u8; 12] = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];
         match self {
             user_attribute::Image::JPEG(data) => {
-                let header = BodyLength::Full(1 + data.len() as u32);
-                header.serialize(o)?;
-                write_byte(o, 0)?;
+                o.write_all(&V1HEADER_TOP[..])?;
+                write_byte(o, 1)?;
+                o.write_all(&V1HEADER_PAD[..])?;
                 o.write_all(&data[..])?;
             }
             user_attribute::Image::Unknown(tag, data)
             | user_attribute::Image::Private(tag, data) => {
-                let header = BodyLength::Full(1 + data.len() as u32);
-                header.serialize(o)?;
+                o.write_all(&V1HEADER_TOP[..])?;
                 write_byte(o, *tag)?;
+                o.write_all(&V1HEADER_PAD[..])?;
                 o.write_all(&data[..])?;
             }
         }
@@ -1995,12 +2004,16 @@ impl Marshal for user_attribute::Image {
 
 impl MarshalInto for user_attribute::Image {
     fn serialized_len(&self) -> usize {
+        const V1HEADER_LEN: usize =
+            2     /* Length */
+            + 1   /* Version */
+            + 1   /* Tag */
+            + 12; /* Reserved padding */
         match self {
             user_attribute::Image::JPEG(data)
             | user_attribute::Image::Unknown(_, data)
             | user_attribute::Image::Private(_, data) =>
-                1 + BodyLength::Full(1 + data.len() as u32).serialized_len()
-                + data.len(),
+                V1HEADER_LEN + data.len(),
         }
     }