From: Omar Polo Subject: misc tweaks for privsep.c To: gameoftrees@openbsd.org Date: Tue, 14 Jun 2022 11:59:22 +0200 after noticing some missing checks on the data received from the libexec helpers in got patch I decided to do some further checks around every got_privsep_recv_imsg in the rest of got. (in doing so I've noticed that patch is the only got subsystem to receive imsgs outside of privsep.c) what follows are some misc tweaks, but sending three/four different mails didn't seemed ideal so I'm bundling everything together. there are a couple of purely stylistic changes (like not checking datalen before recv_imsg_error or using strndup instead of a custom malloc + memcpy), some paranoid bounds checks and a genuine fix (got_privsep_recv_object_idlist/reused_deltas have a typo and are using the pointer size instead of the object size, but we're lucky and those are equal in this case.) I'm pretty confident about most changes, but still not completely sure about all (in particular the bounds check on the index progress and on the pack offsets.) One possible issue of my malloc+memcpy -> strndup change is that if an author/committer name has a NUL byte in it the old code will preserve it, the new don't. Probably isn't an issue but thought of pointing it out. The outcome is a shorter privsep.c and hopefully not more bugs :) diff 5b7126c502f8d046d0779dd2cb0558163c383819 /home/op/w/got blob - a63073982fd405c8f986e41f79764f7b0f74ee7c file + lib/privsep.c --- lib/privsep.c +++ lib/privsep.c @@ -355,7 +355,8 @@ got_privsep_recv_raw_obj(uint8_t **outbuf, off_t *size break; } - if (*size + *hdrlen > GOT_PRIVSEP_INLINE_OBJECT_DATA_MAX) { + if (*size < 0 || + *size + *hdrlen > GOT_PRIVSEP_INLINE_OBJECT_DATA_MAX) { err = got_error(GOT_ERR_PRIVSEP_LEN); break; } @@ -702,10 +703,6 @@ got_privsep_recv_fetch_progress(int *done, struct got_ datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_FETCH_SYMREFS: @@ -930,10 +927,6 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_ datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - goto done; - } err = recv_imsg_error(&imsg, datalen); goto done; case GOT_IMSG_SEND_REMOTE_REF: @@ -1017,10 +1010,6 @@ got_privsep_recv_send_progress(int *done, off_t *bytes datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_SEND_UPLOAD_PROGRESS: @@ -1104,10 +1093,6 @@ got_privsep_recv_index_progress(int *done, int *nobj_t datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_IDXPACK_PROGRESS: @@ -1116,6 +1101,11 @@ got_privsep_recv_index_progress(int *done, int *nobj_t break; } iprogress = (struct got_imsg_index_pack_progress *)imsg.data; + if (iprogress->nobj_total < 0 || iprogress->nobj_indexed < 0 || + iprogress->nobj_loose < 0 || iprogress->nobj_resolved < 0) { + err = got_error(GOT_ERR_PRIVSEP_LEN); + break; + } *nobj_total = iprogress->nobj_total; *nobj_indexed = iprogress->nobj_indexed; *nobj_loose = iprogress->nobj_loose; @@ -1148,6 +1138,9 @@ got_privsep_get_imsg_obj(struct got_object **obj, stru return got_error(GOT_ERR_PRIVSEP_LEN); iobj = imsg->data; + if (iobj->pack_offset < 0) + return got_error(GOT_ERR_PRIVSEP_LEN); + *obj = calloc(1, sizeof(**obj)); if (*obj == NULL) return got_error_from_errno("calloc"); @@ -1317,40 +1310,18 @@ get_commit_from_imsg(struct got_commit_object **commit (*commit)->committer_time = icommit->committer_time; (*commit)->committer_gmtoff = icommit->committer_gmtoff; - if (icommit->author_len == 0) { - (*commit)->author = strdup(""); - if ((*commit)->author == NULL) { - err = got_error_from_errno("strdup"); - goto done; - } - } else { - (*commit)->author = malloc(icommit->author_len + 1); - if ((*commit)->author == NULL) { - err = got_error_from_errno("malloc"); - goto done; - } - memcpy((*commit)->author, imsg->data + len, - icommit->author_len); - (*commit)->author[icommit->author_len] = '\0'; + (*commit)->author = strndup(imsg->data + len, icommit->author_len); + if ((*commit)->author == NULL) { + err = got_error_from_errno("strndup"); + goto done; } len += icommit->author_len; - if (icommit->committer_len == 0) { - (*commit)->committer = strdup(""); - if ((*commit)->committer == NULL) { - err = got_error_from_errno("strdup"); - goto done; - } - } else { - (*commit)->committer = - malloc(icommit->committer_len + 1); - if ((*commit)->committer == NULL) { - err = got_error_from_errno("malloc"); - goto done; - } - memcpy((*commit)->committer, imsg->data + len, - icommit->committer_len); - (*commit)->committer[icommit->committer_len] = '\0'; + (*commit)->committer = strndup(imsg->data + len, + icommit->committer_len); + if ((*commit)->committer == NULL) { + err = got_error_from_errno("strndup"); + goto done; } len += icommit->committer_len; @@ -1776,7 +1747,8 @@ got_privsep_recv_blob(uint8_t **outbuf, size_t *size, break; } - if (*size > GOT_PRIVSEP_INLINE_BLOB_DATA_MAX) { + if (*size > GOT_PRIVSEP_INLINE_BLOB_DATA_MAX || + *size > datalen + sizeof(*iblob)) { err = got_error(GOT_ERR_PRIVSEP_LEN); break; } @@ -1917,21 +1889,10 @@ got_privsep_recv_tag(struct got_tag_object **tag, stru memcpy((*tag)->id.sha1, itag->id, SHA1_DIGEST_LENGTH); - if (itag->tag_len == 0) { - (*tag)->tag = strdup(""); - if ((*tag)->tag == NULL) { - err = got_error_from_errno("strdup"); - break; - } - } else { - (*tag)->tag = malloc(itag->tag_len + 1); - if ((*tag)->tag == NULL) { - err = got_error_from_errno("malloc"); - break; - } - memcpy((*tag)->tag, imsg.data + len, - itag->tag_len); - (*tag)->tag[itag->tag_len] = '\0'; + (*tag)->tag = strndup(imsg.data + len, itag->tag_len); + if ((*tag)->tag == NULL) { + err = got_error_from_errno("strndup"); + break; } len += itag->tag_len; @@ -1939,21 +1900,10 @@ got_privsep_recv_tag(struct got_tag_object **tag, stru (*tag)->tagger_time = itag->tagger_time; (*tag)->tagger_gmtoff = itag->tagger_gmtoff; - if (itag->tagger_len == 0) { - (*tag)->tagger = strdup(""); - if ((*tag)->tagger == NULL) { - err = got_error_from_errno("strdup"); - break; - } - } else { - (*tag)->tagger = malloc(itag->tagger_len + 1); - if ((*tag)->tagger == NULL) { - err = got_error_from_errno("malloc"); - break; - } - memcpy((*tag)->tagger, imsg.data + len, - itag->tagger_len); - (*tag)->tagger[itag->tagger_len] = '\0'; + (*tag)->tagger = strndup(imsg.data + len, itag->tagger_len); + if ((*tag)->tagger == NULL) { + err = got_error_from_errno("strndup"); + break; } len += itag->tagger_len; @@ -2169,11 +2119,10 @@ got_privsep_recv_gitconfig_str(char **str, struct imsg const struct got_error *err = NULL; struct imsg imsg; size_t datalen; - const size_t min_datalen = 0; *str = NULL; - err = got_privsep_recv_imsg(&imsg, ibuf, min_datalen); + err = got_privsep_recv_imsg(&imsg, ibuf, 0); if (err) return err; datalen = imsg.hdr.len - IMSG_HEADER_SIZE; @@ -2415,21 +2364,16 @@ got_privsep_recv_gotconfig_str(char **str, struct imsg const struct got_error *err = NULL; struct imsg imsg; size_t datalen; - const size_t min_datalen = 0; *str = NULL; - err = got_privsep_recv_imsg(&imsg, ibuf, min_datalen); + err = got_privsep_recv_imsg(&imsg, ibuf, 0); if (err) return err; datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_GOTCONFIG_STR_VAL: @@ -2476,10 +2420,6 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_GOTCONFIG_REMOTES: @@ -2488,6 +2428,10 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r break; } memcpy(&iremotes, imsg.data, sizeof(iremotes)); + if (iremotes.nremotes < 0) { + err = got_error(GOT_ERR_PRIVSEP_LEN); + break; + } if (iremotes.nremotes == 0) { imsg_free(&imsg); return NULL; @@ -2517,10 +2461,6 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r switch (imsg.hdr.type) { case GOT_IMSG_ERROR: - if (datalen < sizeof(struct got_imsg_error)) { - err = got_error(GOT_ERR_PRIVSEP_LEN); - break; - } err = recv_imsg_error(&imsg, datalen); break; case GOT_IMSG_GOTCONFIG_REMOTE: @@ -2862,7 +2802,7 @@ got_privsep_recv_enumerated_objects(struct imsgbuf *ib datalen = imsg.hdr.len - IMSG_HEADER_SIZE; switch (imsg.hdr.type) { - case GOT_IMSG_ENUMERATED_COMMIT: + case GOT_IMSG_ENUMERATED_COMMIT: if (have_commit && nentries != -1) { err = got_error(GOT_ERR_PRIVSEP_MSG); break; @@ -3219,7 +3159,8 @@ got_privsep_recv_object_idlist(int *done, struct got_o break; } idlist = imsg.data; - if (idlist->nids > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) { + if (idlist->nids > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS || + idlist->nids * sizeof(**ids) > datalen - sizeof(*idlist)) { err = got_error(GOT_ERR_PRIVSEP_LEN); break; } @@ -3229,7 +3170,7 @@ got_privsep_recv_object_idlist(int *done, struct got_o err = got_error_from_errno("calloc"); break; } - memcpy(*ids, (uint8_t *)imsg.data + sizeof(idlist), + memcpy(*ids, (uint8_t *)imsg.data + sizeof(*idlist), *nids * sizeof(**ids)); break; case GOT_IMSG_OBJ_ID_LIST_DONE: @@ -3324,12 +3265,14 @@ got_privsep_recv_reused_deltas(int *done, struct got_i break; } ideltas = imsg.data; - if (ideltas->ndeltas > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) { + if (ideltas->ndeltas > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS || + ideltas->ndeltas * sizeof(*deltas) > + datalen - sizeof(*ideltas)) { err = got_error(GOT_ERR_PRIVSEP_LEN); break; } *ndeltas = ideltas->ndeltas; - memcpy(deltas, (uint8_t *)imsg.data + sizeof(ideltas), + memcpy(deltas, (uint8_t *)imsg.data + sizeof(*ideltas), *ndeltas * sizeof(*deltas)); break; case GOT_IMSG_DELTA_REUSE_DONE: