Re: [BUG] Two vulnerabilities in zsh - #1 :: null dereference in check_colon_subscript in subst.c

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Aaron Esau wrote on Tue, 19 May 2020 06:48 +0000:
> The following code in check_colon_subscript in subst.c checks if the value at a pointer obtained from a call to parse_subscript is NULL:
> 
>   *endp = parse_subscript(str, 0, ':');
>   if (!*endp) {
>       /* No trailing colon? */
>       *endp = parse_subscript(str, 0, '\0');
>       if (!*endp)
>           return NULL;
>   }  
> 
> However, the pointer itself can be NULL. [...]
> 
> ## Patch
> 
> diff --git Src/subst.c Src/subst.c
> index 90b5fc121..ac12c6d0e 100644
> --- Src/subst.c
> +++ Src/subst.c
> @@ -1571,10 +1571,10 @@ check_colon_subscript(char *str, char **endp)
>      }  
> 

Your MUA corrupted the patch by munging the trailing whitespace.

You generated the patch without the a/ and b/ prefixes.  I tried this
once but discovered (to my dismay) that «git apply» doesn't apply such
patches by default.  How do you deal with that?

>      *endp = parse_subscript(str, 0, ':');
> -    if (!*endp) {
> +    if (endp && !*endp) {
>  	/* No trailing colon? */
>  	*endp = parse_subscript(str, 0, '\0');
> -	if (!*endp)
> +	if (endp && !*endp)
>  	   return NULL;
>      }  
>      sav = **endp;

endp has already been dereferenced by the time you have it tested for
NULL.  Thus, this patch is a no-op.  In fact, you'll probably find the
(optimized) object code generated for this snippet is not changed by
applying this patch.  If this patch changes the behaviour at all, that'd
be a compiler bug.

Besides, all callers pass non-NULL values for endp.

Did you post the correct patch?

Daniel