Merge pull request #2516 from citusdata/security-audit

Review some strcpy/memcpy/strprintf/sscanf usage in the code.

src/backend/distributed/connection/connection_configuration.c

@@ -260,15 +260,37 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values,
 	/* auth keywords will begin after global and runtime ones are appended */
 	Index authParamsIdx = ConnParams.size + lengthof(runtimeKeywords);
 
+	int paramIndex = 0;
+	int runtimeParamIndex = 0;
+
+	if (ConnParams.size + lengthof(runtimeKeywords) > ConnParams.maxSize)
+	{
+		/* unexpected, intended as developers rather than users */
+		ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						errmsg("too many connParams entries")));
+	}
+
 	pg_ltoa(key->port, nodePortString); /* populate node port string with port */
 
 	/* first step: copy global parameters to beginning of array */
-	memcpy(connKeywords, ConnParams.keywords, ConnParams.size * sizeof(char *));
+	for (paramIndex = 0; paramIndex < ConnParams.size; paramIndex++)
-	memcpy(connValues, ConnParams.values, ConnParams.size * sizeof(char *));
+	{
+		/* copy the keyword&value pointers to the new array */
+		connKeywords[paramIndex] = ConnParams.keywords[paramIndex];
+		connValues[paramIndex] = ConnParams.values[paramIndex];
+	}
 
 	/* second step: begin at end of global params and copy runtime ones */
-	memcpy(&connKeywords[ConnParams.size], runtimeKeywords, sizeof(runtimeKeywords));
+	for (runtimeParamIndex = 0;
-	memcpy(&connValues[ConnParams.size], runtimeValues, sizeof(runtimeValues));
+		 runtimeParamIndex < lengthof(runtimeKeywords);
+		 runtimeParamIndex++)
+	{
+		/* copy the keyword&value pointers to the new array */
+		connKeywords[ConnParams.size + runtimeParamIndex] =
+			(char *) runtimeKeywords[runtimeParamIndex];
+		connValues[ConnParams.size + runtimeParamIndex] =
+			(char *) runtimeValues[runtimeParamIndex];
+	}
 
 	/* final step: add terminal NULL, required by libpq */
 	connKeywords[authParamsIdx] = connValues[authParamsIdx] = NULL;

src/backend/distributed/connection/placement_connection.c

@@ -689,7 +689,7 @@ FindOrCreatePlacementEntry(ShardPlacement *placement)
 			ColocatedPlacementsHashKey key;
 			ColocatedPlacementsHashEntry *colocatedEntry = NULL;
 
-			strcpy(key.nodeName, placement->nodeName);
+			strlcpy(key.nodeName, placement->nodeName, MAX_NODE_LENGTH);
 			key.nodePort = placement->nodePort;
 			key.colocationGroupId = placement->colocationGroupId;
 			key.representativeValue = placement->representativeValue;

src/backend/distributed/relay/relay_event_utility.c

@@ -692,12 +692,12 @@ void
 AppendShardIdToName(char **name, uint64 shardId)
 {
 	char extendedName[NAMEDATALEN];
-	uint32 extendedNameLength = 0;
 	int nameLength = strlen(*name);
 	char shardIdAndSeparator[NAMEDATALEN];
 	int shardIdAndSeparatorLength;
 	uint32 longNameHash = 0;
 	int multiByteClipLength = 0;
+	int neededBytes = 0;
 
 	if (nameLength >= NAMEDATALEN)
 	{
@@ -756,11 +756,20 @@ AppendShardIdToName(char **name, uint64 shardId)
 				 SHARD_NAME_SEPARATOR, longNameHash,
 				 shardIdAndSeparator);
 	}
-	extendedNameLength = strlen(extendedName) + 1;
-	Assert(extendedNameLength <= NAMEDATALEN);
 
