Bug 478324

Created attachment 327895 [details]
printstr-buffer-overflow.patch

The original printstr code quoted below could overflow the 'outstr' buffer because:

1. The size allocated for 'outstr' is 4*max_strlen + 6 bytes.
2. string_quote could potentially write 4*(max_strlen + 1) bytes to 'outstr' when 'size' is max_strlen + 1.
3. The strcat(outstr, "...") call would then write beyond 'outstr's allocated space.

void
printstr(struct tcb *tcp, long addr, int len)
{
        static char *str = NULL;
        static char *outstr;
        int size;

        if (!addr) {
                tprintf("NULL");
                return;
        }
        if (!str) {
                if ((str = malloc(max_strlen + 1)) == NULL
                    || (outstr = malloc(4*max_strlen
                                        + sizeof "\"\"...")) == NULL) {
                        fprintf(stderr, "out of memory\n");
                        tprintf("%#lx", addr);
                        return;
                }
        }

        if (len < 0) {
                size = max_strlen + 1;
                if (umovestr(tcp, addr, size, str) < 0) {
                        tprintf("%#lx", addr);
                        return;
                }
        }
        else {
                size = MIN(len, max_strlen + 1);
                if (umoven(tcp, addr, size, str) < 0) {
                        tprintf("%#lx", addr);
                        return;
                }
        }

        if (string_quote(str, outstr, len, size))
                strcat(outstr, "...");

        tprintf("%s", outstr);
}

Current code in cvs tree is different:

void
printstr(struct tcb *tcp, long addr, int len)
{
        static char *str = NULL;
        static char *outstr;
        int size;
        if (!addr) {
                tprintf("NULL");
                return;
        }
        if (!str)
                str = malloc(max_strlen + 1);
        if (!outstr)
                outstr = malloc(4 * max_strlen + sizeof "\"...\"");
        if (!str || !outstr) {
                fprintf(stderr, "out of memory\n");
                tprintf("%#lx", addr);
                return;
        }
        if (len < 0) {
                /*
                 * Treat as a NUL-terminated string: fetch one byte more
                 * because string_quote() quotes one byte less.
                 */
                size = max_strlen + 1;
                str[max_strlen] = '\0';
                if (umovestr(tcp, addr, size, str) < 0) {
                        tprintf("%#lx", addr);
                        return;
                }
        } else {
                size = MIN(len, max_strlen);
                if (umoven(tcp, addr, size, str) < 0) {
                        tprintf("%#lx", addr);
                        return;
                }
        }
        if (string_quote(str, outstr, len, size) &&
            (len < 0 || len > max_strlen))
                strcat(outstr, "...");
        tprintf("%s", outstr);
}

And string_quote() carries the following comment:
/*
 * Quote string `instr' of length `size'
 * Write up to (3 + `size' * 4) bytes to `outstr' buffer.
 * If `len' < 0, treat `instr' as a NUL-terminated string
 * and quote at most (`size' - 1) bytes.
 */

In the case "len < 0" string_quote() will put 3 + max_strlen*4 chars max, including NUL. Then "..." adds 3 more bytes, -> 6 + max_strlen*4.

Do you have a small example which overflows printstr()?

(In reply to comment #2)
 1. I re-ran my test case against CVS HEAD of trace.cvs.sourceforge.net and the new code indeed works.

 2. Is there a chance to get a new strace build with this fix for Fedora 10? I rely on strace to diagnose all sorts of random sysadmin and development problems and would really like it to not crash.

*** Bug 470529 has been marked as a duplicate of this bug. ***

From bug #470529, the full upstream fix for this is:

http://git.altlinux.org/people/ldv/packages/?p=strace.git;a=commitdiff;h=f46c65be75810812088b64fc05ecef8515d79b91

This issue was fixed in upstream git long time ago, see http://strace.git.sourceforge.net/git/gitweb.cgi?p=strace/strace;h=v4.5.18-17-ga501f14

strace-4.5.19-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/strace-4.5.19-1.fc10

strace-4.5.19-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/strace-4.5.19-1.fc11

strace-4.5.19-1.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update strace'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-10733

strace-4.5.19-1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update strace'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10843

strace-4.5.19-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

strace-4.5.19-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.