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

Message ID mvm7ei8swnb.fsf@suse.de
State New
Headers show
Series
  • Add more checks for valid ld.so.cache file (bug 18093)
Related show

Commit Message

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

-- 
2.19.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Comments

Florian Weimer Oct. 23, 2018, 12:27 p.m. | #1
* 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. | #2
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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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.  */