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 <[email protected]> > --- > 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
