Add more checks for valid ld.so.cache file (bug 18093)
Commit Message
[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(-)
Comments
* 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.
@@ -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)));
@@ -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. */