openpgp: Add optional parameters to unknown S2K variants.

  - This mirrors how we handle other unknown variants.  However, since
    we do not know the length of the parameters for unknown S2K
    variants, we cannot parse them back.  To work around that, the
    parameter field is optional, and will be `None` when an unknown
    S2K is parsed.  The data is not lost, but stored in the packet
    containing the S2K object, so that we can serialize it again.

  - Carefully preserve the invariant that we can parse any packet we
    can serialize by comparing the serialized form of the packet
    fragments containing the S2K and any fields the parameters of
    unknown variants bleed into on parsing.

  - Unfortunately, this means that S2K on its own no longer
    roundtrips.  Remove that test accordingly.

6 files changed, 138 insertions, 61 deletions

diff --git a/openpgp/src/crypto/s2k.rs b/openpgp/src/crypto/s2k.rs
index b99e1a56..84b75b02 100644
--- a/openpgp/src/crypto/s2k.rs
+++ b/openpgp/src/crypto/s2k.rs
@@ -76,10 +76,39 @@ pub enum S2K {
         hash: HashAlgorithm
     },
 
-    /// Private S2K algorithm
-    Private(u8),
-    /// Unknown S2K algorithm
-    Unknown(u8),
+    /// Private S2K algorithm.
+    Private {
+        /// Tag identifying the private algorithm.
+        ///
+        /// Tags 100 to 110 are reserved for private use.
+        tag: u8,
+
+        /// The parameters for the private algorithm.
+        ///
+        /// This is optional, because when we parse a packet
+        /// containing an unknown S2K algorithm, we do not know how
+        /// many octets to attribute to the S2K's parameters.  In this
+        /// case, `parameters` is set to `None`.  Note that the
+        /// information is not lost, but stored in the packet.  If the
+        /// packet is serialized again, it is written out.
+        parameters: Option<Box<[u8]>>,
+    },
+
+    /// Unknown S2K algorithm.
+    Unknown {
+        /// Tag identifying the unknown algorithm.
+        tag: u8,
+
+        /// The parameters for the unknown algorithm.
+        ///
+        /// This is optional, because when we parse a packet
+        /// containing an unknown S2K algorithm, we do not know how
+        /// many octets to attribute to the S2K's parameters.  In this
+        /// case, `parameters` is set to `None`.  Note that the
+        /// information is not lost, but stored in the packet.  If the
+        /// packet is serialized again, it is written out.
+        parameters: Option<Box<[u8]>>,
+    },
 
     /// This marks this enum as non-exhaustive.  Do not use this
     /// variant.
@@ -196,7 +225,8 @@ impl S2K {
                                 hash.update(&data[0..tail]);
                             }
                         }
-                        &S2K::Unknown(_) | &S2K::Private(_) => unreachable!(),
+                        S2K::Unknown { .. } | &S2K::Private { .. } =>
+                            unreachable!(),
                         S2K::__Nonexhaustive => unreachable!(),
                     }
 
@@ -206,9 +236,9 @@ impl S2K {
 
                 Ok(ret.into())
             }),
-            &S2K::Unknown(u) | &S2K::Private(u) =>
+            S2K::Unknown { tag, .. } | S2K::Private { tag, .. } =>
                 Err(Error::MalformedPacket(
-                        format!("Unknown S2K type {:#x}", u)).into()),
+                        format!("Unknown S2K type {:#x}", tag)).into()),
             S2K::__Nonexhaustive => unreachable!(),
         }
     }
@@ -323,9 +353,18 @@ impl fmt::Display for S2K {
                     salt[4], salt[5], salt[6], salt[7],
                     hash_bytes))
             }
