Project

General

Profile

Bug #11280

custr in belt but not braces, bungles vsnprintf() buffer length

Added by Joshua M. Clulow 4 months ago. Updated 4 months ago.

Status:
Closed
Priority:
Normal
Category:
lib - userland libraries
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

For reasons best left unexplored, I built and ran a program that makes use of the custr family of functions on a SPARC machine. This program which has worked correctly on x86 systems on at least three platforms (illumos, Linux, and Mac OS X) did not work on the SPARC machine in a fairly spectacular fashion:

$ ./proto/SunOS.sparc/make_ssh_config -h xxx -d ../ssh >/dev/null
make_ssh_config: custr_append_printf: Value too large for defined data type

The custr_append_printf() function was failing with the EOVERFLOW error code. We will return this explicitly for the overrun of a fixed buffer custr_t, but this was a dynamic custr_t object. Looking at the code, that routine is implemented in terms of vsnprintf() underneath, which can fail with EOVERFLOW:

       The snprintf() function will fail if:

       EOVERFLOW
                    The value of n is greater than INT_MAX or the number of
                    bytes needed to hold the output excluding the terminating
                    null is greater than INT_MAX.

This didn't seem right: I was appending a short string to a previous empty buffer! It turns out that we're attempting to construct a bounds checking length to cover the remaining (unoccupied) end of the buffer into which we're using vsnprintf() to write the appended string. While doing this, we're using the buffer pointer, not the buffer length -- this is pretty obviously not what was intended:

123         len = vsnprintf(cus->cus_data + cus->cus_strlen,
124             (uintptr_t)cus->cus_data - (uintptr_t)cus->cus_strlen, fmt, ap);

This doesn't end up causing a warning because the pointer is cast to uintptr_t. Why, you might wonder, does this only fail on SPARC? Looking at the pointer and size values passed to the second vsnprintf() call, we see that they're all less than INT_MAX:

$ file bin/sunos/i386/make_ssh_config 
bin/sunos/i386/make_ssh_config: ELF 64-bit LSB executable AMD64 Version 1, dynamically linked, not stripped, no debugging information available

$ pfexec dtrace -w -n 'struct custr { size_t cus_strlen; size_t cus_datalen; char *cus_data; int flags; };
     pid$target::custr_append_vprintf:entry { self->x = 1; }
     pid$target::custr_append_vprintf:return { self->x = 0; }
     pid$target::vsnprintf:entry /arg0 != NULL && self->x/ {
         this->cus = (userland struct custr *)arg0;
         @[arg0, arg1, arg1 > 2147483647 ? "yes" : "no"] = count();
     }
     END {
         freopen("/dev/stderr");
         printa("%016X %016X %4s %@d\n", @);
     }' -c 'bin/sunos/i386/make_ssh_config -h xxx -d ssh' >/dev/null
dtrace: description 'struct custr ' matched 4 probes
dtrace: allowing destructive actions
dtrace: pid 159249 has exited
000000000041A547 000000000041A539   no 8
000000000041A4F0 000000000041A4F0   no 67
000000000041A546 000000000041A53A   no 81
000000000041A545 000000000041A53B   no 121
000000000041A540 000000000041A540   no 229
000000000041A541 000000000041A53F   no 229
000000000041A542 000000000041A53E   no 229
000000000041A543 000000000041A53D   no 229
000000000041A544 000000000041A53C   no 229

On SPARC, they're decidedly not:

$ file proto/SunOS.sparc/make_ssh_config
proto/SunOS.sparc/make_ssh_config:      ELF 64-bit MSB executable SPARCV9 Version 1, dynamically linked, not stripped, no debugging information available

$ pfexec dtrace -w -n 'struct custr { size_t cus_strlen; size_t cus_datalen; char *cus_data; int flags; };
    pid$target::custr_append_vprintf:entry { self->x = 1; }
    pid$target::custr_append_vprintf:return { self->x = 0; }
    pid$target::vsnprintf:entry /arg0 != NULL && self->x/ {
        this->cus = (userland struct custr *)arg0;
        @[arg0, arg1, arg1 > 2147483647 ? "yes" : "no"] = count();
    }
    END {
        freopen("/dev/stderr");
        printa("%016X %016X %4s %@d\n", @);
    }' -c 'proto/SunOS.sparc/make_ssh_config -h xxx -d ../ssh' >/dev/null

dtrace: description 'struct custr ' matched 4 probes
dtrace: allowing destructive actions
make_ssh_config: custr_append_printf: Value too large for defined data type
dtrace: pid 957 has exited
000000010010ABB0 000000010010ABB0  yes 1

Fixing the bug gets everything under control again:

$ pfexec dtrace -w -n 'struct custr { size_t cus_strlen; size_t cus_datalen; char *cus_data; int flags; };
    pid$target::custr_append_vprintf:entry { self->x = 1; }
    pid$target::custr_append_vprintf:return { self->x = 0; }
    pid$target::vsnprintf:entry /arg0 != NULL && self->x/ {
        this->cus = (userland struct custr *)arg0;
        @[arg0, arg1, arg1 > 2147483647 ? "yes" : "no"] = count();
    }
    END {
        freopen("/dev/stderr");
        printa("%016X %016X %4s %@d\n", @);
    }' -c 'proto/SunOS.sparc/make_ssh_config -h xxx -d ../ssh' >/dev/null
