Skip to content

Commit e26ad0d

Browse files
wferichrissie-c
authored andcommitted
Make it impossible to truncate or overflow the connection description
It's hard to predict the length of formatted output, so we'd better notice (and abort) if the description is truncated. Incidentally, mkdtemp() does this for us in the shared memory branch, but do an explicit check there as well for consistency, and get rid of the wrongly parametrized strncat() risking a buffer overflow (CONNECTION_DESCRIPTION is not the length of the source "/qb"). Similar truncation checks should be added to qb_ipcs_{shm,us}_connect() where they build the request/response names, and possibly to other places using snprintf().
1 parent 08806c5 commit e26ad0d

File tree

1 file changed

+23
-5
lines changed

1 file changed

+23
-5
lines changed

lib/ipc_setup.c

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -628,6 +628,8 @@ handle_new_connection(struct qb_ipcs_service *s,
628628
int32_t res2 = 0;
629629
uint32_t max_buffer_size = QB_MAX(req->max_msg_size, s->max_buffer_size);
630630
struct qb_ipc_connection_response response;
631+
const char suffix[] = "/qb";
632+
int desc_len;
631633

632634
c = qb_ipcs_connection_alloc(s);
633635
if (c == NULL) {
@@ -654,8 +656,16 @@ handle_new_connection(struct qb_ipcs_service *s,
654656
memset(&response, 0, sizeof(response));
655657

656658
#if defined(QB_LINUX) || defined(QB_CYGWIN)
657-
snprintf(c->description, CONNECTION_DESCRIPTION,
658-
"/dev/shm/qb-%d-%d-%d-XXXXXX", s->pid, ugp->pid, c->setup.u.us.sock);
659+
desc_len = snprintf(c->description, CONNECTION_DESCRIPTION - sizeof suffix,
660+
"/dev/shm/qb-%d-%d-%d-XXXXXX", s->pid, ugp->pid, c->setup.u.us.sock);
661+
if (desc_len < 0) {
662+
res = -errno;
663+
goto send_response;
664+
}
665+
if (desc_len >= CONNECTION_DESCRIPTION - sizeof suffix) {
666+
res = -ENAMETOOLONG;
667+
goto send_response;
668+
}
659669
if (mkdtemp(c->description) == NULL) {
660670
res = -errno;
661671
goto send_response;
@@ -668,10 +678,18 @@ handle_new_connection(struct qb_ipcs_service *s,
668678
(void)chown(c->description, c->auth.uid, c->auth.gid);
669679

670680
/* We can't pass just a directory spec to the clients */
671-
strncat(c->description,"/qb", CONNECTION_DESCRIPTION);
681+
memcpy(c->description + desc_len, suffix, sizeof suffix);
672682
#else
673-
snprintf(c->description, CONNECTION_DESCRIPTION,
674-
"%d-%d-%d", s->pid, ugp->pid, c->setup.u.us.sock);
683+
desc_len = snprintf(c->description, CONNECTION_DESCRIPTION,
684+
"%d-%d-%d", s->pid, ugp->pid, c->setup.u.us.sock);
685+
if (desc_len < 0) {
686+
res = -errno;
687+
goto send_response;
688+
}
689+
if (desc_len >= CONNECTION_DESCRIPTION) {
690+
res = -ENAMETOOLONG;
691+
goto send_response;
692+
}
675693
#endif
676694

677695

0 commit comments

Comments
 (0)