Many string command updates to help prevent buffer overflows. #72

tdm4 · 2018-02-18T19:23:06Z

OK here's a pretty big pull request. I rewrote many of the strings functions to help prevent overflows. There are still some using the older functions that did pointer arithmetic that I didn't feel comfortable changing.
I tested all the Quake 1 servers as mentioned in Issue #12 and none of them do a buffer overflow, however I think this will need quite a bit of testing.

tdm4 · 2018-02-18T19:42:49Z

Sorry, realized Linux doesn't have strlcpy or strlcat. I'll have to fix this up.

…k up some conversions though.

stevenh

Thanks for taking the time to do this, I’ve not had chance to review in full yet but there does seem to be number of places where you have introduced leading and training spaces around the braces for function calls.

It we be good if you’re you could fix those before we do a full review 😊

tdm4 · 2018-02-20T09:41:03Z

@stevenh - I did some of them for readability, but if there is a particular style you prefer, if you could give me an example I can revert the change back.

stevenh · 2018-02-20T11:49:22Z

a2s.c

@@ -312,7 +312,7 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
 		server->max_players = (unsigned char)pkt[1];

 		// version
-		sprintf(buf, "%hhu", pkt[2]);
+		snprintf( buf, sizeof(buf), "%hhu", pkt[2] );


This is an example of the style we don't use. It should instead be:

snprintf(buf, sizeof(buf), "%hhu", pkt[2]);

stevenh · 2018-02-20T16:13:10Z

config.c

@@ -210,14 +210,16 @@ qsc_load_default_config_files()
 	}

 #ifdef sysconfdir
-		strcpy(path, sysconfdir "/qstat.cfg");
+		strncpy(path, sysconfdir "/qstat.cfg", sizeof(path) -1);


strncpy doesn't require manual termination, remove the -1 and it will do the right thing.