-            S2K::Private(u) =>
-                f.write_fmt(format_args!("Private/Experimental S2K {}", u)),
-            S2K::Unknown(u) => f.write_fmt(format_args!("Unknown S2K {}", u)),
+            S2K::Private { tag, parameters } =>
+                if let Some(p) = parameters.as_ref() {
+                    write!(f, "Private/Experimental S2K {}:{:?}", tag, p)
+                } else {
+                    write!(f, "Private/Experimental S2K {}", tag)
+                },
+            S2K::Unknown { tag, parameters } =>
+                if let Some(p) = parameters.as_ref() {
+                    write!(f, "Unknown S2K {}:{:?}", tag, p)
+                } else {
+                    write!(f, "Unknown S2K {}", tag)
+                },
             S2K::__Nonexhaustive => unreachable!(),
         }
     }
@@ -335,7 +374,7 @@ impl fmt::Display for S2K {
 impl Arbitrary for S2K {
     fn arbitrary<G: Gen>(g: &mut G) -> Self {
         #[allow(deprecated)]
-        match g.gen_range(0, 5) {
+        match g.gen_range(0, 7) {
             0 => S2K::Simple{ hash: HashAlgorithm::arbitrary(g) },
             1 => S2K::Salted{
                 hash: HashAlgorithm::arbitrary(g),
@@ -346,8 +385,22 @@ impl Arbitrary for S2K {
                 salt: g.gen(),
                 hash_bytes: S2K::nearest_hash_count(g.gen()),
             },
-            3 => S2K::Private(g.gen_range(100, 111)),
-            4 => S2K::Unknown(g.gen_range(4, 100)),
+            3 => S2K::Private {
+                tag: g.gen_range(100, 111),
+                parameters: Option::<Vec<u8>>::arbitrary(g).map(|v| v.into()),
+            },
+            4 => S2K::Unknown {
+                tag: 2,
+                parameters: Option::<Vec<u8>>::arbitrary(g).map(|v| v.into()),
+            },
+            5 => S2K::Unknown {
+                tag: g.gen_range(4, 100),
+                parameters: Option::<Vec<u8>>::arbitrary(g).map(|v| v.into()),
+            },
+            6 => S2K::Unknown {
+                tag: g.gen_range(111, 256) as u8,
+                parameters: Option::<Vec<u8>>::arbitrary(g).map(|v| v.into()),
+            },
             _ => unreachable!(),
         }
     }
@@ -493,29 +546,6 @@ mod tests {
     }
 
     quickcheck! {
-        fn s2k_roundtrip(s2k: S2K) -> bool {
-            use crate::serialize::Marshal;
-            use crate::serialize::MarshalInto;
-
-            eprintln!("in {:?}", s2k);
-            use std::io::Cursor;
-
-            let mut w = Cursor::new(Vec::new());
-            let l = s2k.serialized_len();
-            s2k.serialize(&mut w).unwrap();
-            let buf = w.into_inner();
-            eprintln!("raw: {:?}", buf);
-
-            assert_eq!(buf.len(), l);
-            let mut r = Cursor::new(buf.into_boxed_slice());
-            let s = S2K::from_reader(&mut r).unwrap();
-            eprintln!("out {:?}", s);
-
-            s2k == s
-        }
-    }
-
-    quickcheck! {
         fn s2k_display(s2k: S2K) -> bool {
             let s = format!("{}", s2k);
             !s.is_empty()
@@ -525,8 +555,10 @@ mod tests {
     quickcheck! {
         fn s2k_parse(s2k: S2K) -> bool {
             match s2k {
-                S2K::Unknown(u) => (u > 3 && u < 100) || u == 2 || u > 110,
-                S2K::Private(u) => u >= 100 && u <= 110,
+                S2K::Unknown { tag, .. } =>
+                    (tag > 3 && tag < 100) || tag == 2 || tag > 110,
+                S2K::Private { tag, .. } =>
+                    tag >= 100 && tag <= 110,
                 _ => true
             }
         }
diff --git a/openpgp/src/packet/key.rs b/openpgp/src/packet/key.rs
index 2efe7d54..462c5396 100644
--- a/openpgp/src/packet/key.rs
+++ b/openpgp/src/packet/key.rs
@@ -1404,8 +1404,15 @@ impl PartialEq for Encrypted {
     fn eq(&self, other: &Encrypted) -> bool {
         self.algo == other.algo
             // Treat S2K and ciphertext as opaque blob.
-            && self.s2k == other.s2k
-            && self.raw_ciphertext() == other.raw_ciphertext()
+            && {
+                // XXX: This would be nicer without the allocations.
+                use crate::serialize::MarshalInto;
+                let mut a = self.s2k.to_vec().unwrap();
+                let mut b = other.s2k.to_vec().unwrap();
+                a.extend_from_slice(self.raw_ciphertext());
+                b.extend_from_slice(other.raw_ciphertext());
+                a == b
+            }
     }
 }
 
@@ -1415,8 +1422,11 @@ impl std::hash::Hash for Encrypted {
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
         self.algo.hash(state);
         // Treat S2K and ciphertext as opaque blob.
-        self.s2k.hash(state);
-        self.raw_ciphertext().hash(state);
+        // XXX: This would be nicer without the allocations.
+        use crate::serialize::MarshalInto;
+        let mut a = self.s2k.to_vec().unwrap();
+        a.extend_from_slice(self.raw_ciphertext());
+        a.hash(state);
     }
 }
 
diff --git a/openpgp/src/packet/skesk.rs b/openpgp/src/packet/skesk.rs
index 3f535def..d5ea7d91 100644
--- a/openpgp/src/packet/skesk.rs
+++ b/openpgp/src/packet/skesk.rs
@@ -89,8 +89,15 @@ impl PartialEq for SKESK4 {
         self.version == other.version
             && self.sym_algo == other.sym_algo
             // Treat S2K and ESK as opaque blob.
-            && self.s2k == other.s2k
-            && self.raw_esk() == other.raw_esk()
+            && {
+                // XXX: This would be nicer without the allocations.
+                use crate::serialize::MarshalInto;
+                let mut a = self.s2k.to_vec().unwrap();
+                let mut b = other.s2k.to_vec().unwrap();
+                a.extend_from_slice(self.raw_esk());
+                b.extend_from_slice(other.raw_esk());
+                a == b
+            }
     }
 }
 
@@ -101,8 +108,11 @@ impl std::hash::Hash for SKESK4 {
         self.version.hash(state);
         self.sym_algo.hash(state);
         // Treat S2K and ESK as opaque blob.
-        self.s2k.hash(state);
-        self.raw_esk().hash(state);
+        // XXX: This would be nicer without the allocations.
+        use crate::serialize::MarshalInto;
+        let mut a = self.s2k.to_vec().unwrap();
+        a.extend_from_slice(self.raw_esk());
+        a.hash(state);
     }
 }
 
@@ -337,11 +347,11 @@ impl PartialEq for SKESK5 {
             && self.skesk4.sym_algo == other.skesk4.sym_algo
             && self.aead_digest == other.aead_digest
             // Treat S2K, IV, and ESK as opaque blob.
-            && self.skesk4.s2k == other.skesk4.s2k
             && {
                 // XXX: This would be nicer without the allocations.
-                let mut a = Vec::new();
-                let mut b = Vec::new();
+                use crate::serialize::MarshalInto;
+                let mut a = self.skesk4.s2k.to_vec().unwrap();
+                let mut b = other.skesk4.s2k.to_vec().unwrap();
                 if let Ok(iv) = self.aead_iv() {
                     a.extend_from_slice(iv);
                 }
@@ -363,11 +373,14 @@ impl std::hash::Hash for SKESK5 {
         self.skesk4.sym_algo.hash(state);
         self.aead_digest.hash(state);
         // Treat S2K, IV, and ESK as opaque blob.
-        self.skesk4.s2k.hash(state);
+        // XXX: This would be nicer without the allocations.
+        use crate::serialize::MarshalInto;
+        let mut a = self.skesk4.s2k.to_vec().unwrap();
         if let Some(iv) = self.aead_iv.as_ref() {
-            iv.hash(state);
+            a.extend_from_slice(iv);
         }
-        self.skesk4.raw_esk().hash(state);
+        a.extend_from_slice(self.skesk4.raw_esk());
+        a.hash(state);
     }
 }
 
diff --git a/openpgp/src/parse.rs b/openpgp/src/parse.rs
index 9146e043..a5769120 100644
--- a/openpgp/src/parse.rs
+++ b/openpgp/src/parse.rs
@@ -1071,8 +1071,14 @@ impl S2K {
                 salt: Self::read_salt(php)?,
                 hash_bytes: S2K::decode_count(php.parse_u8("s2k_count")?),
             },
-            100..=110 => S2K::Private(s2k),
-            u => S2K::Unknown(u),
+            100..=110 => S2K::Private {
+                tag: s2k,
+                parameters: None,
+            },
+            u => S2K::Unknown {
+                tag: u,
+                parameters: None,
+            },
         };
 
         Ok(ret)
diff --git a/openpgp/src/serialize.rs b/openpgp/src/serialize.rs
index c2c8b84f..800dceb7 100644
--- a/openpgp/src/serialize.rs
+++ b/openpgp/src/serialize.rs
@@ -1261,8 +1261,12 @@ impl Marshal for S2K {
                 w.write_all(&salt[..])?;
                 w.write_all(&[S2K::encode_count(hash_bytes)?])?;
             }
-            &S2K::Private(s2k) | &S2K::Unknown(s2k) => {
-                w.write_all(&[s2k])?;
+            S2K::Private { tag, parameters }
+            | S2K::Unknown { tag, parameters} => {
+                w.write_all(&[*tag])?;
+                if let Some(p) = parameters.as_ref() {
+                    w.write_all(p)?;
+                }
             }
             S2K::__Nonexhaustive => unreachable!(),
         }
@@ -1278,7 +1282,9 @@ impl MarshalInto for S2K {
             &S2K::Simple{ .. } => 2,
             &S2K::Salted{ .. } => 2 + 8,
             &S2K::Iterated{ .. } => 2 + 8 + 1,
-            &S2K::Private(_) | &S2K::Unknown(_) => 1,
+            S2K::Private { parameters, .. }
+            | S2K::Unknown { parameters, .. } =>
+                1 + parameters.as_ref().map(|p| p.len()).unwrap_or(0),
             S2K::__Nonexhaustive => unreachable!(),
         }
     }
diff --git a/tool/src/commands/dump.rs b/tool/src/commands/dump.rs
index 12752a44..3ecd864f 100644
--- a/tool/src/commands/dump.rs
+++ b/tool/src/commands/dump.rs
@@ -886,10 +886,20 @@ impl PacketDumper {
                 writeln!(output, "{}    Salt: {}", i, hex::encode(salt))?;
                 writeln!(output, "{}    Hash bytes: {}", i, hash_bytes)?;
             },
-            Private(n) =>
-                writeln!(output, "Private({})", n)?,
-            Unknown(n) =>
-                writeln!(output, "Unknown({})", n)?,
+            Private { tag, parameters } => {
+                writeln!(output, "Private")?;
+                writeln!(output, "{}    Tag: {}", i, tag)?;
+                if let Some(p) = parameters.as_ref() {
+                    writeln!(output, "{}    Parameters: {:?}", i, p)?;
+                }
+            },
+            Unknown { tag, parameters } => {
+                writeln!(output, "Unknown")?;
+                writeln!(output, "{}    Tag: {}", i, tag)?;
+                if let Some(p) = parameters.as_ref() {
+                    writeln!(output, "{}    Parameters: {:?}", i, p)?;
+                }
+            },
             __Nonexhaustive => unreachable!(),
         }
         Ok(())