[PATCH 2/3] elf_begin.c: Use relative offset in archive size check

[Mark Wielaard]

Hi Aaron,

On Sun, Sep 07, 2025 at 10:17:28PM -0400, Aaron Merey wrote:
> Before creating an elf descriptor for an archive member, dup_elf
> verifies that the size of an archive is large enough to contain the
> member.  This check uses the member's offset relative to the map_address
> of the top-level archive containing the member.
> 
> This check can incorrectly fail when an archive contains another
> archive as a member.  For members of the inner archive, their offset
> relative to the outer archive might be significantly larger than
> the max_size of the inner archive.  This appears as if the offset and
> max_size values are inconsistent and the creation of the member's elf
> descriptor is stopped unnecessarily.
> 
> Fix this by accounting for the inner archive's non-zero start_offset
> when judging whether the member can fit within it.
> 
> Also perform this size check before creating a copy of the member's
> Elf_Arhdr to prevent leaking the header's ar_name and ar_rawname if
> the size check fails.

This all makes sense, if you know that ar.offset is the offset against
the map_address instead of the start_offset of the Elf. And that
maximum_size does take the start_offset into account. Which at least
to me wasn't entirely intuitive.

> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  libelf/elf_begin.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
> index 823a4324..037a4234 100644
> --- a/libelf/elf_begin.c
> +++ b/libelf/elf_begin.c
> @@ -1107,22 +1107,24 @@ dup_elf (int fildes, Elf_Cmd cmd, Elf *ref)
>      /* Something went wrong.  Maybe there is no member left.  */
>      return NULL;
>  
> -  Elf_Arhdr ar_hdr = {0};
> -  if (copy_arhdr (&ar_hdr, ref) != 0)
> -    /* Out of memory.  */
> -    return NULL;
> -

OK, move copy later, when we know it is necessary.

>    /* We have all the information we need about the next archive member.
>       Now create a descriptor for it. Check parent size can contain member.  */
> +  if (ref->state.ar.offset < ref->start_offset)
> +    return NULL;

Yes, that would not make sense if true.

>    size_t max_size = ref->maximum_size;
> -  size_t offset = (size_t) ref->state.ar.offset;
> +  size_t offset = (size_t) (ref->state.ar.offset - ref->start_offset);

OK, take start_offset into account.

>    size_t hdr_size = sizeof (struct ar_hdr);
>    size_t ar_size = (size_t) ref->state.ar.elf_ar_hdr.ar_size;
>    if (max_size - hdr_size < offset)
>      return NULL;

Note that this assumes there really is at least an header.
Which I think is an correct assumption if we end up here.
But maybe a more conservative check would be:

    if (max_size < hdr_size || max_size - hdr_size < offset)
      return NULL;

What do you think, too paranoid?

> -  else
> -    result = read_file (fildes, ref->state.ar.offset + sizeof (struct ar_hdr),
> -			MIN (max_size - hdr_size - offset, ar_size), cmd, ref);
> +
> +  Elf_Arhdr ar_hdr = {0};
> +  if (copy_arhdr (&ar_hdr, ref) != 0)
> +    /* Out of memory.  */
> +    return NULL;

OK, swap these calls.

> +  result = read_file (fildes, ref->state.ar.offset + sizeof (struct ar_hdr),
> +		      MIN (max_size - hdr_size - offset, ar_size), cmd, ref);

Right, here we use ar.offset instead of offset (plus the hdr_size) to
set the new Elf member start_offset, which is correct. The MIN
calculation is also now correct (using offset).

>    if (result != NULL)
>      {

Thanks,

Mark