Refactor challenge creation, no more duplicated memory shenanigans

src/d_clisrv.c

@@ -183,9 +183,8 @@ consvar_t cv_allowguests = CVAR_INIT ("allowguests", "On", CV_SAVE, CV_OnOff, NU
 	consvar_t cv_nochallenge = CVAR_INIT ("nochallenge", "0", 0, CV_Unsigned, NULL);
 	consvar_t cv_badresults = CVAR_INIT ("badresults", "0", 0, CV_Unsigned, NULL);
 	consvar_t cv_noresults = CVAR_INIT ("noresults", "0", 0, CV_Unsigned, NULL);
-	consvar_t cv_badjointime = CVAR_INIT ("badjointime", "0", 0, CV_Unsigned, NULL);
+	consvar_t cv_badtime = CVAR_INIT ("badtime", "0", 0, CV_Unsigned, NULL);
 	consvar_t cv_badip = CVAR_INIT ("badip", "0", 0, CV_Unsigned, NULL);
-	consvar_t cv_badchallengetime = CVAR_INIT ("badchallengetime", "0", 0, CV_Unsigned, NULL);
 #endif
 
 // engine
@@ -220,6 +219,74 @@ consvar_t cv_httpsource = CVAR_INIT ("http_source", "", CV_SAVE, NULL, NULL);
 
 consvar_t cv_kicktime = CVAR_INIT ("kicktime", "10", CV_SAVE, CV_Unsigned, NULL);
 
+// https://github.com/jameds/holepunch/blob/master/holepunch.c#L75
+static int IsExternalAddress (const void *p)
+{
+	const int a = ((const unsigned char*)p)[0];
+	const int b = ((const unsigned char*)p)[1];
+
+	if (*(const int*)p == ~0)/* 255.255.255.255 */
+		return 0;
+
+	switch (a)
+	{
+		case 0:
+		case 10:
+		case 127:
+			return 0;
+		case 172:
+			return (b & ~15) != 16;/* 16 - 31 */
+		case 192:
+			return b != 168;
+		default:
+			return 1;
+	}
+}
+
+
+void GenerateChallenge(uint8_t *buf)
+{
+	time_t now = time(NULL);
+	csprng(buf, sizeof(&buf)); // Random noise so the client can't guess...
+	memcpy(buf, &now, sizeof(now)); // ...timestamp so we can't reuse it...
+	memcpy(buf + sizeof(now), &ourIP, sizeof(ourIP)); // ...and server IP so others can't reuse it.
+
+	#ifdef DEVELOP
+		if (cv_badtime.value)
+		{
+			CV_AddValue(&cv_badtime, -1);
+			CONS_Alert(CONS_WARNING, "cv_badtime enabled, trashing time in auth message\n");
+			memset(buf, 0, sizeof(now));
+		}
+
+		if (cv_badip.value)
+		{
+			CV_AddValue(&cv_badip, -1);
+			CONS_Alert(CONS_WARNING, "cv_badip enabled, trashing IP in auth message\n");
+			memset(buf + sizeof(now), 0, sizeof(ourIP));
+		}
+	#endif
+}
+
+shouldsign_t ShouldSignChallenge(uint8_t *message)
+{
+	time_t then, now;
+	UINT32 claimedIP, realIP;
+
+	now = time(NULL);
+	memcpy(&then, message, sizeof(then));
+	memcpy(&claimedIP, message + sizeof(then), sizeof(claimedIP));
+	realIP = *I_GetNodeAddressInt(servernode);
+
+	if (abs(now - then) > 60*5)
+		return SIGN_BADTIME;
+
+	if (realIP != claimedIP && IsExternalAddress(&realIP))
+		return SIGN_BADIP;
+
+	return SIGN_OK;
+}
+
 static inline void *G_DcpyTiccmd(void* dest, const ticcmd_t* src, const size_t n)
 {
 	const size_t d = n / sizeof(ticcmd_t);
@@ -815,31 +882,6 @@ static boolean CL_AskFileList(INT32 firstfile)
 	return HSendPacket(servernode, false, 0, sizeof (INT32));
 }
 
-// https://github.com/jameds/holepunch/blob/master/holepunch.c#L75
-static int IsExternalAddress (const void *p)
-{
-	const int a = ((const unsigned char*)p)[0];
-	const int b = ((const unsigned char*)p)[1];
-
-	if (*(const int*)p == ~0)/* 255.255.255.255 */
-		return 0;
-
-	switch (a)
-	{
-		case 0:
-		case 10:
-		case 127:
-			return 0;
-		case 172:
-			return (b & ~15) != 16;/* 16 - 31 */
-		case 192:
-			return b != 168;
-		default:
-			return 1;
-	}
-}
-
-
 /** Sends a special packet to declare how many players in local
   * Used only in arbitratrenetstart()
   * Sends a PT_CLIENTJOIN packet to the server
@@ -887,22 +929,23 @@ static boolean CL_SendJoin(void)
 
 	if (client && netgame)
 	{
-		UINT32 claimedIP;
+		shouldsign_t safe = ShouldSignChallenge(awaitingChallenge);
-		UINT32 realIP = *I_GetNodeAddressInt(servernode);
-		time_t receivedTime;
-		time_t now = time(NULL);
 
-		memcpy(&claimedIP, awaitingChallenge, sizeof(claimedIP));
+		if (safe != SIGN_OK)
-		memcpy(&receivedTime, awaitingChallenge + sizeof(claimedIP), sizeof(receivedTime));
-
-		if (realIP != claimedIP && IsExternalAddress(&realIP))
 		{
-			I_Error("External server IP didn't match the message it sent.\nSomething is very wrong here.");
+			if (safe == SIGN_BADIP)
-		}
+			{
-
+				I_Error("External server IP didn't match the message it sent.");
-		if (abs(now - receivedTime) > 60*5)
+			}
-		{
+			else if (safe == SIGN_BADTIME)
-			I_Error("External server sent a message with an unusual timestamp.\nReceived: %ld\nNow: %ld\nCheck your clocks!", receivedTime, now);
+			{
+				I_Error("External server sent a message with an unusual timestamp.\nCheck your clocks!");
+			}
+			else
+			{
+				I_Error("External server asked for a signature on something strange.\nPlease notify a developer if you've seen this more than once.");
+			}
+			return;
 		}
 	}
 
@@ -4342,6 +4385,7 @@ static void HandleConnect(SINT8 node)
 			else 
 			{
 				CONS_Printf("Adding remote. Doing sigcheck for node %d, ID %s\n", node, GetPrettyRRID(lastReceivedKey[node][i], true));
+				
 				if (IsSplitPlayerOnNodeGuest(node, i)) // We're a GUEST and the server throws out our keys anyway.
 				{
 					sigcheck = 0; // Always succeeds. Yes, this is a success response. C R Y P T O
@@ -4356,7 +4400,6 @@ static void HandleConnect(SINT8 node)
 					sigcheck = crypto_eddsa_check(netbuffer->u.clientcfg.challengeResponse[i], lastReceivedKey[node][i], lastSentChallenge[node], 32);
 				}
 
-
 				if (netgame && sigcheck != 0)
 				{
 					SV_SendRefuse(node, M_GetText("Signature verification failed."));
@@ -5320,37 +5363,29 @@ static void HandlePacketFromPlayer(SINT8 node)
 				CL_PrepareDownloadLuaFile();
 			break;
 		case PT_CHALLENGEALL: ; // -Wpedantic
-			if (demo.playback)
+			if (demo.playback || node != servernode) // SERVER should still respond to this to prove its own identity, just not from clients.
 				break;
 
-			if (node != servernode)
-				break;
-			
 			int challengeplayers;
-			time_t now, then;
-			UINT32 claimedIP;
 
 			memcpy(lastChallengeAll, netbuffer->u.challengeall.secret, sizeof(lastChallengeAll));
 
-			now = time(NULL);
+			shouldsign_t safe = ShouldSignChallenge(lastChallengeAll);
-			memcpy(&then, lastChallengeAll, sizeof(then));
 
-			CONS_Printf("Time offset: %d\n", abs(now - then));
+			if (safe != SIGN_OK)
-
-			if (abs(now - then) > 300)
 			{
-				HandleSigfail("Bad challenge - time difference, check clocks");
+				if (safe == SIGN_BADIP)
-				break;
+				{
-			}
+					HandleSigfail("External server sent the wrong IP");
-
+				}
-			memcpy(&claimedIP, lastChallengeAll + sizeof(then), sizeof(claimedIP));
+				else if (safe == SIGN_BADTIME)
-			UINT32 realIP = *I_GetNodeAddressInt(servernode);
+				{
-
+					HandleSigfail("Bad timestamp - check your clocks");
-			CONS_Printf("Got IP %u, known IP %u\n", claimedIP, gamemap);
+				}
-
+				else
-			if (realIP != claimedIP && IsExternalAddress(&realIP))
+				{
-			{
+					HandleSigfail("Unknown auth error - contact a developer");
-				HandleSigfail("Bad challenge - server claimed wrong IP");
+				}
 				break;
 			}
 
@@ -5398,41 +5433,38 @@ static void HandlePacketFromPlayer(SINT8 node)
 			HSendPacket(servernode, true, 0, sizeof(netbuffer->u.responseall));
 			break;
 		case PT_RESPONSEALL:
-			if (demo.playback)
+			if (demo.playback || client)
 				break;
 
-			if (server)
+			int responseplayer;
+			//CONS_Printf("Got PT_RESPONSEALL from node %d, player %d\n", node, nodetoplayer[node]);
+			for (responseplayer = 0; responseplayer < MAXSPLITSCREENPLAYERS; responseplayer++)
 			{
-				int responseplayer;
+				int targetplayer = NodeToSplitPlayer(node, responseplayer);
-				//CONS_Printf("Got PT_RESPONSEALL from node %d, player %d\n", node, nodetoplayer[node]);
+				if (targetplayer == -1)
-				for (responseplayer = 0; responseplayer < MAXSPLITSCREENPLAYERS; responseplayer++)
+					continue;
-				{
-					int targetplayer = NodeToSplitPlayer(node, responseplayer);
-					if (targetplayer == -1)
-						continue;
 
-					if (IsSplitPlayerOnNodeGuest(node, responseplayer))
+				if (IsSplitPlayerOnNodeGuest(node, responseplayer))
+				{
+					CONS_Printf("GUEST on node %d player %d split %d, leaving blank\n", node, targetplayer, responseplayer);
+				}
+				else
+				{
+					CONS_Printf("receiving %s pk %s\n", GetPrettyRRID(lastChallengeAll, true), GetPrettyRRID(players[targetplayer].public_key, true));
+					if (crypto_eddsa_check(netbuffer->u.responseall.signature[responseplayer],
+						players[targetplayer].public_key, lastChallengeAll, sizeof(lastChallengeAll)))
 					{
-						CONS_Printf("GUEST on node %d player %d split %d, leaving blank\n", node, targetplayer, responseplayer);
+						CONS_Alert(CONS_WARNING, "Invalid PT_RESPONSEALL from node %d player %d split %d\n", node, targetplayer, responseplayer);
+						if (playernode[targetplayer] != 0) // NO IDEA.
+						{
+							SendKick(targetplayer, KICK_MSG_SIGFAIL);
+						}
+						break;
 					}
-					else
+					else 
 					{
-						CONS_Printf("receiving %s pk %s\n", GetPrettyRRID(lastChallengeAll, true), GetPrettyRRID(players[targetplayer].public_key, true));
+						memcpy(lastReceivedSignature[targetplayer], netbuffer->u.responseall.signature[responseplayer], sizeof(lastReceivedSignature[targetplayer]));
-						if (crypto_eddsa_check(netbuffer->u.responseall.signature[responseplayer],
+						CONS_Printf("Writing signature %s for node %d player %d split %d\n", GetPrettyRRID(lastReceivedSignature[targetplayer], true), node, targetplayer, responseplayer);
-							players[targetplayer].public_key, lastChallengeAll, sizeof(lastChallengeAll)))
-						{
-							CONS_Alert(CONS_WARNING, "Invalid PT_RESPONSEALL from node %d player %d split %d\n", node, targetplayer, responseplayer);
-							if (playernode[targetplayer] != 0) // NO IDEA.
-							{
-								SendKick(targetplayer, KICK_MSG_SIGFAIL);
-							}
-							break;
-						}
-						else 
-						{
-							memcpy(lastReceivedSignature[targetplayer], netbuffer->u.responseall.signature[responseplayer], sizeof(lastReceivedSignature[targetplayer]));
-							CONS_Printf("Writing signature %s for node %d player %d split %d\n", GetPrettyRRID(lastReceivedSignature[targetplayer], true), node, targetplayer, responseplayer);
-						}
 					}
 				}
 			}
@@ -5444,16 +5476,7 @@ static void HandlePacketFromPlayer(SINT8 node)
 
 			CONS_Printf("Got PT_RESULTSALL\n");
 
-			if (demo.playback)
+			if (demo.playback || server || node != servernode || !expectChallenge)
-				break;
-
-			if (server)
-				break;
-
-			if (node != servernode)
-				break;
-
-			if (!expectChallenge)
 				break;
 
 			CONS_Printf("Checking PT_RESULTSALL\n");
@@ -6335,20 +6358,7 @@ static void UpdateChallenges(void)
 
 			memset(knownWhenChallenged, 0, sizeof(knownWhenChallenged));
 
-			time_t now = time(NULL);
+			GenerateChallenge(netbuffer->u.challengeall.secret);
-			#ifdef DEVELOP
-				if (cv_badchallengetime.value)
-				{
-					CV_AddValue(&cv_badchallengetime, -1);
-					CONS_Alert(CONS_WARNING, "cv_badchallengetime enabled, scrubbing time from PT_CHALLENGEALL\n");
-					now = 0;
-				}
-			#endif
-			CONS_Printf("now: %ld, ip: %u\n", now, ourIP);
-			csprng(netbuffer->u.challengeall.secret, sizeof(netbuffer->u.challengeall.secret)); // Random noise so the client can't guess...
-			memcpy(netbuffer->u.challengeall.secret, &now, sizeof(now)); // ...timestamp...
-			memcpy(netbuffer->u.challengeall.secret + sizeof(now), &ourIP, sizeof(ourIP)); // ...and server IP so the server can't reuse it.
-
 			memcpy(lastChallengeAll, netbuffer->u.challengeall.secret, sizeof(lastChallengeAll));
 
 			memset(lastReceivedSignature, 0, sizeof(lastReceivedSignature));

src/d_clisrv.h

@@ -130,6 +130,13 @@ typedef enum
 	NUMPACKETTYPE
 } packettype_t;
 
+typedef enum
+{
+	SIGN_OK,
+	SIGN_BADTIME,
+	SIGN_BADIP
+} shouldsign_t;
+
 #ifdef PACKETDROP
 void Command_Drop(void);
 void Command_Droprate(void);
@@ -530,15 +537,17 @@ extern consvar_t cv_allowguests;
 	extern consvar_t cv_nochallenge;
 	extern consvar_t cv_badresults;
 	extern consvar_t cv_noresults;
-	extern consvar_t cv_badjointime;
+	extern consvar_t cv_badtime;
 	extern consvar_t cv_badip;
-	extern consvar_t cv_badchallengetime;
 #endif
 
 // Used in d_net, the only dependence
 tic_t ExpandTics(INT32 low, tic_t basetic);
 void D_ClientServerInit(void);
 
+void GenerateChallenge(uint8_t *buf);
+shouldsign_t ShouldSignChallenge(uint8_t *message);
+
 // Initialise the other field
 void RegisterNetXCmd(netxcmd_t id, void (*cmd_f)(UINT8 **p, INT32 playernum));
 void SendNetXCmdForPlayer(UINT8 playerid, netxcmd_t id, const void *param, size_t nparam);

src/d_netcmd.c

@@ -956,9 +956,8 @@ void D_RegisterClientCommands(void)
 		CV_RegisterVar(&cv_nochallenge);
 		CV_RegisterVar(&cv_badresults);
 		CV_RegisterVar(&cv_noresults);
-		CV_RegisterVar(&cv_badjointime);
+		CV_RegisterVar(&cv_badtime);
 		CV_RegisterVar(&cv_badip);
-		CV_RegisterVar(&cv_badchallengetime);
 	#endif
 
 	// HUD

src/d_netfil.c

@@ -1323,30 +1323,8 @@ void PT_ClientKey(INT32 node)
 	CONS_Printf("Got keys from node %d, %s / %s / %s / %s\n", node, GetPrettyRRID(lastReceivedKey[node][0], true), GetPrettyRRID(lastReceivedKey[node][1], true), GetPrettyRRID(lastReceivedKey[node][2], true), GetPrettyRRID(lastReceivedKey[node][3], true));
 
 	netbuffer->packettype = PT_SERVERCHALLENGE;
-	time_t now = time(NULL);
 
-	#ifdef DEVELOP
+	GenerateChallenge(lastSentChallenge[node]);
-		if (cv_badjointime.value)
-		{
-			CV_AddValue(&cv_badjointime, -1);
-			CONS_Alert(CONS_WARNING, "cv_badjointime enabled, scrubbing time from PT_SERVERCHALLENGE\n");
-			now = 0;
-		}
-	#endif
-
-	// Include our IP and current time in the message to be signed, to guard against signature reuse.
-	csprng(lastSentChallenge[node], sizeof(serverchallenge_pak));
-	memcpy(lastSentChallenge[node], &ourIP, sizeof(ourIP));
-	memcpy(lastSentChallenge[node] + sizeof(ourIP), &now, sizeof(time_t));
-
-	#ifdef DEVELOP
-		if (cv_badip.value)
-		{
-			CV_AddValue(&cv_badip, -1);
-			CONS_Alert(CONS_WARNING, "cv_badip enabled, scrubbing IP from PT_SERVERCHALLENGE\n");
-			memset(lastSentChallenge[node], 0, sizeof(ourIP));
-		}
-	#endif
 
 	memcpy(&netbuffer->u.serverchallenge, lastSentChallenge[node], sizeof(serverchallenge_pak));
 	HSendPacket(node, false, 0, sizeof (serverchallenge_pak));