Repository navigation
elf.c: read the section headers in the byte order of the ELF file - #133
gianlucarenzi wants to merge 1 commit into
Conversation
appimage_get_elf_section_offset_and_length() read e_shoff, e_shnum, e_shstrndx, sh_name, sh_offset and sh_size as they are in the file: with the runtime of a big endian architecture (ppc64) on a little endian machine, it got wrong offsets and read past the mapping, so appimagetool crashed (segmentation fault) when embedding the update information or the MD5 digest. They now go through file16/32/64_to_cpu(), which know the byte order from e_ident; open() and mmap() failures are reported instead of crashing. read_elf32() converted the 32 bit sh_offset and sh_size with file64_to_cpu(): file32_to_cpu() now.
There was a problem hiding this comment.
Thank you for this fix. I read the diff and verified it; the change is correct and fixes a real crash.
What it changes. appimage_get_elf_section_offset_and_length() read e_shoff, e_shnum, e_shstrndx, sh_name, sh_offset and sh_size raw from the mapped file, so for a runtime of the other byte order (big-endian ppc64 on an x86_64 host) the offsets were wrong and the lookup read past the mapping. The PR copies e_ident into the file-scope ehdr first, so the existing file16/32/64_to_cpu() helpers know the byte order, and routes every field through them with the right width (32-bit sh_name in the ELF64 branch, 16-bit e_shnum and e_shstrndx). open() and mmap() failures now return false with a message, and read_elf32() uses the 32-bit helper for the 32-bit sh_offset/sh_size.
Verification. I compiled src/elf.c from main and from this PR with AddressSanitizer against a small harness and ran both on /bin/ls and on four synthetic ELF files (32/64 bit, little/big endian) carrying an .upd_info section:
| Input | main |
this PR |
|---|---|---|
/bin/ls, .dynstr |
correct | correct, same values |
| ELF64 little-endian | correct | correct |
| ELF64 big-endian | ASan abort | correct, matches readelf -S |
| ELF32 little-endian | correct | correct |
| ELF32 big-endian | ASan abort | correct, matches readelf -S |
| nonexistent file | SEGV | false, message |
| 3-byte file | — | false, message |
The native case is unchanged, the widths are right everywhere I checked, and the error paths unmap correctly.
CI. All checks on e66658f passed: the x86_64, i686, aarch64 and armhf builds and the release-and-upload job.
Three remarks, none of them blocking:
- The function still trusts
e_shoff,e_shstrndxand thesh_offsetvalues againstmap_size, so a truncated or crafted file can still read out of bounds. Pre-existing and out of scope here; the newmap_sizecheck would be the natural place for bounds checks later. read_elf32()andread_elf64()have no callers left insrc/, so thefile32_to_cpufix there is correct but touches dead code.- The byte-order state lives in the file-scope
static Elf64_Ehdr ehdr, so the function is not reentrant. Also pre-existing.
Nothing stands in the way of merging from my side.
Generated by Claude Code
The problem
appimage_get_elf_section_offset_and_length()maps the AppImage and readse_shoff,e_shnum,e_shstrndx,sh_name,sh_offsetandsh_sizeas they are in the file, in the byte order of the machine. When the runtime has the other byte order (e.g. the runtime of big endian ppc64 given with--runtime-fileon x86_64, or appimagetool running on a big endian machine with a little endian runtime), the offsets are garbage and the lookup reads past the mapping: appimagetool crashes with a segmentation fault while embedding the update information or the MD5 digest, right after "Marking the AppImage as executable...".The rest of
elf.calready hasfile16_to_cpu(),file32_to_cpu()andfile64_to_cpu()for this; this function did not use them.The fix
file16/32/64_to_cpu();e_identof the mapped file is copied into theehdrthey read the byte order from.open()andmmap()failures are reported and returnfalseinstead of crashing.read_elf32()converted the 32 bitsh_offsetandsh_sizeof a 32 bit section header withfile64_to_cpu(); it usesfile32_to_cpu()now (it only mattered for 32 bit files of the other byte order).Nothing changes for runtimes of the byte order of the machine, i.e. all the cases that worked before.
Tested
src/elf.c(with AddressSanitizer) gives for.upd_info,.digest_md5and.sha256_sigthe same offsets and sizes asreadelf -S, for the runtimes of x86_64, i686, armhf, riscv64, ppc64le and big endian ppc64.-u; in the ppc64 AppImage the update information and the MD5 digest are at the offsets of.upd_infoand.digest_md5thatreadelfgives.This came up while porting appimagetool and the runtime to riscv64, ppc64 and ppc64le (https://github.com/gianlucarenzi/appimagetool, https://github.com/gianlucarenzi/type2-runtime); those ports will be proposed separately.