Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
24437a1
Many string command updates to help prevent buffer overflows.
Feb 18, 2018
f2a1077
Update strlcat/strlcpy to strncat/strncpy for portability. Had to bac…
Feb 18, 2018
5c83e0b
Convert strlcat to strncat
Feb 18, 2018
e0beb80
Fix missing parentheses
Feb 18, 2018
d6656ad
Fix whitespace/style
Feb 20, 2018
787235a
Fix strncpy lengths
Feb 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions a2s.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

add_rule(server, "version", buf, 0);

// dedicated
Expand Down Expand Up @@ -373,13 +373,13 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
}

// mod version
sprintf(buf, "%u", swap_long_from_little(pkt));
snprintf(buf, sizeof(buf), "%u", swap_long_from_little(pkt));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

add_rule(server, "mod_ver", buf, 0);
pkt += 4;
pktlen -= 4;

// mod size
sprintf(buf, "%u", swap_long_from_little(pkt));
snprintf(buf, sizeof(buf), "%u", swap_long_from_little(pkt));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

add_rule(server, "mod_size", buf, 0);
pkt += 4;
pktlen -= 4;
Expand All @@ -405,7 +405,7 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
pktlen--;

// Bots
sprintf(buf, "%hhu", *pkt);
snprintf(buf, sizeof(buf), "%hhu", *pkt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

add_rule(server, "bots", buf, 0);
pkt++;
pktlen--;
Expand Down Expand Up @@ -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]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

add_rule(server, "bots", buf, 0);

add_rule(server, "dedicated", pkt[5] ? "1" : "0", 0);
Expand Down Expand Up @@ -524,7 +524,7 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
goto out_too_short;
}
gameport = swap_short_from_little(pkt);
sprintf(buf, "%hu", gameport);
snprintf(buf, sizeof(buf), "%hu", gameport);
add_rule(server, "game_port", buf, 0);
change_server_port(server, gameport, 0);
pkt += 2;
Expand All @@ -547,7 +547,7 @@ deal_with_a2s_packet(struct qserver *server, char *rawpkt, int pktlen)
goto out_too_short;
}
spectator_port = swap_short_from_little(pkt);
sprintf(buf, "%hu", spectator_port);
snprintf(buf, sizeof(buf), "%hu", spectator_port);
add_rule(server, "spectator_port", buf, 0);
pkt += 2;
pktlen -= 2;
Expand Down
17 changes: 10 additions & 7 deletions config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

strncat(path, HOME_CONFIG_FILE, sizeof(path) - 1 - strlen(path));
/* sprintf( path, "%s/%s", var, HOME_CONFIG_FILE); */
rc = try_load_config_file(path, 0);
if ((rc == 0) || (rc == -1)) {
Expand All @@ -210,14 +210,16 @@ qsc_load_default_config_files()
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

path[sizeof(path) -1] = '\0';
filename = path;
#elif defined(_WIN32)
if ((filename == NULL) && _pgmptr && strchr(_pgmptr, '\\')) {
char *slash = strrchr(_pgmptr, '\\');
strncpy(path, _pgmptr, slash - _pgmptr);
path[slash - _pgmptr] = '\0';
strcat(path, "\\qstat.cfg");
strncat(path, "\\qstat.cfg", sizeof(path) -1);
path[sizeof(path) -1] = '\0';
filename = path;
}
#endif
Expand Down Expand Up @@ -556,7 +558,8 @@ get_config_key(char *first_token, const ConfigKey *keys)
char key_name[1024], *token;
int key = 0;

strcpy(key_name, first_token);
strncpy(key_name, first_token, sizeof(key_name) -1);
key_name[sizeof(key_name) -1] = '\0';
do {
int k;
for (k = 0; keys[k].key_name; k++) {
Expand All @@ -576,8 +579,8 @@ get_config_key(char *first_token, const ConfigKey *keys)
REPORT_ERROR((stderr, "Key name too long"));
return (-1);
}
strcat(key_name, " ");
strcat(key_name, token);
strncat(key_name, " ", sizeof(key_name) - 1 - strlen(key_name));
strncat(key_name, token, sizeof(key_name) - 1 - strlen(key_name));
} while (1);

if (key == 0) {
Expand Down
10 changes: 5 additions & 5 deletions crysis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

break;

case 1:
server->challenge++;
password = get_param_value(server, "password", "");
sprintf(cmd, "%s:%s", server->challenge_string, password);
snprintf(cmd, sizeof(cmd), "%s:%s", server->challenge_string, password);
md5 = md5_hex(cmd, strlen(cmd));
sprintf(cmd, "authenticate %s", md5);
snprintf(cmd, sizeof(cmd), "authenticate %s", md5);
free(md5);
break;

Expand All @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

snprintf(cmd, sizeof(cmd), "status");
break;

case 3:
return (DONE_FORCE);
}

server->saved_data.pkt_max = -1;
sprintf(buf, "POST /RPC2 HTTP/1.1\015\012Keep-Alive: 300\015\012User-Agent: qstat %s\015\012Content-Length: %d\015\012Content-Type: text/xml\015\012\015\012<?xml version=\"1.0\" encoding=\"UTF-8\"?><methodCall><methodName>%s</methodName><params /></methodCall>", VERSION, (int)(98 + strlen(cmd)), cmd);
snprintf(buf, sizeof(buf), "POST /RPC2 HTTP/1.1\015\012Keep-Alive: 300\015\012User-Agent: qstat %s\015\012Content-Length: %d\015\012Content-Type: text/xml\015\012\015\012<?xml version=\"1.0\" encoding=\"UTF-8\"?><methodCall><methodName>%s</methodName><params /></methodCall>", VERSION, (int)(98 + strlen(cmd)), cmd);

return (send_packet(server, buf, strlen(buf)));
}
Expand Down
20 changes: 10 additions & 10 deletions cube2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,43 +179,43 @@ deal_with_cube2_packet(struct qserver *server, char *rawpkt, int pktlen)

