Created attachment 11569 [details] The PoC to demonstrate the unsigned integer overflow Hi There Peng Li and Shengjian Guo at Baidu XLab found a suspicious unsigned integer overflow which may bypass a check unintentionally. The bug is found in function process_program_headers of readelf.c of version 2.31.51.20190117. static bfd_boolean process_program_headers (Filedata * filedata) { … /* PR binutils/17512: Avoid corrupt dynamic section info in the segment. Check this after matching against the section headers so we don't warn on debuginfo file (which have NOBITS .dynamic sections). */ if (dynamic_addr + dynamic_size >= filedata->file_size) { error (_("the dynamic segment offset + size exceeds the size of the file\n")); dynamic_addr = dynamic_size = 0; } break; … } If you compile readelf with -fsanitize=unsigned-integer-overflow and run ./readelf -a input, it is found that dynamic_addr + dynamic_size overflows and may bypass the check. Can you please help verify if it is a true positive and do you think adding check for each variable against file_size is necessary? If you have any questions about this issue and input in the attachment, please let me know. Thanks Peng
(In reply to poppeter1982 from comment #0) Hi Peng, > if (dynamic_addr + dynamic_size >= filedata->file_size) > If you compile readelf with -fsanitize=unsigned-integer-overflow The version of gcc that I am using (8.2.1 on Fedora 29) does not support a -fsanitize=unsigned-integer-overflow option. Is this a new feature ? > and run > ./readelf -a input, it is found that dynamic_addr + dynamic_size overflows > and may bypass the check. Can you please help verify if it is a true > positive I added a printf statement before the check to examine the values of these variables: fprintf (stderr, "addr %lx size %llx file %llx plus %llx\n", dynamic_addr, dynamic_size, filedata->file_size, dynamic_addr + dynamic_size); But it appears that the arithmetic works: addr 22000016 size 60000e002200002f file c190d plus 60000e0044000045 readelf: Error: the dynamic segment offset + size exceeds the size of the file (This is with a 32-bit toolchain, which I presume you are using. You did not actually specify how you configured your binutils build). The point is that the dynamic_size and file_size variables are both unsigned long long types, and so the arithmetic does not overflow. So I think that the check should be OK. Cheers Nick
Created attachment 11572 [details] attachment-66164-0.html Hi Nick nickc at redhat dot com <sourceware-bugzilla@sourceware.org> 于2019年1月25日周五 上午4:14写道： > https://sourceware.org/bugzilla/show_bug.cgi?id=24132 > > Nick Clifton <nickc at redhat dot com> changed: > > What |Removed |Added > > ---------------------------------------------------------------------------- > CC| |nickc at redhat dot com > > --- Comment #1 from Nick Clifton <nickc at redhat dot com> --- > (In reply to poppeter1982 from comment #0) > > Hi Peng, > > > if (dynamic_addr + dynamic_size >= filedata->file_size) > > > If you compile readelf with -fsanitize=unsigned-integer-overflow > > The version of gcc that I am using (8.2.1 on Fedora 29) does not support > a -fsanitize=unsigned-integer-overflow option. Is this a new feature ? > I used clang as the compilation frontend > > > > and run > > ./readelf -a input, it is found that dynamic_addr + dynamic_size > overflows > > and may bypass the check. Can you please help verify if it is a true > > positive > > I added a printf statement before the check to examine the values of > these variables: > > fprintf (stderr, "addr %lx size %llx file %llx plus %llx\n", > dynamic_addr, dynamic_size, filedata->file_size, > dynamic_addr + dynamic_size); > > But it appears that the arithmetic works: > > addr 22000016 size 60000e002200002f file c190d plus 60000e0044000045 > readelf: Error: the dynamic segment offset + size exceeds the size of the > file > > (This is with a 32-bit toolchain, which I presume you are using. You > did not actually specify how you configured your binutils build). > > The point is that the dynamic_size and file_size variables are both > unsigned long long types, and so the arithmetic does not overflow. > > So I think that the check should be OK. > I compiled binutils in 64-bits platform, and carefully checked the output. I agree with you that based on this input, the check will not be bypassed. However, do you think there exist the possibility that this check will fail due to overflow? I reported a similar issue https://sourceware.org/bugzilla/show_bug.cgi?id=24138, the check fails due to overflow in this case. Best, Peng
Created attachment 11575 [details] Proposed patch Hi Peng, OK, well I cannot reproduce the problem, but that does not mean that it does not exist. Please could you try out this potential patch and let me know if it solves the problem for you ? Cheers Nick PS. The patch should also work for PR 24138.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c22b42ce308eb538050b4b5789e406b63102b35a commit c22b42ce308eb538050b4b5789e406b63102b35a Author: Alan Modra <amodra@gmail.com> Date: Wed Feb 20 18:22:50 2019 +1030 Unsigned integer overflows in readelf checks PR 24132 PR 24138 * readelf.c (get_data): Avoid possibility of overflow when checking for a read that may extend past end of file. (process_program_headers): Likewise.
Fixed.