From 12bc3e29378fb83819a842a5fddaedd2ad5f45a2 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Thu, 3 Oct 2019 15:09:31 +0200 Subject: [PATCH 01/16] prevent integer overflow This change adds individual bounds checks for namelen and prefixlen in order to prevent integer overflow (which could cause memory corruption). --- minix/lib/libsys/rmib.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/minix/lib/libsys/rmib.c b/minix/lib/libsys/rmib.c index e4b686389b..82084de60d 100644 --- a/minix/lib/libsys/rmib.c +++ b/minix/lib/libsys/rmib.c @@ -711,7 +711,9 @@ rmib_call(const message * m_in) */ /* A zero name length is valid and should always yield EISDIR. */ namelen = m_in->m_mib_lsys_call.name_len; - if (prefixlen + namelen > __arraycount(name)) + if (namelen > __arraycount(name) || + prefixlen > __arraycount(name) || + prefixlen + namelen > __arraycount(name)) return EINVAL; if (namelen > 0) { From fc1b542624e72765c66f2d948408830b47a571eb Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Thu, 3 Oct 2019 15:18:52 +0200 Subject: [PATCH 02/16] add a bounds check This change adds a bounds check to make sure count is not larger than MAX_PARAMS when copying data from the caller. This prevents a buffer overflow from occurring. --- minix/drivers/vmm_guest/vbox/hgcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minix/drivers/vmm_guest/vbox/hgcm.c b/minix/drivers/vmm_guest/vbox/hgcm.c index c230e1d020..605f9f5c22 100644 --- a/minix/drivers/vmm_guest/vbox/hgcm.c +++ b/minix/drivers/vmm_guest/vbox/hgcm.c @@ -575,7 +575,7 @@ static int do_call(message *m_ptr, int ipc_status, int *code) hgcm_conn[conn].req[req].grant = m_ptr->VBOX_GRANT; hgcm_conn[conn].req[req].count = count; - if (count > 0) { + if (count > 0 && count <= MAX_PARAMS) { if ((r = sys_safecopyfrom(m_ptr->m_source, m_ptr->VBOX_GRANT, 0, (vir_bytes) hgcm_conn[conn].req[req].param, count * sizeof(vbox_param_t))) != OK) From c83104101c5941668491e30eab98bde4ad69a638 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Thu, 3 Oct 2019 15:26:53 +0200 Subject: [PATCH 03/16] validated returned length value Make sure the service isn't lying about the amount of bytes it wrote into vaddr. This prevents possible out of bound reads and writes by callers that rely on and trust the length value returned by the server. --- minix/lib/libsys/ds.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/minix/lib/libsys/ds.c b/minix/lib/libsys/ds.c index 7f665e9b0b..223f08302f 100644 --- a/minix/lib/libsys/ds.c +++ b/minix/lib/libsys/ds.c @@ -141,6 +141,9 @@ static int ds_retrieve_raw(const char *ds_name, char *vaddr, size_t *length, m.m_ds_req.val_len = *length; m.m_ds_req.flags = flags; r = do_invoke_ds(&m, DS_RETRIEVE, ds_name); + if (m.m_ds_reply.val_len > *length) { + return EINVAL; + } *length = m.m_ds_reply.val_len; cpf_revoke(gid); From bf0147da26b7d0a4350deeb1a4c4315c2ea23e65 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Thu, 3 Oct 2019 15:39:37 +0200 Subject: [PATCH 04/16] clear message structure before filling it in This change clears the message structure before filling it in. This prevents uninitialized stack data from vfs to leak to the other endpoint. --- minix/servers/vfs/request.c | 1 + 1 file changed, 1 insertion(+) diff --git a/minix/servers/vfs/request.c b/minix/servers/vfs/request.c index 19ae609b2a..ea00690b1c 100644 --- a/minix/servers/vfs/request.c +++ b/minix/servers/vfs/request.c @@ -187,6 +187,7 @@ int req_create( panic("req_create: cpf_grant_direct failed"); /* Fill in request message */ + memset(&m, 0, sizeof(m)); m.m_type = REQ_CREATE; m.m_vfs_fs_create.inode = inode_nr; m.m_vfs_fs_create.mode = omode; From ad9475f6bf2a4b1745f5368fe6c011278650977d Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Thu, 3 Oct 2019 15:42:33 +0200 Subject: [PATCH 05/16] make sure name_len is not negative This change adds a check to make sure name_len is not negative. This change was made to prevent memory corruption from occurring if name_len is negative. --- minix/servers/vfs/path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minix/servers/vfs/path.c b/minix/servers/vfs/path.c index 5360d2c1f3..7598d9cfe9 100644 --- a/minix/servers/vfs/path.c +++ b/minix/servers/vfs/path.c @@ -621,7 +621,7 @@ get_name(struct vnode *dirp, struct vnode *entry, char ename[NAME_MAX + 1]) cur = (struct dirent *) (buf + consumed); name_len = cur->d_reclen - offsetof(struct dirent, d_name) - 1; - if(cur->d_name + name_len+1 > &buf[sizeof(buf)]) + if(name_len < 0 || cur->d_name + name_len+1 > &buf[sizeof(buf)]) return(EINVAL); /* Rubbish in dir entry */ if (entry->v_inode_nr == cur->d_fileno) { /* found the entry we were looking for */ From 02d84e84337cf0e8cc9f5d0c21be945d9d768d51 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Fri, 4 Oct 2019 12:38:54 +0200 Subject: [PATCH 06/16] don't leak grant on error corner case fix a small code change where a grant would get leaked. --- minix/lib/libsys/ds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minix/lib/libsys/ds.c b/minix/lib/libsys/ds.c index 223f08302f..1de57d2ad8 100644 --- a/minix/lib/libsys/ds.c +++ b/minix/lib/libsys/ds.c @@ -141,11 +141,11 @@ static int ds_retrieve_raw(const char *ds_name, char *vaddr, size_t *length, m.m_ds_req.val_len = *length; m.m_ds_req.flags = flags; r = do_invoke_ds(&m, DS_RETRIEVE, ds_name); + cpf_revoke(gid); if (m.m_ds_reply.val_len > *length) { return EINVAL; } *length = m.m_ds_reply.val_len; - cpf_revoke(gid); return r; } From 439ac04a65a685d280eedef438807e03e4f30c27 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Sat, 5 Oct 2019 21:56:07 +0200 Subject: [PATCH 07/16] add bounds check to prevent memory corruption and integer overflow This change adds a bounds check to make sure userland can't overflow a kernel stack buffer. In addition the change also prevents an integer overflow from occurring. --- minix/kernel/system/do_safecopy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/minix/kernel/system/do_safecopy.c b/minix/kernel/system/do_safecopy.c index 83ad0ad4ec..eac8c0ebd0 100644 --- a/minix/kernel/system/do_safecopy.c +++ b/minix/kernel/system/do_safecopy.c @@ -412,6 +412,9 @@ int do_vsafecopy(struct proc * caller, message * m_ptr) /* No. of vector elements. */ els = m_ptr->m_lsys_kern_vsafecopy.vec_size; + if (els > SCPVEC_NR) { + return EINVAL; + } bytes = els * sizeof(struct vscp_vec); /* Obtain vector of copies. */ From 37230bf0c3710250e69cd660ed570534dd2557b7 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Sat, 5 Oct 2019 22:57:30 +0200 Subject: [PATCH 08/16] fix boundscheck was checking upper bounds, but not lower bounds. This change fixes that. --- minix/kernel/system/do_safecopy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minix/kernel/system/do_safecopy.c b/minix/kernel/system/do_safecopy.c index eac8c0ebd0..a76be4688e 100644 --- a/minix/kernel/system/do_safecopy.c +++ b/minix/kernel/system/do_safecopy.c @@ -412,7 +412,7 @@ int do_vsafecopy(struct proc * caller, message * m_ptr) /* No. of vector elements. */ els = m_ptr->m_lsys_kern_vsafecopy.vec_size; - if (els > SCPVEC_NR) { + if (els < 0 || els > SCPVEC_NR) { return EINVAL; } bytes = els * sizeof(struct vscp_vec); From 6d152649a3e1a31116b33e99971ea918c6a56186 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Sun, 6 Oct 2019 20:31:18 +0200 Subject: [PATCH 09/16] fix off-by-one This change strengthens a bounds check to make sure no out of bound indexing occurs when using a port. --- minix/drivers/usb/usb_hub/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/minix/drivers/usb/usb_hub/usb_hub.c b/minix/drivers/usb/usb_hub/usb_hub.c index 6925f681cf..0c456a57e2 100644 --- a/minix/drivers/usb/usb_hub/usb_hub.c +++ b/minix/drivers/usb/usb_hub/usb_hub.c @@ -417,7 +417,7 @@ hub_task(void * UNUSED(arg)) HUB_DEBUG_MSG("bHubContrCurrent %4X", d->bHubContrCurrent); /* Check for sane number of ports... */ - if (d->bNbrPorts > USB_HUB_PORT_LIMIT) { + if (d->bNbrPorts >= USB_HUB_PORT_LIMIT) { HUB_MSG("Too many hub ports declared: %d", d->bNbrPorts); goto HUB_ERROR; } From b3c3f2a9986151eb756add76612114be08e571e4 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Sun, 6 Oct 2019 21:02:10 +0200 Subject: [PATCH 10/16] fix uninitialized struct data leak This change clears the tm structure on the stack before filling it in. This guarantees that the whole structure is initialized, and as such, no uninitialized stack data is copied to the caller. --- minix/drivers/clock/readclock/readclock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/minix/drivers/clock/readclock/readclock.c b/minix/drivers/clock/readclock/readclock.c index b3c0990ef1..fbabddec23 100644 --- a/minix/drivers/clock/readclock/readclock.c +++ b/minix/drivers/clock/readclock/readclock.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -70,6 +71,7 @@ main(int argc, char **argv) switch (m.m_type) { case RTCDEV_GET_TIME: /* Any user can read the time */ + memset(&t, 0x00, sizeof(t)); reply_status = rtc.get_time(&t, m.m_lc_readclock_rtcdev.flags); if (reply_status != OK) { break; From 590393d9a873937334bc5c050210e7129ae89d23 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Mon, 7 Oct 2019 14:45:34 +0200 Subject: [PATCH 11/16] validate devinf This change adds validation for the devman_device_info that is provided by the caller. This is done by introducing a new function devman_validate_device_info. --- minix/servers/devman/{device.c => device.h} | 114 +++++++++++++++++++- 1 file changed, 113 insertions(+), 1 deletion(-) rename minix/servers/devman/{device.c => device.h} (82%) diff --git a/minix/servers/devman/device.c b/minix/servers/devman/device.h similarity index 82% rename from minix/servers/devman/device.c rename to minix/servers/devman/device.h index 0babbfe1bb..0311218e85 100644 --- a/minix/servers/devman/device.c +++ b/minix/servers/devman/device.h @@ -217,6 +217,109 @@ static void do_reply(message *msg, int res) ipc_send(msg->m_source, msg); } +/*===========================================================================* + * devman_string_terminated * + *===========================================================================*/ +int devman_is_string(char *begin, char *end) { + while(begin < end) { + if (*begin == '\0') + return 1; + begin++; + } + return 0; +} + +/*===========================================================================* + * devman_validate_device_info * + *===========================================================================*/ +int +devman_validate_device_info(struct devman_device_info *devinf, size_t len) { + char * buffer = (char *) (devinf); + char * end = buffer + len; + struct devman_device_info_entry *entries; + int i; + + /* make sure caller gave us enough data so it can hold sizeof(*devinf) */ + if (len < sizeof(struct devman_device_info)) { + return EINVAL; + } + + /* make sure name offset isn't bogus */ + if (devinf->name_offset > len) { + return EINVAL; + } + + /* make sure the name is 0-terminated */ + if (!(devman_is_string(buffer + devinf->name_offset, end))) { + return EINVAL; + } + + /* make sure string is reasonably sized. */ + if (strlen(buffer + devinf->name_offset) > NAME_MAX) { + return EINVAL; + } + + /* no negative counts */ + if (devinf->count < 0) { + return EINVAL; + } + + /* make sure count isn't bogus: integer wrap check */ + if (devinf->count > INT_MAX / sizeof(struct devman_device_info_entry)) { + return EINVAL; + } + + /* make sure count isn't bogus: addition overflow check */ + if ((devinf->count * sizeof(struct devman_device_info_entry)) + + sizeof(struct devman_device_info_entry) < + sizeof(struct devman_device_info_entry)) { + return EINVAL; + } + + /* make sure count isn't bogus */ + if ((devinf->count * sizeof(struct devman_device_info_entry)) + + sizeof(struct devman_device_info_entry) > len) { + return EINVAL; + } + + entries = (struct devman_device_info_entry *) + (buffer + sizeof(struct devman_device_info)); + + for (i = 0; i < devinf->count; i++) { + + /* other types aren't implemented. can't validate what isn't there */ + if (entries[i].type != DEVMAN_DEVINFO_STATIC) + continue; + + /* make sure name offset isn't bogus */ + if (entries[i].name_offset > len) { + return EINVAL; + } + + /* make sure data offset isn't bogus */ + if (entries[i].data_offset > len) { + return EINVAL; + } + + /* make sure name is 0-terminated */ + if (!(devman_is_string(buffer + entries[i].name_offset, end))) { + return EINVAL; + } + + /* make sure string is reasonably sized. */ + if (strlen(buffer + entries[i].name_offset) > NAME_MAX) { + return EINVAL; + } + + /* make sure data is 0-terminated */ + if (!(devman_is_string(buffer + entries[i].data_offset, end))) { + return EINVAL; + } + } + + return OK; +} + /*===========================================================================* * do_add_device * *===========================================================================*/ @@ -245,7 +348,16 @@ int do_add_device(message *msg) do_reply(msg, res); return 0; } - + + res = devman_validate_device_info(devinf, msg->DEVMAN_GRANT_SIZE); + + if (res != OK) { + res = EINVAL; + free(devinf); + do_reply(msg, res); + return 0; + } + if ((parent = _find_dev(&root_dev, devinf->parent_dev_id)) == NULL) { res = ENODEV; From 2ced1c6ef34acd6bc2f3030ab96ebe361cde91ba Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Mon, 7 Oct 2019 14:47:11 +0200 Subject: [PATCH 12/16] ... yea, that happened .., --- minix/servers/devman/{device.h => device.c} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename minix/servers/devman/{device.h => device.c} (100%) diff --git a/minix/servers/devman/device.h b/minix/servers/devman/device.c similarity index 100% rename from minix/servers/devman/device.h rename to minix/servers/devman/device.c From f7973340e688fd850dc75e7d56a0f9e32ff22779 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Mon, 7 Oct 2019 14:48:16 +0200 Subject: [PATCH 13/16] include header need limits.h for INT_MAX --- minix/servers/devman/devman.h | 1 + 1 file changed, 1 insertion(+) diff --git a/minix/servers/devman/devman.h b/minix/servers/devman/devman.h index a9d77d5f01..b530035ddd 100644 --- a/minix/servers/devman/devman.h +++ b/minix/servers/devman/devman.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include From d51e668f6096527373507b3c8ffa40d2c402ac98 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Fri, 11 Oct 2019 22:13:58 +0200 Subject: [PATCH 14/16] don't trust filesystem we're calling out to This change guarantees the vfs stat buffer is initialized. In the event that the fs we're calling in to is lying (says it wrote into buf, but really didn't) uninitialized vfs stack data would get send to the vfs caller. --- minix/servers/vfs/request.c | 1 + 1 file changed, 1 insertion(+) diff --git a/minix/servers/vfs/request.c b/minix/servers/vfs/request.c index ea00690b1c..a7af02ba60 100644 --- a/minix/servers/vfs/request.c +++ b/minix/servers/vfs/request.c @@ -239,6 +239,7 @@ int req_statvfs(endpoint_t fs_e, struct statvfs *buf) cp_grant_id_t grant_id; message m; + memset(buf, 0x00, sizeof(struct statvfs)); grant_id = cpf_grant_direct(fs_e, (vir_bytes) buf, sizeof(struct statvfs), CPF_WRITE); if(grant_id == GRANT_INVALID) From a8d533d2c996e94aa0a0d4761a4c67df94bfe641 Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Fri, 11 Oct 2019 22:36:14 +0200 Subject: [PATCH 15/16] clear message structure before filling it in This change clears the message structure before filling it in. This prevents uninitialized stack data from vfs leaking to a file system driver. --- minix/servers/vfs/request.c | 1 + 1 file changed, 1 insertion(+) diff --git a/minix/servers/vfs/request.c b/minix/servers/vfs/request.c index a7af02ba60..8419c7e55d 100644 --- a/minix/servers/vfs/request.c +++ b/minix/servers/vfs/request.c @@ -1202,6 +1202,7 @@ int req_utime(endpoint_t fs_e, ino_t inode_nr, struct timespec * actimespec, assert(actimespec != NULL); assert(modtimespec != NULL); + memset(&m, 0, sizeof(m)); /* Fill in request message */ m.m_type = REQ_UTIME; m.m_vfs_fs_utime.inode = inode_nr; From df7dbfe5c608e7ea1cfc747172605611305a119b Mon Sep 17 00:00:00 2001 From: Ilja van Sprundel Date: Fri, 11 Oct 2019 23:17:40 +0200 Subject: [PATCH 16/16] make sure len is not 0 This change makes sure len is not 0. If len is 0 an int underflow would occur when looking for a terminating 0-byte and could lead to operating on uninitialized data. --- minix/servers/vfs/utility.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/minix/servers/vfs/utility.c b/minix/servers/vfs/utility.c index f68bf38be7..85763d33e2 100644 --- a/minix/servers/vfs/utility.c +++ b/minix/servers/vfs/utility.c @@ -43,6 +43,11 @@ int copy_path(char *dest, size_t size) if (len > M_PATH_STRING_MAX) return fetch_name(name, len, dest); + if (len == 0) { + err_code = EINVAL; + return(EGENERIC); + } + /* Just copy the path from the message */ strncpy(dest, job_m_in.m_lc_vfs_path.buf, len);