server->protocol_version = attr[0];

sprintf(buf, "%d %s", attr[0], sb_getversion_s(attr[0]));
snprintf(buf, sizeof(buf), "%d %s", attr[0], sb_getversion_s (attr[0]));
add_rule(server, "version", buf, NO_FLAGS);

sprintf(buf, "%d %s", attr[1], sb_getmode_s(attr[1]));
snprintf(buf, sizeof(buf), "%d %s", attr[1], sb_getmode_s (attr[1]));
add_rule(server, "mode", buf, NO_FLAGS);

sprintf(buf, "%d", attr[2]);
snprintf(buf, sizeof(buf), "%d", attr[2]);
add_rule(server, "seconds_left", buf, NO_FLAGS);

server->max_players = attr[3];

switch (attr[5]) {
case MM_OPEN:
sprintf(buf, "%d open", attr[5]);
snprintf(buf, sizeof(buf), "%d open", attr[5]);
break;

case MM_VETO:
sprintf(buf, "%d veto", attr[5]);
snprintf(buf, sizeof(buf), "%d veto", attr[5]);
break;

case MM_LOCKED:
sprintf(buf, "%d locked", attr[5]);
snprintf(buf, sizeof(buf), "%d locked", attr[5]);
break;

case MM_PRIVATE:
sprintf(buf, "%d private", attr[5]);
snprintf(buf, sizeof(buf), "%d private", attr[5]);
break;

default:
sprintf(buf, "%d unknown", attr[5]);
snprintf(buf, sizeof(buf), "%d unknown", attr[5]);
}
add_rule(server, "mm", buf, NO_FLAGS);

for (i = 0; i < numattr && i < MAX_ATTR; i++) {
char buf2[MAX_STRING];
sprintf(buf, "attr%d", i);
sprintf(buf2, "%d", attr[i]);
snprintf(buf, sizeof(buf), "attr%d", i);
snprintf(buf2, sizeof(buf2), "%d", attr[i]);
add_rule(server, buf, buf2, NO_FLAGS);
}

Expand Down
4 changes: 2 additions & 2 deletions debug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ malformed_packet(const struct qserver *server, const char *fmt, ...)
char fn[PATH_MAX] = { 0 };
int fd;

sprintf(fn, "%03u_%s.pkt", count++, tag);
snprintf(fn, sizeof(fn), "%03u_%s.pkt", count++, tag);
fprintf(stderr, "dumping to %s\n", fn);
fd = open(fn, O_WRONLY | O_CREAT | O_EXCL, 0644);
if (fd == -1) {
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space

a = astart = h + 16 * 2 + 16 / 4 + 2;
for (b = 16; b && i; b--, i--, p++) {
if ((b & 3) == 0) {
Expand Down
2 changes: 1 addition & 1 deletion dirtybomb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ send_dirtybomb_request_packet(struct qserver *server)
chunks |= 0x04; // Player
chunks |= 0x08; // Team - Currently not supported
}
sprintf(buf, "%c%c%s%c", 0x01, len, password, chunks);
snprintf(buf, sizeof(buf), "%c%c%s%c", 0x01, len, password, chunks);

server->saved_data.pkt_max = -1;

Expand Down
2 changes: 1 addition & 1 deletion doom3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ _deal_with_doom3_packet(struct qserver *server, char *rawpkt, int pktlen, unsign
server->max_players = atoi(val);
} else if (0 == strcasecmp(key, "ri_maxViewers")) {
char max[20];
sprintf(max, "%d", server->max_players);
snprintf(max, sizeof(max), "%d", server->max_players);
add_rule(server, "si_maxplayers", max, NO_FLAGS);
server->max_players = atoi(val);
} else if (0 == strcasecmp(key, "ri_numViewers")) {
Expand Down
2 changes: 1 addition & 1 deletion farmsim.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ send_farmsim_request_packet(struct qserver *server)

server->saved_data.pkt_max = -1;
code = get_param_value(server, "code", "");
sprintf(buf, "GET /feed/dedicated-server-stats.xml?code=%s HTTP/1.1\015\012User-Agent: qstat\015\012\015\012", code);
snprintf(buf, sizeof(buf), "GET /feed/dedicated-server-stats.xml?code=%s HTTP/1.1\015\012User-Agent: qstat\015\012\015\012", code);

return (send_packet(server, buf, strlen(buf)));
}
Expand Down
8 changes: 4 additions & 4 deletions fl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized same for all in this function.

add_rule(server, "frametime", buf, 0);

// Round
sprintf(buf, "%hhu", *pkt++);
snprintf(buf, sizeof(buf), "%hhu", *pkt++);
add_rule(server, "round", buf, 0);

// RoundMax
sprintf(buf, "%hhu", *pkt++);
snprintf(buf, sizeof(buf), "%hhu", *pkt++);
add_rule(server, "roundmax", buf, 0);

// RoundSeconds
tmp_short = ((unsigned short)pkt[0] << 8) | ((unsigned short)pkt[1]);
sprintf(buf, "%hu", tmp_short);
snprintf(buf, sizeof(buf), "%hhu", tmp_short);
add_rule(server, "roundseconds", buf, 0);
pkt += 2;

Expand Down
16 changes: 9 additions & 7 deletions gs3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer is correctly sized

strncat(name, val, sizeof(name) - 1 - strlen(name));
server->server_name = name;
}
}
Expand Down Expand Up @@ -381,8 +381,8 @@ process_gs3_packet(struct qserver *server)
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));
strncat(name, val, sizeof(name) - 1 - strlen(name));
server->server_name = name;
}
}
Expand Down Expand Up @@ -643,7 +643,7 @@ process_gs3_packet(struct qserver *server)
case TEAM_OTHER_HEADER:
default:
// add as a server rule
sprintf(rule, "%s%d", header, total_teams);
snprintf(rule, sizeof(rule), "%s%d", header, total_teams);
add_rule(server, rule, val, NO_FLAGS);
break;
}
Expand Down Expand Up @@ -675,8 +675,9 @@ send_gs3_request_packet(struct qserver *server)
server->flags |= TF_PLAYER_QUERY | TF_RULES_QUERY;
if (server->challenge) {
// we've recieved a challenge response, send the query + challenge id
len = sprintf(
len = snprintf(
query_buf,
sizeof(query_buf),
"\xfe\xfd%c\x10\x20\x30\x40%c%c%c%c\xff\xff\xff\x01",
0x00,
(unsigned char)(server->challenge >> 24),
Expand All @@ -694,8 +695,9 @@ send_gs3_request_packet(struct qserver *server)
server->flags |= TF_STATUS_QUERY;
if (server->challenge) {
// we've recieved a challenge response, send the query + challenge id
len = sprintf(
len = snprintf(
query_buf,
sizeof(query_buf),
"\xfe\xfd%c\x10\x20\x30\x40%c%c%c%c\x06\x01\x06\x05\x08\x0a\x04%c%c",
0x00,
(unsigned char)(server->challenge >> 24),
Expand Down
5 changes: 3 additions & 2 deletions haze.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ process_haze_packet(struct qserver *server)
case TEAM_OTHER_HEADER:
default:
// add as a server rule
sprintf(rule, "%s%d", header, total_teams);
snprintf(rule, sizeof(rule), "%s%d", header, total_teams);
add_rule(server, rule, val, NO_FLAGS);
break;
}
Expand Down Expand Up @@ -533,8 +533,9 @@ send_haze_request_packet(struct qserver *server)

if (server->challenge) {
// we've recieved a challenge response, send the query + challenge id
len = sprintf(
len = snprintf(
query_buf,
sizeof(query_buf),
"frdquery%c%c%c%c%c",
(unsigned char)(server->challenge >> 24),
(unsigned char)(server->challenge >> 16),
Expand Down
4 changes: 2 additions & 2 deletions mumble.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

add_rule(server, "version", version, NO_FLAGS);
pkt += 4;

Expand All @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lost space after long

add_rule(server, "allowed_bandwidth", bandwidth, NO_FLAGS);
pkt += 4;

Expand Down
Loading