| Summary: | passwd applet ruins UID field in /etc/passwd | ||
|---|---|---|---|
| Product: | Busybox | Reporter: | Jelle Martijn Kok <jmkok> |
| Component: | Other | Assignee: | unassigned |
| Status: | RESOLVED INVALID | ||
| Severity: | major | CC: | busybox-cvs |
| Priority: | P5 | ||
| Version: | 1.19.x | ||
| Target Milestone: | --- | ||
| Hardware: | Other | ||
| OS: | Linux | ||
| Host: | Target: | ||
| Build: | |||
| Attachments: |
libbb/update_passwd.s (CONFIG_DEBUG=n)
libbb/update_passwd.s (CONFIG_DEBUG=y + CONFIG_DEBUG_PESSIMIZE=y) strstr assembly dump from libc-2.13.so Proposed patch isolated code that contains the bug |
||
Does it work if you turn on debugging (and debug pessimise) so you build with O0 (check make V=1) run "make libbb/update_passwd.s" and attach resulting libbb/update_passwd.s file to this bug Aha... now it works correctly (but Busybox is twice the size...) On my development machine: $ make clean $ export CONFIG_DEBUG=y $ export CONFIG_DEBUG_PESSIMIZE=y $ make V=1 (The lines now say "-O0" instead of "-Os") $ make install CONFIG_PREFIX=$ROOTFS (To get it on the arm9) Busyboc has grown from 699664 bytes to 1166624 bytes. ~# cat /etc/passwd | grep -E ^root root:x:0:0:root:/root:/bin/sh ~# passwd -d root Password for root changed by root ~# cat /etc/passwd | grep -E ^root root:x:0:0:root:/root:/bin/sh Other tests correctly changes the password and shadow file. So it seems to be an optimizer issued... I'll now built the "libbb/update_passwd.s" file and attach it to the next message as suggested by Denys. Created attachment 3758 [details]
libbb/update_passwd.s (CONFIG_DEBUG=n)
Created attachment 3764 [details]
libbb/update_passwd.s (CONFIG_DEBUG=y + CONFIG_DEBUG_PESSIMIZE=y)
I attached the libbb/update_passwd.s. Once when debug is enabled (and it works correctly) and once when compiled without debugging and the issue occurs. Note that the command line used to compile it was: (copy and paste after "make V=1") arm-none-linux-gnueabi-gcc -Wp,-MD,libbb/.update_passwd.s.d -std=gnu99 -Iinclude -Ilibbb -include include/autoconf.h -D_GNU_SOURCE -DNDEBUG -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D"BB_VER=KBUILD_STR(1.19.3)" -DBB_BT=AUTOCONF_TIMESTAMP -Wall -mcpu=arm926ej-s -O3 -I/home/jmkok/Projects/ARM9/rootfs/include -I/home/jmkok/Projects/ARM9/rootfs/usr/include -I/home/jmkok/Projects/ARM9/rootfs/usr/local/include -Wall -Wshadow -Wwrite-strings -Wundef -Wstrict-prototypes -Wunused -Wunused-parameter -Wunused-function -Wunused-value -Wmissing-prototypes -Wmissing-declarations -Wdeclaration-after-statement -Wold-style-definition -fno-builtin-strlen -finline-limit=0 -fomit-frame-pointer -ffunction-sections -fdata-sections -fno-guess-branch-probability -funsigned-char -static-libgcc -falign-functions=1 -falign-jumps=1 -falign-labels=1 -falign-loops=1 -Os -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(update_passwd)" -D"KBUILD_MODNAME=KBUILD_STR(update_passwd)" -fverbose-asm -S -o libbb/update_passwd.s libbb/update_passwd.c I just removed my own "CFLAGS" by doing: unset CFLAGS LDFLAGS CXXFLAGS The result was the same though (the UID was replaced by the date) The line that was used to compile the file was now: arm-none-linux-gnueabi-gcc -Wp,-MD,libbb/.update_passwd.s.d -std=gnu99 -Iinclude -Ilibbb -include include/autoconf.h -D_GNU_SOURCE -DNDEBUG -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D"BB_VER=KBUILD_STR(1.19.3)" -DBB_BT=AUTOCONF_TIMESTAMP -Wall -Wshadow -Wwrite-strings -Wundef -Wstrict-prototypes -Wunused -Wunused-parameter -Wunused-function -Wunused-value -Wmissing-prototypes -Wmissing-declarations -Wdeclaration-after-statement -Wold-style-definition -fno-builtin-strlen -finline-limit=0 -fomit-frame-pointer -ffunction-sections -fdata-sections -fno-guess-branch-probability -funsigned-char -static-libgcc -falign-functions=1 -falign-jumps=1 -falign-labels=1 -falign-loops=1 -Os -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(update_passwd)" -D"KBUILD_MODNAME=KBUILD_STR(update_passwd)" -fverbose-asm -S -o libbb/update_passwd.s libbb/update_passwd.c (In reply to comment #4) > Created attachment 3758 [details] > libbb/update_passwd.s (CONFIG_DEBUG=n) Here strstr(filename, "shadow") is saved to [sp, #20] ldr r1, .L31 @, mov r4, r0 @ filename, filename str r2, [sp, #32] @ new_passwd, %sfp bl strstr @ str r0, [sp, #20] @, %sfp and here it is retrieved to check for != NULL. If non-NULL, then date is inserted. ldr r3, [sp, #20] @, %sfp cmp r3, #0 @, beq .L18 @, ldrb r1, [r0, #0] @ zero_extendqisi2 @ tmp218,* cp cmp r1, #58 @ tmp218, bne .L18 @, add r0, r0, #1 @, cp, bl strchrnul @ mov r3, r0 @ cp, ldr r0, [sp, #36] @, %sfp str r3, [sp, #12] @, bl time @ ldr r1, .L31+44 @, bl __aeabi_uidiv @ ldr r3, [sp, #12] @, ldr r1, .L31+48 @, stmia sp, {r0, r3} @ phole stm mov r0, r8 @, new_fp b .L29 @ .L18: Both these code parts look ok. Your debug printouts in comment #1 point towards a bug in strstr implementation. You might want to step into strstr call under gdb when filename = "/etc/passwd" and find out how the hell does it manage to return non-NULL in this case. Which libc do you use? Can you disassemble busybox (objdump -dr busybox) and post here the fragment which shows strstr function's assembly code? Hi Denys, > Which libc do you use? I'm using glibc 2.13 (included in Sourcery G++ Lite 2011.03-41) > You might want to step into strstr call under gdb Did a quick-course on gdb and arm assembly... Started gdb and gdb-server: - I ran the code until strstr(filename, "shadow") - I then requested some (register) states (gdb) x/i $pc => 0x90c1c <update_passwd+12>: bl 0xc860 <strstr> (gdb) x/s $r0 0x9e21b: "/etc/shadow" (gdb) x/s $r1 0x9e220: "shadow" I noticed that R1 (the needle) overlaps R0 (the haystack). Could this cause the problem. I guess this might be the result of the Os flag. However it seems that this is allowed... (I could only find that memcpy is not allowed to overlap). perform stepi in strstr() seems to be a bit tough, I'll give it another go... > Can you disassemble busybox (objdump -dr busybox) and post here the fragment which shows strstr function's assembly code? I use dynamic libraries so I guess you want to have the dump from "libc-2.13.so" I performed: objdump -dr lib/libc-2.13.so | grep '<strstr>:' -A 350 libc-2.13.so.s > strstr.s Created attachment 3776 [details]
strstr assembly dump from libc-2.13.so
> Started gdb and gdb-server: > - I ran the code until strstr(filename, "shadow") > - I then requested some (register) states > (gdb) x/i $pc > => 0x90c1c <update_passwd+12>: bl 0xc860 <strstr> > (gdb) x/s $r0 > 0x9e21b: "/etc/shadow" > (gdb) x/s $r1 > 0x9e220: "shadow" > > I noticed that R1 (the needle) overlaps R0 (the haystack). Could this cause the > problem. I guess this might be the result of the Os flag. However it seems that > this is allowed... (I could only find that memcpy is not allowed to overlap). That's the first call to strstr (see comment #1). Continue, and stop on the second call. It should check "/etc/passwd" against "shadow". Then step over bl instruction, and check return value. What r0 points to? Created attachment 3896 [details]
Proposed patch
I had exactly the same problem. This patch fixed it for me.
It's clearly a compiler or libc bug. Please report it through proper channels to the corresponding projects. We are using the same compiler as Jelle: compiler: arm-none-linux-gnueabi v4.5.2 (Sourcery G++ Lite 2011.03-41) This is still the latest available ARM toolchain for our platform. As Jelle already pointed out, the previous release of this toolchain didn't work for him either - so it is not easy for me to work around with another toolchain. Is it possible to apply my patch in the meantime until this bug gets fixed in the compiler? (In reply to comment #14) > We are using the same compiler as Jelle: > > compiler: arm-none-linux-gnueabi v4.5.2 (Sourcery G++ Lite 2011.03-41) > > This is still the latest available ARM toolchain for our platform. As Jelle > already pointed out, the previous release of this toolchain didn't work for him > either - so it is not easy for me to work around with another toolchain. > > Is it possible to apply my patch in the meantime until this bug gets fixed in > the compiler? Did you guys report the bug to libc or gcc? Can I have URL to these reports? If the patch will be applied, what incentive there will be to fix the REAL bug? I isolated the bug, and it is indeed completely outside of busybox. There seems to be an issue with the cross compiling setup gcc/libc I am using. It is not reproducable on an i386/x64 system, but only on the ARMv5 system we have here. See the attached file strstr.c for information. It only occurs under very specific conditions (see the file): - it must be compiled using the -Os option - the 2nd searched text must end in the first character you are looking for - also the main file must be built up like the example - change anything and the bug disappears... - a gdb trace shows that the second call to strstr is made with the correct parameters, but returns the wrong result... I agree with Denys that the patch is indeed not the solution for the problem. Looking at the bug page for libc, it does not accept bug reports from libraries built by external parties, and as I use the libraries from CodeSourcery they won't accept it. I'll try to locate the bug further, but for now, I would like to thank you for your support. Created attachment 3938 [details]
isolated code that contains the bug
|
When changing the password for a user, the UID field (in /etc/passwd) is overwritten by the "date of last password change" (from /etc/shadow). [/etc/passwd] before: root:x:0:0:root:/root:/bin/sh Now I execute: passwd -d root And /etc/passwd root:x:15296:0:root:/root:/bin/sh (note: 15296 is the day which is normally to be set in the "shadow" file) After some bugtracking I ended up in "update_passwd.c". The update_passwd() is used for both updating the passwd as well as the shadow file. It seems that the following line is strangely not working the second time the function is run: const char *shadow = strstr(filename, "shadow"); I added the following debug lines to the code: + printf("filename: %s (%p)\n", filename, filename); const char *shadow = strstr(filename, "shadow"); + printf("shadow: %s (%p)\n", shadow, shadow); compiled it, and ran it: ~# passwd -d root filename: /etc/shadow (0x9e923) shadow: shadow (0x9e928) filename: /etc/passwd (0x9e917) shadow: shadow (0x9e928) Password for root changed by root The bizar thing is that you see that the second us of strstr(filename, "shadow") actually returns the pointer of the first strstr(filename, "shadow"). This must be a compiler or linker optimization... As a result the update_passwd() thinks it is updating the shadow file instead of the passwd file, and incorrectly setting the 2nd field in this file... More info: platform: ARM9 compiler: arm-none-linux-gnueabi v4.5.2 (Sourcery G++ Lite 2011.03-41) Tried the following with no avail: - removed the "FAST_FUNC" attribute - compiled using gcc version 4.4.1 (Sourcery G++ Lite 2009q3-67)