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

login
register
mail settings
Submitter Andreas Schwab
Date Oct. 23, 2018, 11:57 a.m.
Message ID <mvm7ei8swnb.fsf@suse.de>
Download mbox | patch
Permalink /patch/29849/
State New
Headers show

Comments

Andreas Schwab - Oct. 23, 2018, 11:57 a.m.
[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(-)
Florian Weimer - Oct. 23, 2018, 12:27 p.m.
* 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
Andreas Schwab - Oct. 23, 2018, 12:34 p.m.
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.

Patch

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)));
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))
 	{
 	  size_t offset;
 	  /* Looks ok.  */