Skip to content

harden bounds checks and layout size validation in providers #627

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

Open
jmestwa-coder wants to merge 1 commit into
base: trunk
Choose a base branch
from
+92 −67
Open

harden bounds checks and layout size validation in providers #627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 32 additions & 23 deletions modules/slotmem/mod_slotmem_plain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,24 +38,31 @@ struct ap_slotmem_instance_t {
static struct ap_slotmem_instance_t *globallistmem = NULL;
static apr_pool_t *gpool = NULL;

static int slotmem_size_mul(apr_size_t a, apr_size_t b, apr_size_t *res)
static apr_status_t slotmem_layout_sizes(apr_size_t item_size,
unsigned int item_num,
apr_size_t *basesize,
apr_size_t *totalsize)
{
if (a != 0 && b > ((apr_size_t)-1) / a) {
return 0;
}
apr_size_t slotdata;
apr_size_t total;

*res = a * b;
return 1;
}
if (item_num && item_size > APR_SIZE_MAX / item_num) {
return APR_EINVAL;
}
slotdata = item_size * item_num;

static int slotmem_size_add(apr_size_t a, apr_size_t b, apr_size_t *res)
{
if (a > ((apr_size_t)-1) - b) {
return 0;
if (APR_SIZE_MAX - slotdata < item_num) {
return APR_EINVAL;
}
total = slotdata + item_num;

*res = a + b;
return 1;
if (basesize) {
*basesize = slotdata;
}
if (totalsize) {
*totalsize = total;
}
return APR_SUCCESS;
}

static apr_status_t slotmem_do(ap_slotmem_instance_t *mem, ap_slotmem_callback_fn_t *func, void *data, apr_pool_t *pool)
Expand Down Expand Up @@ -88,16 +95,14 @@ static apr_status_t slotmem_create(ap_slotmem_instance_t **new, const char *name
ap_slotmem_instance_t *res;
ap_slotmem_instance_t *next = globallistmem;
apr_size_t basesize;
apr_size_t inuse_size;
apr_size_t alloc_size;
apr_status_t rv;

const char *fname;

if (!slotmem_size_mul(item_size, (apr_size_t)item_num, &basesize)
|| !slotmem_size_mul((apr_size_t)item_num, sizeof(char),
&inuse_size)
|| !slotmem_size_add(basesize, inuse_size, &alloc_size)) {
return APR_EINVAL;
rv = slotmem_layout_sizes(item_size, item_num, &basesize, &alloc_size);
if (rv != APR_SUCCESS) {
return rv;
}

if (name) {
Expand Down Expand Up @@ -208,7 +213,7 @@ static apr_status_t slotmem_get(ap_slotmem_instance_t *slot, unsigned int id, un
if (id >= slot->num) {
return APR_EINVAL;
}
if (dest_len > slot->size) {
if (dest_len > slot->size || (dest_len && !dest)) {
return APR_EINVAL;
}

Expand All @@ -221,7 +226,9 @@ static apr_status_t slotmem_get(ap_slotmem_instance_t *slot, unsigned int id, un
return ret;
}
*inuse=1;
memcpy(dest, ptr, dest_len); /* bounds check? */
if (dest_len) {
memcpy(dest, ptr, dest_len);
}
return APR_SUCCESS;
}

Expand All @@ -237,7 +244,7 @@ static apr_status_t slotmem_put(ap_slotmem_instance_t *slot, unsigned int id, un
if (id >= slot->num) {
return APR_EINVAL;
}
if (src_len > slot->size) {
if (src_len > slot->size || (src_len && !src)) {
return APR_EINVAL;
}

Expand All @@ -250,7 +257,9 @@ static apr_status_t slotmem_put(ap_slotmem_instance_t *slot, unsigned int id, un
return ret;
}
*inuse=1;
memcpy(ptr, src, src_len); /* bounds check? */
if (src_len) {
memcpy(ptr, src, src_len);
}
return APR_SUCCESS;
}

Expand Down
104 changes: 60 additions & 44 deletions modules/slotmem/mod_slotmem_shm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,24 +72,46 @@ struct ap_slotmem_instance_t {
static struct ap_slotmem_instance_t *globallistmem = NULL;
static apr_pool_t *gpool = NULL;

static int slotmem_size_mul(apr_size_t a, apr_size_t b, apr_size_t *res)
static apr_status_t slotmem_layout_sizes(apr_size_t item_size,
unsigned int item_num,
apr_size_t *basesize,
apr_size_t *persistsize,
apr_size_t *totalsize)
{
if (a != 0 && b > ((apr_size_t)-1) / a) {
return 0;
apr_size_t slotdata;
apr_size_t persist;
apr_size_t total;

if (item_num && item_size > APR_SIZE_MAX / item_num) {
return APR_EINVAL;
}
slotdata = item_size * item_num;

*res = a * b;
return 1;
}
if (APR_SIZE_MAX - AP_UNSIGNEDINT_OFFSET < (apr_size_t)item_num) {
return APR_EINVAL;
}
persist = AP_UNSIGNEDINT_OFFSET + item_num;

static int slotmem_size_add(apr_size_t a, apr_size_t b, apr_size_t *res)
{
if (a > ((apr_size_t)-1) - b) {
return 0;
if (APR_SIZE_MAX - persist < slotdata) {
return APR_EINVAL;
}
persist += slotdata;

*res = a + b;
return 1;
if (APR_SIZE_MAX - AP_SLOTMEM_OFFSET < persist) {
return APR_EINVAL;
}
total = AP_SLOTMEM_OFFSET + persist;

if (basesize) {
*basesize = slotdata;
}
if (persistsize) {
*persistsize = persist;
}
if (totalsize) {
*totalsize = total;
}
return APR_SUCCESS;
}

#define DEFAULT_SLOTMEM_PREFIX "slotmem-shm-"
Expand Down Expand Up @@ -195,8 +217,13 @@ static void store_slotmem(ap_slotmem_instance_t *slotmem)
if (AP_SLOTMEM_IS_CLEARINUSE(slotmem)) {
slotmem_clearinuse(slotmem);
}
nbytes = (slotmem->desc->size * slotmem->desc->num) +
(slotmem->desc->num * sizeof(char)) + AP_UNSIGNEDINT_OFFSET;
rv = slotmem_layout_sizes(slotmem->desc->size, slotmem->desc->num,
NULL, &nbytes, NULL);
if (rv != APR_SUCCESS) {
apr_file_close(fp);
apr_file_remove(storename, slotmem->gpool);
return;
}
apr_md5(digest, slotmem->persist, nbytes);
rv = apr_file_write_full(fp, slotmem->persist, nbytes, NULL);
if (rv == APR_SUCCESS) {
Expand Down Expand Up @@ -368,9 +395,7 @@ static apr_status_t slotmem_create(ap_slotmem_instance_t **new,
ap_slotmem_instance_t *next = globallistmem;
const char *fname, *pname = NULL;
apr_shm_t *shm;
apr_size_t header_size;
apr_size_t basesize;
apr_size_t inuse_size;
apr_size_t size;
int persist = (type & AP_SLOTMEM_TYPE_PERSIST) != 0;
apr_status_t rv;
Expand All @@ -379,13 +404,9 @@ static apr_status_t slotmem_create(ap_slotmem_instance_t **new,
if (gpool == NULL) {
return APR_ENOSHMAVAIL;
}
if (!slotmem_size_mul(item_size, (apr_size_t)item_num, &basesize)
|| !slotmem_size_mul((apr_size_t)item_num, sizeof(char), &inuse_size)
|| !slotmem_size_add(AP_SLOTMEM_OFFSET, AP_UNSIGNEDINT_OFFSET,
&header_size)
|| !slotmem_size_add(header_size, inuse_size, &size)
|| !slotmem_size_add(size, basesize, &size)) {
return APR_EINVAL;
rv = slotmem_layout_sizes(item_size, item_num, &basesize, NULL, &size);
if (rv != APR_SUCCESS) {
return rv;
}
if (slotmem_filenames(pool, name, &fname, persist ? &pname : NULL)) {
/* first try to attach to existing slotmem */
Expand Down Expand Up @@ -512,11 +533,8 @@ static apr_status_t slotmem_attach(ap_slotmem_instance_t **new,
sharedslotdesc_t *desc;
const char *fname;
apr_shm_t *shm;
apr_size_t header_size;
apr_size_t basesize;
apr_size_t inuse_size;
apr_size_t expected_size;
apr_size_t shm_size;
apr_status_t rv;

if (gpool == NULL) {
Expand Down Expand Up @@ -559,18 +577,9 @@ static apr_status_t slotmem_attach(ap_slotmem_instance_t **new,
/* Read the description of the slotmem */
desc = (sharedslotdesc_t *)apr_shm_baseaddr_get(shm);

if (!slotmem_size_mul(desc->size, (apr_size_t)desc->num, &basesize)
|| !slotmem_size_mul((apr_size_t)desc->num, sizeof(char), &inuse_size)
|| !slotmem_size_add(AP_SLOTMEM_OFFSET, AP_UNSIGNEDINT_OFFSET,
&header_size)
|| !slotmem_size_add(header_size, basesize, &expected_size)
|| !slotmem_size_add(expected_size, inuse_size, &expected_size)) {
apr_shm_detach(shm);
return APR_EINVAL;
}

shm_size = apr_shm_size_get(shm);
if (expected_size > shm_size) {
rv = slotmem_layout_sizes(desc->size, desc->num,
&basesize, NULL, &expected_size);
if (rv != APR_SUCCESS || apr_shm_size_get(shm) < expected_size) {
apr_shm_detach(shm);
return APR_EINVAL;
}
Expand Down Expand Up @@ -639,7 +648,7 @@ static apr_status_t slotmem_get(ap_slotmem_instance_t *slot, unsigned int id,
if (id >= slot->desc->num) {
return APR_EINVAL;
}
if (dest_len > slot->desc->size) {
if (dest_len > slot->desc->size || (dest_len && !dest)) {
return APR_EINVAL;
}

Expand All @@ -652,7 +661,9 @@ static apr_status_t slotmem_get(ap_slotmem_instance_t *slot, unsigned int id,
return ret;
}
*inuse = 1;
memcpy(dest, ptr, dest_len); /* bounds check? */
if (dest_len) {
memcpy(dest, ptr, dest_len);
}
return APR_SUCCESS;
}

Expand All @@ -669,7 +680,7 @@ static apr_status_t slotmem_put(ap_slotmem_instance_t *slot, unsigned int id,
if (id >= slot->desc->num) {
return APR_EINVAL;
}
if (src_len > slot->desc->size) {
if (src_len > slot->desc->size || (src_len && !src)) {
return APR_EINVAL;
}

Expand All @@ -682,7 +693,9 @@ static apr_status_t slotmem_put(ap_slotmem_instance_t *slot, unsigned int id,
return ret;
}
*inuse=1;
memcpy(ptr, src, src_len); /* bounds check? */
if (src_len) {
memcpy(ptr, src, src_len);
}
return APR_SUCCESS;
}

Expand Down Expand Up @@ -768,18 +781,21 @@ static apr_status_t slotmem_release(ap_slotmem_instance_t *slot,
unsigned int id)
{
char *inuse;
int inuse_val;

if (!slot) {
return APR_ENOSHMAVAIL;
}

inuse = slot->inuse;

if (id >= slot->desc->num || !inuse[id] ) {
inuse_val = (id < slot->desc->num) ? (int)inuse[id] : -1;

if (id >= slot->desc->num || !inuse_val) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02294)
"slotmem(%s) release failed. Num %u/inuse[%u] %d",
slot->name, slotmem_num_slots(slot),
id, (int)inuse[id]);
id, inuse_val);
if (id >= slot->desc->num) {
return APR_EINVAL;
} else {
Expand Down