Message ID | 20180814105402.C227D4056649D@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | nscd: Fix use-after-free in addgetnetgrentX [BZ #23520] | expand |
On 08/14/2018 06:54 AM, Florian Weimer wrote: > addinnetgrX may use the heap-allocated buffer, so free the buffer > in this function. OK for master. Reviewed-by: Carlos O'Donell <carlos@redhat.com> Thanks for this, I found the code and fix difficult to audit, a more detailed explanation of the failure would have helped, particularly when they require auditing allocation ownership. Just to give you an example this is what I would like to see for these kinds of fixes. The things that I audit: * There is an alloca. The ownership of which can't be passed to the caller. I need to make sure this is true. * There is a malloc. The ownership of which must be passed to the caller, and I need to make sure this is true for all conditions in the code. (a) In addinnetgrX if cache_search returns NULL because there is no entry. 476 struct dataset *result = (struct dataset *) cache_search (GETNETGRENT, 477 group, group_len, 478 db, uid); (b) We pass a NULL result pointer to addgetnetgrentX (&result is not NULL) 489 timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL, 490 &result); (c) The dataset is pointed to a malloc allocated buffer in some cases. 152 memset (&data, '\0', sizeof (data)); 153 buffer = xmalloc (buflen); ^^^^^^ 154 first_needed->next = first_needed; 155 memcpy (first_needed->name, key, group_len); 156 data.needed_groups = first_needed; ... 354 /* Fill in the dataset. */ 355 dataset = (struct dataset *) buffer; (d) We land in a low-memory situation and mempool_alloc fails. 393 { 394 struct dataset *newp 395 = (struct dataset *) mempool_alloc (db, total + req->key_len, 1); 396 if (__glibc_likely (newp != NULL)) 397 { 398 /* Adjust pointer into the memory block. */ 399 key_copy = (char *) newp + (key_copy - buffer); 400 401 dataset = memcpy (newp, dataset, total + req->key_len); 402 cacheable = true; 403 404 if (he != NULL) 405 /* Mark the old record as obsolete. */ 406 dh->usable = false; 407 } 408 } The low-memory situation implies we couldn't resize the database, that mempool_alloc failed, and so the dataset still points to the buffer. The caller may still want to read the results so we can't own it and free it, we should return it instead. (e) We free buffer before returning, but the result is pointing at buffer. 442 free (buffer); 443 444 *resultp = dataset; 445 446 return timeout; (f) We dereference result which could have been freed. 509 datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len, 510 sizeof (innetgroup_response_header), 511 he == NULL ? 0 : dh->nreloads + 1, result->head.ttl); An alternative solution is *not* to return after the low-memory situation causes the database not to expand; could we return an error? Either way, your fix is OK because it continues supporting the code as-is without changing the semantic behaviour at the time of a low-memory scenario. Also, as far as I can tell the alloca for data->needed_groups is not used anywhere outside of addgetnetgrentX, since it's part of the dataset for iterating a netgroup. > 2018-08-14 Florian Weimer <fweimer@redhat.com> > > [BZ #23520] > nscd: Fix use-after-free in addgetnetgrentX and its callers. > * nscd/netgroupcache.c > (addgetnetgrentX): Add tofreep parameter. Do not free > heap-allocated buffer. > (addinnetgrX): Free buffer allocated bt addgetnetgrentX. > (addgetnetgrentX_ignore): New function. > (addgetnetgrent): Call it. > (readdgetnetgrent): Likewise. > > diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c > index 2b35389cc8..87059fb280 100644 > --- a/nscd/netgroupcache.c > +++ b/nscd/netgroupcache.c > @@ -113,7 +113,8 @@ do_notfound (struct database_dyn *db, int fd, request_header *req, > static time_t > addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > const char *key, uid_t uid, struct hashentry *he, > - struct datahead *dh, struct dataset **resultp) > + struct datahead *dh, struct dataset **resultp, > + void **tofreep) OK, this is going to be passed a pointer which will contain data that the caller is responsible for freeing. The ownership is passed up to the caller. > { > if (__glibc_unlikely (debug_level > 0)) > { > @@ -139,6 +140,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > size_t group_len = strlen (key) + 1; > struct name_list *first_needed > = alloca (sizeof (struct name_list) + group_len); > + *tofreep = NULL; OK. > > if (netgroup_database == NULL > && __nss_database_lookup ("netgroup", NULL, NULL, &netgroup_database)) > @@ -151,6 +153,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > > memset (&data, '\0', sizeof (data)); > buffer = xmalloc (buflen); > + *tofreep = buffer; OK, here ownership of the buffer is passed to the caller. > first_needed->next = first_needed; > memcpy (first_needed->name, key, group_len); > data.needed_groups = first_needed; > @@ -439,8 +442,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, > } > > out: > - free (buffer); > - OK, we don't free it because we don't own it. > *resultp = dataset; > > return timeout; > @@ -477,8 +478,12 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > group, group_len, > db, uid); > time_t timeout; > + void *tofree; > if (result != NULL) > - timeout = result->head.timeout; > + { > + timeout = result->head.timeout; > + tofree = NULL; OK, if result was not NULL then we don't have anything to free either. > + } > else > { > request_header req_get = > @@ -487,7 +492,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > .key_len = group_len > }; > timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL, > - &result); > + &result, &tofree); OK, request ownership of the locally allocated buffer in addgetnetgrentX (if it is required). > } > > struct indataset > @@ -560,7 +565,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > ++dh->nreloads; > if (cacheable) > pthread_rwlock_unlock (&db->lock); > - return timeout; > + goto out; OK. Exit through a single path. > } > > if (he == NULL) > @@ -596,17 +601,30 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, > dh->usable = false; > } > > + out: > + free (tofree); OK, done with tofree and free it (or not in some cases, but it's NULL then). > return timeout; > } > > > +static time_t > +addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req, > + const char *key, uid_t uid, struct hashentry *he, > + struct datahead *dh) > +{ > + struct dataset *ignore; > + void *tofree; > + time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, > + &ignore, &tofree); > + free (tofree); > + return timeout; > +} OK, simplify code to a call of addgetnetgrentX_ignore which ignores the dataset. > + > void > addgetnetgrent (struct database_dyn *db, int fd, request_header *req, > void *key, uid_t uid) > { > - struct dataset *ignore; > - > - addgetnetgrentX (db, fd, req, key, uid, NULL, NULL, &ignore); > + addgetnetgrentX_ignore (db, fd, req, key, uid, NULL, NULL); OK. > } > > > @@ -619,10 +637,8 @@ readdgetnetgrent (struct database_dyn *db, struct hashentry *he, > .type = GETNETGRENT, > .key_len = he->len > }; > - struct dataset *ignore; > - > - return addgetnetgrentX (db, -1, &req, db->data + he->key, he->owner, he, dh, > - &ignore); > + return addgetnetgrentX_ignore > + (db, -1, &req, db->data + he->key, he->owner, he, dh); OK. > } > > >
On 08/27/2018 09:03 PM, Carlos O'Donell wrote: > Thanks for this, I found the code and fix difficult to audit, a more detailed > explanation of the failure would have helped, particularly when they require > auditing allocation ownership. Just to give you an example this is what I would > like to see for these kinds of fixes. I didn't want to post my analysis to prejudice yours, and wanted to see if you came up with the same sequence of events in your review. I'm not sure if this is the right approach. How can we otherwise ensure that a review has some level of independence? How far should we backport this fix? Thanks, Florian
On 08/28/2018 07:40 AM, Florian Weimer wrote: > On 08/27/2018 09:03 PM, Carlos O'Donell wrote: >> Thanks for this, I found the code and fix difficult to audit, a >> more detailed explanation of the failure would have helped, >> particularly when they require auditing allocation ownership. Just >> to give you an example this is what I would like to see for these >> kinds of fixes. > > I didn't want to post my analysis to prejudice yours, and wanted to > see if you came up with the same sequence of events in your review. > I'm not sure if this is the right approach. How can we otherwise > ensure that a review has some level of independence? This is a very valid point. Perhaps it is sufficient to state this clearly so the reviewer knows you have your own analysis and can perhaps discuss aspects of it with you, but that you haven't posted it to avoid tainting any subsequent analysis. > How far should we backport this fix? Not far. AFAICT only a low-memory failure will trigger the use-after-free and correctness under low-memory constraints is difficult to prove. I'd fix it in master only, or master and release/2.28/master if you are feeling generous :-)
diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c index 2b35389cc8..87059fb280 100644 --- a/nscd/netgroupcache.c +++ b/nscd/netgroupcache.c @@ -113,7 +113,8 @@ do_notfound (struct database_dyn *db, int fd, request_header *req, static time_t addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, const char *key, uid_t uid, struct hashentry *he, - struct datahead *dh, struct dataset **resultp) + struct datahead *dh, struct dataset **resultp, + void **tofreep) { if (__glibc_unlikely (debug_level > 0)) { @@ -139,6 +140,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, size_t group_len = strlen (key) + 1; struct name_list *first_needed = alloca (sizeof (struct name_list) + group_len); + *tofreep = NULL; if (netgroup_database == NULL && __nss_database_lookup ("netgroup", NULL, NULL, &netgroup_database)) @@ -151,6 +153,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, memset (&data, '\0', sizeof (data)); buffer = xmalloc (buflen); + *tofreep = buffer; first_needed->next = first_needed; memcpy (first_needed->name, key, group_len); data.needed_groups = first_needed; @@ -439,8 +442,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, } out: - free (buffer); - *resultp = dataset; return timeout; @@ -477,8 +478,12 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, group, group_len, db, uid); time_t timeout; + void *tofree; if (result != NULL) - timeout = result->head.timeout; + { + timeout = result->head.timeout; + tofree = NULL; + } else { request_header req_get = @@ -487,7 +492,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, .key_len = group_len }; timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL, - &result); + &result, &tofree); } struct indataset @@ -560,7 +565,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, ++dh->nreloads; if (cacheable) pthread_rwlock_unlock (&db->lock); - return timeout; + goto out; } if (he == NULL) @@ -596,17 +601,30 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, dh->usable = false; } + out: + free (tofree); return timeout; } +static time_t +addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req, + const char *key, uid_t uid, struct hashentry *he, + struct datahead *dh) +{ + struct dataset *ignore; + void *tofree; + time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, + &ignore, &tofree); + free (tofree); + return timeout; +} + void addgetnetgrent (struct database_dyn *db, int fd, request_header *req, void *key, uid_t uid) { - struct dataset *ignore; - - addgetnetgrentX (db, fd, req, key, uid, NULL, NULL, &ignore); + addgetnetgrentX_ignore (db, fd, req, key, uid, NULL, NULL); } @@ -619,10 +637,8 @@ readdgetnetgrent (struct database_dyn *db, struct hashentry *he, .type = GETNETGRENT, .key_len = he->len }; - struct dataset *ignore; - - return addgetnetgrentX (db, -1, &req, db->data + he->key, he->owner, he, dh, - &ignore); + return addgetnetgrentX_ignore + (db, -1, &req, db->data + he->key, he->owner, he, dh); }