dtrace: description 'struct custr ' matched 4 probes
dtrace: allowing destructive actions
dtrace: pid 1021 has exited
000000010010AC07 0000000000000039   no 8
000000010010ABB0 0000000000000040   no 68
000000010010AC06 000000000000003A   no 84
000000010010AC05 000000000000003B   no 125
000000010010AC00 0000000000000040   no 236
000000010010AC01 000000000000003F   no 236
000000010010AC02 000000000000003E   no 236
000000010010AC03 000000000000003D   no 236
000000010010AC04 000000000000003C   no 236

Chalk one up for a second CPU architecture, I suppose!

(Note, this does not appear to have serious security implications because we've already used the input once in the initial call to vsnprintf() to determine how long the string will be and then allocated at least that much space. The second bounds check is, thus, not load-bearing.)

History

#1

Updated by Joshua M. Clulow 4 months ago

  • Description updated (diff)
#2

Updated by Joshua M. Clulow 4 months ago

Testing Notes

I worked out a basic program that overrides malloc(), allowing us to force buffer addresses higher than INT_MAX. The test fails before fixing:

$ ./test 
start program
mapped pre_buf @        200000000
alloc sz 32 -> actual size 32 -> buf @        200000000
alloc sz 64 -> actual size 64 -> buf @        200000020
test: custr append (0): alloc sz 48 -> actual size 64 -> buf @        200000060
Value too large for defined data type

And succeeds after:

$ ./test
start program
mapped pre_buf @        200000000
alloc sz 32 -> actual size 32 -> buf @        200000000
alloc sz 64 -> actual size 64 -> buf @        200000020
alloc sz 128 -> actual size 128 -> buf @        200000060
alloc sz 192 -> actual size 256 -> buf @        2000000e0
alloc sz 256 -> actual size 256 -> buf @        2000001e0
alloc sz 320 -> actual size 512 -> buf @        2000002e0
alloc sz 384 -> actual size 512 -> buf @        2000004e0
alloc sz 448 -> actual size 512 -> buf @        2000006e0
alloc sz 512 -> actual size 512 -> buf @        2000008e0
alloc sz 576 -> actual size 1024 -> buf @        200000ae0
alloc sz 640 -> actual size 1024 -> buf @        200000ee0
alloc sz 704 -> actual size 1024 -> buf @        2000012e0
alloc sz 768 -> actual size 1024 -> buf @        2000016e0
alloc sz 832 -> actual size 1024 -> buf @        200001ae0
alloc sz 896 -> actual size 1024 -> buf @        200001ee0
alloc sz 960 -> actual size 1024 -> buf @        2000022e0
alloc sz 1024 -> actual size 1024 -> buf @        2000026e0
end program

Test program:

#include <stdlib.h>
#include <stdio.h>
#include <libcustr.h>
#include <limits.h>
#include <err.h>
#include <sys/mman.h>

static uintptr_t pre_pos = 0;
static void *pre_buf = NULL;

void *
malloc(size_t sz)
{
        size_t actual = 1;

        while (actual < sz) {
                actual *= 2;
        }

        if (pre_buf == NULL) {
                /*
                 * Aim for a memory location that will be bigger than INT_MAX.
                 */
                void *targ = (void *)(((uintptr_t)INT_MAX + 1) * 4);

                if ((pre_buf = mmap(targ, 64 * 1024 * 1024,
                    PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0)) ==
                    MAP_FAILED) {
                        err(1, "mmap");
                }

                printf("mapped pre_buf @ %16p\n", pre_buf);
        }

        void *ret = (void *)((uintptr_t)pre_buf + pre_pos);
        pre_pos += actual;

        printf("alloc sz %lu -> actual size %lu -> buf @ %16p\n", sz, actual,
            ret);

        return (ret);
}

void
free(void *buf)
{
        /*
         * This is a basic bump allocator; we'll just ignore free() calls.
         */
}

int
main(int argc, char *argv[])
{
        printf("start program\n");

        custr_t *cu;
        if (custr_alloc(&cu) != 0) {
                err(1, "custr_alloc");
        }

        for (int i = 0; i < 1000; i++) {
                if (custr_appendc(cu, '-') != 0) {
                        err(1, "custr append (%d)", i);
                }
        }

        custr_reset(cu);

        if (custr_append_printf(cu, "end") != 0 ||
            custr_appendc(cu, ' ') != 0 ||
            custr_append(cu, "program\n") != 0) {
                err(1, "custr append");
        }

        printf(custr_cstr(cu));
}

Built in usr/src/lib/libcustr via:

gcc -Wall -Wextra -Werror -Wno-unused-parameter -m64 -R./amd64 -Icommon -o test test.c $PWD/amd64/libcustr.so.1
#4

Updated by Electric Monk 4 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 09cb6b0fb8744ab06f86a15ad49eb1f39c6fcbf1

commit  09cb6b0fb8744ab06f86a15ad49eb1f39c6fcbf1
Author: Joshua M. Clulow <josh@sysmgr.org>
Date:   2019-07-04T19:17:28.000Z

    11280 custr in belt but not braces, bungles vsnprintf() buffer length
    Reviewed by: Jason King <jason.king@joyent.com>
    Reviewed by: Andy Fiddaman <omnios@citrus-it.co.uk>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF