Re: [PATCH] Fix invalid memory access when parsing netgroup files with blank lines (BZ #16506)

[OndÅej BÃlka <neleai at seznam dot cz>]

On Mon, Jan 27, 2014 at 12:35:05PM +0530, Siddhesh Poyarekar wrote:
> Hi,
> 
> The netgroups file parsing code tries to access the character before
> the newline in parsed lines to see if it is a backslash (\).  This
> results in an access before the block allocated for the line if the
> line is blank, i.e. does not have anything other than the newline
> character.  This doesn't seem like it will cause any crashes because
> the byte belongs to the malloc metadata block and hence access to it
> will always succeed.
>     
> There could be an invalid alteration in code flow where a blank line
> is seen as a continuation due to the preceding byte *happening* to be
> '\\'.  This could be done by interposing malloc, but that's not really
> a security problem since one could interpose getnetgrent_r itself and
> achieve a similar 'exploit'.
>     
> The possibility of actually exploiting this is remote to impossible
> since it also requires the previous line to end with a '\\', which
> would happen only on invalid configurations.
> 
> Verified on x86_64 that this patch fixes the valgrind warning that
> caused me to notice the problem and also does not break netgroups
> queries.  OK to commit?
> 
> Siddhesh
> 
> 	[BZ #16506]
> 	* nss/nss_files/files-netgrp.c (_nss_files_setnetgrent): Avoid
> 	access beyond array bounds when parsing netgroups file.
> 
> diff --git a/nss/nss_files/files-netgrp.c b/nss/nss_files/files-netgrp.c
> index 339f704..34eae4c 100644
> --- a/nss/nss_files/files-netgrp.c
> +++ b/nss/nss_files/files-netgrp.c
> @@ -103,7 +103,8 @@ _nss_files_setnetgrent (const char *group, struct __netgrent *result)
>  	      result->cursor += (curlen - group_len) - 1;
>  	    }
>  
> -	  while (line[curlen - 1] == '\n' && line[curlen - 2] == '\\')
> +	  while (curlen > 1 && line[curlen - 1] == '\n'
> +		 && line[curlen - 2] == '\\')
>  	    {
>  	      /* Yes, we have a continuation line.  */
>  	      if (found)
looks ok.