nscd: Fix use-after-free in addgetnetgrentX [BZ #23520]

  addinnetgrX may use the heap-allocated buffer, so free the buffer
in this function.

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.
  

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 :-)