-	(*name) = (char *) repalloc((*name), extendedNameLength);
+	(*name) = (char *) repalloc((*name), NAMEDATALEN);
-	snprintf((*name), extendedNameLength, "%s", extendedName);
+	neededBytes = snprintf((*name), NAMEDATALEN, "%s", extendedName);
+	if (neededBytes < 0)
+	{
+		ereport(ERROR, (errcode(ERRCODE_OUT_OF_MEMORY),
+						errmsg("out of memory: %s", strerror(errno))));
+	}
+	else if (neededBytes >= NAMEDATALEN)
+	{
+		ereport(ERROR, (errcode(ERRCODE_STRING_DATA_RIGHT_TRUNCATION),
+						errmsg("new name %s would be truncated at %d characters",
+							   extendedName, NAMEDATALEN)));
+	}
 }
 
 

src/backend/distributed/transaction/remote_transaction.c

@@ -24,6 +24,7 @@
 #include "distributed/transaction_management.h"
 #include "distributed/transaction_recovery.h"
 #include "distributed/worker_manager.h"
+#include "utils/builtins.h"
 #include "utils/hsearch.h"
 
 
@@ -1321,21 +1322,79 @@ Assign2PCIdentifier(MultiConnection *connection)
  * format ParsePreparedTransactionName returns false, and true otherwise.
  */
 bool
-ParsePreparedTransactionName(char *preparedTransactionName, int *groupId, int *procId,
+ParsePreparedTransactionName(char *preparedTransactionName,
-							 uint64 *transactionNumber, uint32 *connectionNumber)
+							 int *groupId, int *procId,
+							 uint64 *transactionNumber,
+							 uint32 *connectionNumber)
 {
-	const int expectedFieldCount = 4;
+	char *currentCharPointer = preparedTransactionName;
-	int parsedFieldCount = 0;
-	bool nameValid = false;
 
-	parsedFieldCount = sscanf(preparedTransactionName, PREPARED_TRANSACTION_NAME_FORMAT,
+	currentCharPointer = strchr(currentCharPointer, '_');
-							  groupId, procId, transactionNumber, connectionNumber);
+	if (currentCharPointer == NULL)
-	if (parsedFieldCount == expectedFieldCount)
 	{
-		nameValid = true;
+		return false;
 	}
 
-	return nameValid;
+	/* step ahead of the current '_' character */
+	++currentCharPointer;
+
+	*groupId = strtol(currentCharPointer, NULL, 10);
+
+	if ((*groupId == 0 && errno == EINVAL) ||
+		(*groupId == INT_MAX && errno == ERANGE))
+	{
+		return false;
+	}
+
+	currentCharPointer = strchr(currentCharPointer, '_');
+	if (currentCharPointer == NULL)
+	{
+		return false;
+	}
+
+	/* step ahead of the current '_' character */
+	++currentCharPointer;
+
+	*procId = strtol(currentCharPointer, NULL, 10);
+	if ((*procId == 0 && errno == EINVAL) ||
+		(*procId == INT_MAX && errno == ERANGE))
+	{
+		return false;
+	}
+
+	currentCharPointer = strchr(currentCharPointer, '_');
+	if (currentCharPointer == NULL)
+	{
+		return false;
+	}
+
+	/* step ahead of the current '_' character */
+	++currentCharPointer;
+
+	*transactionNumber = pg_strtouint64(currentCharPointer, NULL, 10);
+	if ((*transactionNumber == 0 && errno != 0) ||
+		(*transactionNumber == ULLONG_MAX && errno == ERANGE))
+	{
+		return false;
+	}
+
+	currentCharPointer = strchr(currentCharPointer, '_');
+	if (currentCharPointer == NULL)
+	{
+		return false;
+	}
+
+	/* step ahead of the current '_' character */
+	++currentCharPointer;
+
+	*connectionNumber = strtoul(currentCharPointer, NULL, 10);
+	if ((*connectionNumber == 0 && errno == EINVAL) ||
+		(*connectionNumber == UINT_MAX && errno == ERANGE))
+	{
+		return false;
+	}
+
+	return true;
 }