Add more checks for valid ld.so.cache file (bug 18093)

  [BZ #18093]
	* elf/dl-cache.c (_dl_load_cache_lookup): Check for truncated old
	format cache.
	* elf/cache.c (print_cache): Likewise.
---
 elf/cache.c    | 5 +++++
 elf/dl-cache.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
  

* Andreas Schwab:

> 	[BZ #18093]
> 	* elf/dl-cache.c (_dl_load_cache_lookup): Check for truncated old
> 	format cache.
> 	* elf/cache.c (print_cache): Likewise.
> ---
>  elf/cache.c    | 5 +++++
>  elf/dl-cache.c | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/elf/cache.c b/elf/cache.c
> index e63979da7d..83de25484b 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -199,6 +199,11 @@ print_cache (const char *cache_name)
>      }
>    else
>      {
> +      /* Check for overflow.  */
> +      if ((cache_size - sizeof (struct cache_file)) / sizeof (struct file_entry)
> +	  < cache->nlibs)
> +	error (EXIT_FAILURE, 0, _("File is not a cache file.\n"));
> +
>        size_t offset = ALIGN_CACHE (sizeof (struct cache_file)
>  				   + (cache->nlibs
>  				      * sizeof (struct file_entry)));

I think the “Check for overflow” are misleading because you are checking
for file truncation *and* avoiding an overflow in the check.


> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 6ee5153ff9..0f5d035213 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -204,7 +204,10 @@ _dl_load_cache_lookup (const char *name)
>  	 - only the new format
>  	 The following checks if the cache contains any of these formats.  */
>        if (file != MAP_FAILED && cachesize > sizeof *cache
> -	  && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0)
> +	  && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
> +	  /* Check for overflow.  */
> +	  && ((cachesize - sizeof *cache) / sizeof (struct file_entry)
> +	      >= ((struct cache_file *) file)->nlibs))


Should the new check be nested inside the if statement, so that we do
not fall through to the CACHEMAGIC_VERSION_NEW check?

Thanks,
Florian
  

On Okt 23 2018, Florian Weimer <fweimer@redhat.com> wrote:

>> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
>> index 6ee5153ff9..0f5d035213 100644
>> --- a/elf/dl-cache.c
>> +++ b/elf/dl-cache.c
>> @@ -204,7 +204,10 @@ _dl_load_cache_lookup (const char *name)
>>  	 - only the new format
>>  	 The following checks if the cache contains any of these formats.  */
>>        if (file != MAP_FAILED && cachesize > sizeof *cache
>> -	  && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0)
>> +	  && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
>> +	  /* Check for overflow.  */
>> +	  && ((cachesize - sizeof *cache) / sizeof (struct file_entry)
>> +	      >= ((struct cache_file *) file)->nlibs))
>
>
> Should the new check be nested inside the if statement, so that we do
> not fall through to the CACHEMAGIC_VERSION_NEW check?

We want to fall through to the last alternative that unmaps the file.

Andreas.