The stpncpy() and strncpy() functions copy at most len characters from src into dst. If src is less than len characters long, the remainder of dst is filled with `\0' characters. Otherwise, dst is not terminated.

Actually, it doesn't always terminate.

strncpy() only NUL terminates the destination string when the length of the source string is less than the length parameter.

source: https://man.openbsd.org/strncpy#DESCRIPTION

The manual termination is a pattern described in the EXAMPLES section for platforms without strlcpy.

https://man.openbsd.org/strncpy#EXAMPLES

stevenh · 2018-02-22T15:09:18Z

crysis.c

@@ -46,15 +46,15 @@ send_crysis_request_packet(struct qserver *server)
 	case 0:
 		// Not seen a challenge yet, request it
 		server->challenge++;
-		sprintf(cmd, "challenge");
+		snprintf(cmd, sizeof(cmd), "challenge");


buffer is correctly sized

stevenh · 2018-02-20T16:24:28Z

debug.c

@@ -171,7 +171,7 @@ output_packet(struct qserver *server, const char *buf, int buflen, int to)
 	for (i = buflen; i; offset += 16) {
 		memset(line, ' ', 256);
 		h = 0;
-		h += sprintf(line, "%5d:", offset);
+		h+= snprintf(line, sizeof(line), "%5d:", offset);


missing space

stevenh · 2018-02-20T16:25:35Z

mumble.c

@@ -43,7 +43,7 @@ deal_with_mumble_packet(struct qserver *server, char *rawpkt, int pktlen)

 	// version
 	server->protocol_version = ntohl(*(unsigned long *)pkt);
-	sprintf(version, "%u.%u.%u", (unsigned char)*pkt + 1, (unsigned char)*pkt + 2, (unsigned char)*pkt + 3);
+	snprintf(version,sizeof(version),"%u.%u.%u", (unsigned char)*pkt+1, (unsigned char)*pkt+2, (unsigned char)*pkt+3);


missing space after comma and removal of spaces before and after +

stevenh · 2018-02-20T16:26:20Z

mumble.c

@@ -59,7 +59,7 @@ deal_with_mumble_packet(struct qserver *server, char *rawpkt, int pktlen)
 	pkt += 4;

 	// allowed bandwidth
-	sprintf(bandwidth, "%d", ntohl(*(unsigned long *)pkt));
+	snprintf(bandwidth, sizeof(bandwidth), "%d", ntohl(*(unsigned long*)pkt));


lost space after long

stevenh · 2018-02-20T16:27:06Z

qstat.c

@@ -471,19 +471,20 @@ display_q_player_info(struct qserver *server)
 	char fmt[128];
 	struct player *player;

-	strcpy(fmt, "\t#%-2d %3d frags %9s ");
+	strncpy(fmt, "\t#%-2d %3d frags %9s ", sizeof(fmt) -1);


no need to manual terminate, check for more of these.

stevenh · 2018-02-20T16:30:24Z

qstat.c

@@ -6732,7 +6743,8 @@ deal_with_qwmaster_packet(struct qserver *server, char *rawpkt, int pktlen)
 		unsigned short port = htons(*((unsigned short *)(rawpkt + pktlen - 2)));

 		//fprintf( stderr, "NEXT IP=%s:%u\n", ip, port );
-		sprintf(server->master_query_tag, "%s:%u", ip, port);
+		snprintf(server->master_query_tag, sizeof(server->master_query_tag),


we're not precious about line length, so no need to wrap, more below

stevenh · 2018-02-20T16:39:18Z

For reference you can check formatting with uncrustify, not sure it will catch all the nits though.

Fix whitespace Run files through uncrustify

stevenh

Having gone through this more closely its adding a lot of clutter which for the most part is unneeded as the buffers are correctly sized.

In addition its actually introduced some bugs in a number of places e.g. sizeof on char* and I may well have missed some due to the size of the patch.

Given these I'm thinking we should only fix the known issues instead of a global replace like this.

stevenh · 2018-02-22T15:07:07Z

config.c

@@ -200,8 +200,8 @@ qsc_load_default_config_files()
 		}
 		strncpy(path, var, len);
 		path[len] = '\0';
-		strcat(path, "/");
-		strcat(path, HOME_CONFIG_FILE);
+		strncat(path, "/", sizeof(path) - 1 - strlen(path));


If this truncates the length and doesn't null terminate then it will break anyway, so I concerned this just adds needless and broken complexity.

It should be noted there is actually no possible buffer overrun here as its protected against by the len check above.

stevenh · 2018-02-22T15:08:52Z

config.c

@@ -210,14 +210,16 @@ qsc_load_default_config_files()
 	}

 #ifdef sysconfdir
-		strcpy(path, sysconfdir "/qstat.cfg");
+		strncpy(path, sysconfdir "/qstat.cfg", sizeof(path));


similar to above, the better fix would be to use malloc'ed buffer here

stevenh · 2018-02-22T15:09:18Z

crysis.c

@@ -46,15 +46,15 @@ send_crysis_request_packet(struct qserver *server)
 	case 0:
 		// Not seen a challenge yet, request it
 		server->challenge++;
-		sprintf(cmd, "challenge");
+		snprintf(cmd, sizeof(cmd), "challenge");


buffer is correctly sized

stevenh · 2018-02-22T15:10:51Z

qstat.c

@@ -471,19 +471,20 @@ display_q_player_info(struct qserver *server)
 	char fmt[128];
 	struct player *player;

-	strcpy(fmt, "\t#%-2d %3d frags %9s ");
+	strncpy(fmt, "\t#%-2d %3d frags %9s ", sizeof(fmt));


none of the changes in display_q_player_info are needed as the buffer is big enough, so its just adding needless complexity.

stevenh · 2018-02-22T15:11:35Z

qstat.c

@@ -506,14 +507,15 @@ display_qw_player_info(struct qserver *server)
 	char fmt[128];
 	struct player *player;

-	strcpy(fmt, "\t#%-6d %5d frags %6s@%-5s %8s");


none of the changes in display_qw_player_info are needed as the buffer is big enough, so its just adding needless complexity.

stevenh · 2018-02-22T15:40:38Z

a2s.c

@@ -474,7 +474,7 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
 		server->num_players = (unsigned char)pkt[2];
 		server->max_players = (unsigned char)pkt[3];
 		// pkt[4] number of bots
-		sprintf(buf, "%hhu", pkt[4]);
+		snprintf(buf, sizeof(buf), "%hhu", pkt[4]);


buffer is correctly sized

stevenh · 2018-02-22T15:43:42Z

crysis.c

@@ -63,15 +63,15 @@ send_crysis_request_packet(struct qserver *server)
 		server->challenge++;
 		server->flags |= TF_STATUS_QUERY;
 		server->n_servers = 3;
-		sprintf(cmd, "status");


buffer is correctly sized

stevenh · 2018-02-22T15:51:46Z

debug.c

@@ -171,7 +171,7 @@ output_packet(struct qserver *server, const char *buf, int buflen, int to)
 	for (i = buflen; i; offset += 16) {
 		memset(line, ' ', 256);
 		h = 0;
-		h += sprintf(line, "%5d:", offset);
+		h += snprintf(line, sizeof(line), "%5d:", offset);


buffer is correctly sized

stevenh · 2018-02-22T15:52:46Z

fl.c

@@ -340,20 +340,20 @@ deal_with_fl_packet(struct qserver *server, char *rawpkt, int pktlen)
 		add_rule(server, "passworded", (*pkt++) ? "1" : "0", 0);

 		// FrameTime
-		sprintf(buf, "%hhu", *pkt++);
+		snprintf(buf, sizeof(buf), "%hhu", *pkt++);


buffer is correctly sized same for all in this function.

stevenh · 2018-02-22T15:53:24Z

gs3.c

@@ -229,8 +229,8 @@ deal_with_gs3_status(struct qserver *server, char *rawpkt, int pktlen)
 						if (server->server_name) {
 							char *name = (char *)realloc(server->server_name, strlen(server->server_name) + strlen(val) + 3);
 							if (name) {
-								strcat(name, ": ");
-								strcat(name, val);
+								strncat(name, ": ", sizeof(name) - 1 - strlen(name));


buffer is correctly sized

stevenh · 2018-05-04T08:01:37Z

While this seemed like a good idea on closer review its adding lots of churn and introducing bugs, so I think we need to address individual issues as they are encountered. That and the lack of feedback I'm going to close this for now.

Many string command updates to help prevent buffer overflows.

24437a1

Tom Murphy added 3 commits February 18, 2018 19:54

Update strlcat/strlcpy to strncat/strncpy for portability. Had to bac…

f2a1077

…k up some conversions though.

Convert strlcat to strncat

5c83e0b

Fix missing parentheses

e0beb80

stevenh suggested changes Feb 19, 2018

View reviewed changes

stevenh reviewed Feb 20, 2018

View reviewed changes

Fix whitespace/style

d6656ad

stevenh suggested changes Feb 20, 2018

View reviewed changes

Fix strncpy lengths

787235a

Fix whitespace Run files through uncrustify

stevenh suggested changes Feb 22, 2018

View reviewed changes

stevenh closed this May 4, 2018

tdm4 deleted the String_function_updates branch June 30, 2018 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Many string command updates to help prevent buffer overflows. #72

Many string command updates to help prevent buffer overflows. #72

tdm4 commented Feb 18, 2018

tdm4 commented Feb 18, 2018

stevenh left a comment

tdm4 commented Feb 20, 2018

stevenh Feb 20, 2018

stevenh Feb 20, 2018

mulander Feb 20, 2018

stevenh Feb 22, 2018

stevenh Feb 20, 2018

stevenh Feb 20, 2018

stevenh Feb 20, 2018

stevenh Feb 20, 2018

stevenh Feb 20, 2018

stevenh commented Feb 20, 2018

stevenh left a comment

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh Feb 22, 2018

stevenh commented May 4, 2018

Many string command updates to help prevent buffer overflows. #72

Many string command updates to help prevent buffer overflows. #72

Conversation

tdm4 commented Feb 18, 2018

tdm4 commented Feb 18, 2018

stevenh left a comment

Choose a reason for hiding this comment

tdm4 commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh commented Feb 20, 2018

stevenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh commented May 4, 2018