additional security sanity checks on pairing messages

2 files changed, 46 insertions, 9 deletions

diff --git a/Assistant/Threads/PairListener.hs b/Assistant/Threads/PairListener.hs
index 09fd9513d..93eef65ba 100644
--- a/Assistant/Threads/PairListener.hs
+++ b/Assistant/Threads/PairListener.hs
@@ -22,6 +22,7 @@ import Utility.Tense
 import Network.Multicast
 import Network.Socket
 import qualified Data.Text as T
+import Data.Char
 
 thisThread :: ThreadName
 thisThread = "PairListener"
@@ -36,16 +37,18 @@ pairListenerThread st dstatus scanremotes urlrenderer = thread $ withSocketsDo $
 		go sock cache = getmsg sock [] >>= \msg -> case readish msg of
 			Nothing -> go sock cache
 			Just m -> do
+				sane <- checkSane msg
 				(pip, verified) <- verificationCheck m
 					=<< (pairingInProgress <$> getDaemonStatus dstatus)
-				case pairMsgStage m of
-					PairReq -> do
+				case (sane, pairMsgStage m) of
+					(False, _) -> go sock cache
+					(_, PairReq) -> do
 						pairReqReceived verified dstatus urlrenderer m
 						go sock $ invalidateCache m cache
-					PairAck -> do
+					(_, PairAck) -> do
 						pairAckReceived verified pip st dstatus scanremotes m cache
 							>>= go sock
-					PairDone -> do
+					(_, PairDone) -> do
 						pairDoneReceived verified pip st dstatus scanremotes m
 						go sock	cache
 
@@ -53,7 +56,8 @@ pairListenerThread st dstatus scanremotes urlrenderer = thread $ withSocketsDo $
 		 - check its UUID against the UUID we have stored. If
 		 - they're the same, someone is sending bogus messages,
 		 - which could be an attempt to brute force the shared
-		 - secret. -}
+		 - secret.
+		 -}
 		verificationCheck m (Just pip) = do
 			let verified = verifiedPairMsg m pip
 			let sameuuid = pairUUID (inProgressPairData pip) == pairUUID (pairMsgData $ m)
@@ -65,6 +69,16 @@ pairListenerThread st dstatus scanremotes urlrenderer = thread $ withSocketsDo $
 					return (Nothing, False)
 				else return (Just pip, verified && sameuuid)
 		verificationCheck _ Nothing = return (Nothing, False)
+		
+		{- Various sanity checks on the content of the message. -}
+		checkSane msg 
+			{- Control characters could be used in a
+			 - console poisoning attack. -}
+			| any isControl msg || any (`elem` "\r\n") msg = do
+				runThreadState st $
+					warning "illegal control characters in pairing message; ignoring"
+				return False
+			| otherwise = return True
 
 		{- PairReqs invalidate the cache of recently finished pairings.
 		 - This is so that, if a new pairing is started with the
diff --git a/doc/design/assistant/pairing.mdwn b/doc/design/assistant/pairing.mdwn
index 388010ca8..fb4f4dd8f 100644
--- a/doc/design/assistant/pairing.mdwn
+++ b/doc/design/assistant/pairing.mdwn
@@ -3,12 +3,23 @@ have some way of pairing devices.
 
 ## security
 
+Pairing uses its own network protocol, built on top of multicast UDP.
+
 It's important that pairing securely verifies that the right host is being
 paired with. This is accomplied by having a shared secret be entered on
-both the hosts that will be paired. They can then construct messages that
-the other host can verify using the shared secret, and so know that,
-for example, the ssh public key it received belongs to the right host
-and has not been altered by a man in the middle.
+both the hosts that will be paired. Hopefully that secret is communicated
+securely out of band. 
+
+(In practice, the security of that communication will vary. To guard against
+interception, each pairing session pairs exactly two hosts and then forgets
+the shared secret. So an attacker who tries to reuse an intercepted secret
+will not succeed in pairing. This does not guard against an intercepted
+secret that is used before the legitimate parties finish pairing.)
+
+Each host can construct messages that the other host can verify using the
+shared secret, and so know that, for example, the ssh public key it
+received belongs to the right host and has not been altered by a man in the
+middle.
 
 The verification works like this: Take a HMAC SHA1 checksum of the message,
 using the shared secret as the HMAC key. Include this checksum after the
@@ -44,6 +55,18 @@ by replaying these:
 
 So replay attacks don't seem to be a problem.
 
+So far I've considered security from a third-party attacker, but either the
+first or second parties in pairing could also be attackers. Presumably they
+trust each other with access to their files as mediated by
+[[git-annex-shell]]. However, one could try to get shell access to the
+other's computer by sending malicious data in a pairing message. So the
+pairing code always checks every data field's content, for example the ssh
+public key is rejected if it looks at all unusual. Any control characters
+in the pairing message cause it to be rejected, to guard against console
+poisoning attacks. Furthermore, git-annex is careful not to expose data to
+the shell, and the webapp uses Yesod's type safety to ensure all user input
+is escaped before going to the browser.
+
 ## TODO
 
 * pairing over IPV6 only networks does not work. Haskell's