Bug 515361

Summary: tftp FORTIFY_SOURCE strcpy crash
Product: [Fedora] Fedora Reporter: Warren Togami <wtogami>
Component: tftpAssignee: Jiri Skala <jskala>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: aglotov, jakub, jskala, pertusus
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-08-05 14:31:53 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 473303    

Description Warren Togami 2009-08-03 21:10:00 UTC 
tftp-0.49-3.fc11 working binary
tftp-0.49-4.fc12 broken binary

tftp> get /path/to/file.txt
*** buffer overflow detected ***: tftp terminated
======= Backtrace: =========
/lib/libc.so.6(__fortify_fail+0x4d)[0xcdf1ad]
/lib/libc.so.6[0xcdd27a]
/lib/libc.so.6(__strcpy_chk+0x44)[0xcdc554]
tftp[0x8049025]
tftp[0x8049be2]
tftp[0x804a807]
tftp[0x804b39f]
/lib/libc.so.6(__libc_start_main+0xe6)[0xbfeb26]
tftp[0x8048f51]
======= Memory map: ========
00498000-004cd000 r-xp 00000000 00:10 5125       /lib/libreadline.so.6.0
004cd000-004d1000 rw-p 00035000 00:10 5125       /lib/libreadline.so.6.0
004d1000-004d2000 rw-p 00000000 00:00 0 
00708000-00728000 r-xp 00000000 00:10 4127       /lib/ld-2.10.90.so
00728000-00729000 r--p 0001f000 00:10 4127       /lib/ld-2.10.90.so
00729000-0072a000 rw-p 00020000 00:10 4127       /lib/ld-2.10.90.so
00755000-00760000 r-xp 00000000 00:10 4150       /lib/libnss_files-2.10.90.so
00760000-00761000 r--p 0000a000 00:10 4150       /lib/libnss_files-2.10.90.so
00761000-00762000 rw-p 0000b000 00:10 4150       /lib/libnss_files-2.10.90.so
00890000-00891000 r-xp 00000000 00:00 0          [vdso]
009fd000-00a13000 r-xp 00000000 00:10 4441       /lib/libtinfo.so.5.7
00a13000-00a16000 rw-p 00015000 00:10 4441       /lib/libtinfo.so.5.7
00aa7000-00ad1000 r-xp 00000000 00:10 4103       /lib/libgcc_s-4.4.1-20090725.so.1
00ad1000-00ad2000 rw-p 00029000 00:10 4103       /lib/libgcc_s-4.4.1-20090725.so.1
00be8000-00d5c000 r-xp 00000000 00:10 4134       /lib/libc-2.10.90.so
00d5c000-00d5e000 r--p 00174000 00:10 4134       /lib/libc-2.10.90.so
00d5e000-00d5f000 rw-p 00176000 00:10 4134       /lib/libc-2.10.90.so
00d5f000-00d62000 rw-p 00000000 00:00 0 
08048000-0804e000 r-xp 00000000 00:10 7932       /usr/bin/tftp
0804e000-0804f000 rw-p 00005000 00:10 7932       /usr/bin/tftp
0804f000-0806f000 rw-p 00000000 00:00 0 
08f0c000-08f2d000 rw-p 00000000 00:00 0          [heap]
b7ea4000-b7eab000 r--s 00000000 00:10 4434       /usr/lib/gconv/gconv-modules.cache
b7eab000-b80ab000 r--p 00000000 00:10 5848       /usr/lib/locale/locale-archive
b80ab000-b80ac000 rw-p 00000000 00:00 0 
b80b1000-b80b4000 rw-p 00000000 00:00 0 
bfba1000-bfbb6000 rw-p 00000000 00:00 0          [stack]
Aborted

gdb backtrace is not useful for FORTIFY crashes in identifying which strcpy is at fault.

http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html
jakub says this is likely effected by this compiler change.  If the binary is built with -D_FORTIFY_SOURCE=1 instead of 2 and it works, then this is confirmation that this is the issue.  I have not successfully built it with -D_FORTIFY_SOURCE=1 yet.
Comment 1 Warren Togami 2009-08-05 01:43:25 UTC 
tftp-hpa-0.49/tftp/tftp.c

static int
makerequest(int request, const char *name,
            struct tftphdr *tp, const char *mode)
{
    char *cp;

    tp->th_opcode = htons((u_short) request);
    cp = (char *)&(tp->th_stuff);
    strcpy(cp, name);
    cp += strlen(name);
    *cp++ = '\0';
    strcpy(cp, mode);
    cp += strlen(mode);
    *cp++ = '\0';
    return (cp - (char *)tp);
}

The first strcpy() is where it crashes.
Comment 2 Warren Togami 2009-08-05 02:03:49 UTC 
This function pre-processed.

static int
makerequest(int request, const char *name,
            struct tftphdr *tp, const char *mode)
{
    char *cp;

    tp->th_opcode = (__extension__ ({ register unsigned short int __v, __x = ((u_short) request); if (__builtin_constant_p (__x)) __v = ((((__x) >> 8) & 0xff) | (((__x) & 0xff) << 8)); else __asm__ ("rorw $8, %w0" : "=r" (__v) : "0" (__x) : "cc"); __v; }));
    cp = (char *)&(tp->th_u.tu_stuff);
    strcpy(cp, name);
    cp += strlen(name);
    *cp++ = '\0';
    strcpy(cp, mode);
    cp += strlen(mode);
    *cp++ = '\0';
    return (cp - (char *)tp);
}

/usr/include/arpa/tftp.h contains the definition of struct tftphdr.

struct  tftphdr {
        short   th_opcode;                      /* packet type */
        union {
                unsigned short  tu_block;       /* block # */
                short   tu_code;                /* error code */
                char    tu_stuff[1];            /* request packet stuff */
        } __attribute__ ((__packed__)) th_u;
        char    th_data[1];                     /* data or error string */
} __attribute__ ((__packed__));

#define th_block        th_u.tu_block
#define th_code         th_u.tu_code
#define th_stuff        th_u.tu_stuff
#define th_msg          th_data

jakub, do you need any more?
Comment 3 Jakub Jelinek 2009-08-05 07:22:17 UTC 
th_u isn't the last field in the struct, so flexible array member and similar exception definitely doesn't apply here.  You can use memcpy instead of strcpy (memcpy is always allowed to cross field boundaries), after all you are computing strlen (name) immediately afterwards anyway, so just trplacing
strcpy(cp, name); cp += strlen(name); *cp++ = '\0'; with size_t name_len = strlen(name) + 1; memcpy(cp, name, name_len); cp += name_len; and similarly for mode should work.