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.

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

several small changes to prevent potential security and reliability issues due to slight implementation bugs. #298

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
2 changes: 2 additions & 0 deletions minix/drivers/clock/readclock/readclock.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <time.h>
#include <errno.h>
#include <lib.h>
#include <string.h>
#include <minix/type.h>
#include <minix/const.h>
#include <minix/callnr.h>
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion minix/drivers/usb/usb_hub/usb_hub.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion minix/drivers/vmm_guest/vbox/hgcm.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions minix/kernel/system/do_safecopy.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 < 0 || els > SCPVEC_NR) {
return EINVAL;
}
bytes = els * sizeof(struct vscp_vec);

/* Obtain vector of copies. */
Expand Down
5 changes: 4 additions & 1 deletion minix/lib/libsys/ds.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +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);
*length = m.m_ds_reply.val_len;
cpf_revoke(gid);
if (m.m_ds_reply.val_len > *length) {
return EINVAL;
}
*length = m.m_ds_reply.val_len;

return r;
}
Expand Down
4 changes: 3 additions & 1 deletion minix/lib/libsys/rmib.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
114 changes: 113 additions & 1 deletion minix/servers/devman/device.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 *
*===========================================================================*/
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions minix/servers/devman/devman.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <stdio.h>
#include <string.h>
#include <lib.h>
#include <limits.h>
#include <minix/timers.h>

#include <minix/callnr.h>
Expand Down
2 changes: 1 addition & 1 deletion minix/servers/vfs/path.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
3 changes: 3 additions & 0 deletions minix/servers/vfs/request.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -238,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)
Expand Down Expand Up @@ -1200,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;
Expand Down
5 changes: 5 additions & 0 deletions minix/servers/vfs/utility.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down