diff options
author | Joey Hess <joeyh@joeyh.name> | 2017-05-20 15:16:40 -0400 |
---|---|---|
committer | Joey Hess <joeyh@joeyh.name> | 2017-05-20 15:16:40 -0400 |
commit | 34b0151e125a6698f57ea476ccfa922c6275edf1 (patch) | |
tree | c4c7f57421ae1e7b87bb0f8b82ff97a1cec93222 | |
parent | 2e16195d151d401a664fa929604413aa613aa9f5 (diff) | |
download | debug-me-34b0151e125a6698f57ea476ccfa922c6275edf1.tar.gz |
move unsafe hashing out of instance to avoid misuse
Avoids breaking backwards compat and should avoid future foot-shooting.
-rw-r--r-- | Crypto.hs | 2 | ||||
-rw-r--r-- | Hash.hs | 22 | ||||
-rw-r--r-- | doc/protocol/comment_4_6c6cd957b3e4db5b77f87b13c4e35e6b._comment | 35 |
3 files changed, 54 insertions, 5 deletions
@@ -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 @@ -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. +"""]] |