Hi Mark,
On Mon, Sep 8, 2025 at 6:04 PM Mark Wielaard <m...@klomp.org> wrote:
>
> 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 <ame...@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?
We might as well check this too. I'll add this and merge the patch.
Aaron
>
> > - 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
>
