From 34b0151e125a6698f57ea476ccfa922c6275edf1 Mon Sep 17 00:00:00 2001
From: Joey Hess <joeyh@joeyh.name>
Date: Sat, 20 May 2017 15:16:40 -0400
Subject: move unsafe hashing out of instance to avoid misuse

Avoids breaking backwards compat and should avoid future foot-shooting.
---
 Crypto.hs                                          |  2 +-
 Hash.hs                                            | 22 +++++++++++---
 ...ent_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment | 35 ++++++++++++++++++++++
 3 files changed, 54 insertions(+), 5 deletions(-)
 create mode 100644 doc/protocol/comment_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment

diff --git a/Crypto.hs b/Crypto.hs
index efc754f..2fe27e0 100644
--- a/Crypto.hs
+++ b/Crypto.hs
@@ -31,7 +31,7 @@ class Signed t where
 instance Hashable a => Signed (Activity a) where
 	getSignature = activitySignature
 	hashExceptSignature (Activity a mpa mpe mt _s) = hash $
-		Tagged "Activity" [hash a, hash mpa, hash mpe, hash mt]
+		Tagged "Activity" [hash a, hashOfMaybeUnsafe mpa, hashOfMaybeUnsafe mpe, hash mt]
 
 instance Signed Control where
 	getSignature = controlSignature
diff --git a/Hash.hs b/Hash.hs
index 89b0384..cb90c85 100644
--- a/Hash.hs
+++ b/Hash.hs
@@ -41,7 +41,7 @@ instance Hashable a => Hashable (Tagged a) where
 
 instance Hashable a => Hashable (Activity a) where
 	hash (Activity a mps mpe mt s) = hash $ Tagged "Activity"
-		[hash a, hash mps, hash mpe, hash mt, hash s]
+		[hash a, hashOfMaybeUnsafe mps, hashOfMaybeUnsafe mpe, hash mt, hash s]
 
 instance Hashable Entered where
 	hash v = hash $ Tagged "Entered"
@@ -52,7 +52,7 @@ instance Hashable Seen where
 
 instance Hashable ControlAction where
 	hash (EnteredRejected h1 h2) = hash $ Tagged "EnteredRejected"
-		[hash h1, hash h2]
+		[hash h1, hashOfMaybeUnsafe h2]
 	hash (SessionKey pk v) = hash $ Tagged "SessionKey" [hash pk, hash v]
 	hash (SessionKeyAccepted pk) = hash $ Tagged "SessionKeyAccepted" pk
 	hash (SessionKeyRejected pk) = hash $ Tagged "SessionKeyRejected" pk
@@ -83,7 +83,21 @@ instance Hashable ElapsedTime where
 instance Hashable [Hash] where
 	hash = hash . B.concat . map (val . hashValue)
 
--- | Hash empty string for Nothing
+-- | Hash a Maybe Hash, such that 
+--   hash Nothing /= hash (Just (hash (mempty :: B.ByteString)))
 instance Hashable (Maybe Hash) where
+	hash (Just v) = hash (val (hashValue v))
 	hash Nothing = hash (mempty :: B.ByteString)
-	hash (Just v) = hash v
+
+-- | Hash a Maybe Hash using the Hash value as-is, or the hash of the empty
+-- string for Nothing.
+--
+-- Note that this is only safe to use when the input value can't possibly
+-- itself be the hash of an empty string. For example, the hash of an
+-- Activity is safe, because it's the hash of a non-empty string.
+--
+-- This is only used to avoid breaking backwards compatability; the
+-- above instance for Maybe Hash should be used for anything new.
+hashOfMaybeUnsafe :: Maybe Hash -> Hash
+hashOfMaybeUnsafe (Just v) = hash v
+hashOfMaybeUnsafe Nothing = hash (mempty :: B.ByteString)
diff --git a/doc/protocol/comment_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment b/doc/protocol/comment_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment
new file mode 100644
index 0000000..ed1bb32
--- /dev/null
+++ b/doc/protocol/comment_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment
@@ -0,0 +1,35 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 4"""
+ date="2017-05-20T17:53:29Z"
+ content="""
+So the problem comes from the hash
+"cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e",
+-- if that's intended to be a `Maybe Hash` that's the hash of a `ByteString`,
+we can't tell if it was produced by hashing `Nothing`, or hashing 
+`Just (mempty :: ByteString)`
+
+Double hashing would avoid this ambiguity, but it does also break backwards
+compatability of the debug-me protocol and logs. It's still early enough to
+perhaps do that without a great deal of bother, but it's not desirable.
+
+debug-me does not appear to be actually affected by this currently. The only
+`Maybe Hash` in debug-me is used for a hash of values of type `Activity`
+and `Entered`, not the hash of a `ByteString`. So, as far as the debug-me
+protocol goes, the above hash value is unambiguously the hash of `Nothing`;
+there's no `Activity` or `Entered` that hashes to that value. 
+(Barring of course, a cryptographic hash collision which would need SHA2
+to be broken to be exploited.)
+
+So, I'd like to clean this up, to avoid any problems creeping in if
+a `Maybe Hash` got used for the hash of a `ByteString`. But, I don't feel
+it's worth breaking backwards compatibility for.
+
+(I tried adding a phantom type to Hash, so the instance could be only
+for `Maybe (Hash Activity)`, but quickly ran into several complications.)
+
+What I've done is fixed the instance to work like you suggested,
+but kept the old function as `hashOfMaybeUnsafe` and used it where
+necessary. This way, anything new will use the fixed instance and we don't
+break back-compat.
+"""]]
-- 
cgit v1.2.3