apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: Buffer overflow in apr_brigade_vprintf() ?
Date Fri, 24 Apr 2009 20:48:27 GMT
On Fri, Apr 24, 2009 at 4:10 PM, C. Michael Pilato <cmpilato@collab.net>wrote:

> [Please Cc: me in responses -- I think I still have APR commit privs, but
>  I'm not active here and not subscribed to the mailing lists.]
>
> In the past couple of weeks, I've seen two different reports of what
> appears
> to be corruption in the stream of data transmitted by Subversion's
> mod_dav_svn through Apache and back to the Subversion client.  What is seen
> client-side is an opening XML tag, a truncated bit of CDATA "inside" the
> tag, and then a missing XML closing tag.  The problem seems to occur with
> magically sized chunks of data, so it can be hard to reproduce[1].
>
> Here are the relevant pieces of the call stack:
>
> mod_dav_svn/reports/replay.c's change_file_or_dir_prop() function contains
> the following (which is base64-encoding Subversion file and directory
> properties, and tossing them into an XML REPORT request response):
>
>   const svn_string_t *enc_value = svn_base64_encode_string2(value, TRUE,
>                                                             pool);
>   SVN_ERR(dav_svn__send_xml
>            (eb->bb, eb->output,
>             "<S:change-%s-prop name=\"%s\">%s</S:change-%s-prop>" DEBUG_CR,
>             file_or_dir, qname, enc_value->data, file_or_dir));
>
> dav_svn__send_xml() is a wrapper around apr_brigade_vprintf().
>
> As you know, apr_brigade_vprintf() (in buckets/apr_brigade.c) looks like
> so:
>
> APU_DECLARE(apr_status_t) apr_brigade_vprintf(apr_bucket_brigade *b,
>                                              apr_brigade_flush flush,
>                                              void *ctx,
>                                              const char *fmt, va_list va)
> {
>    /* the cast, in order of appearance */
>    struct brigade_vprintf_data_t vd;
>    char buf[APR_BUCKET_BUFF_SIZE];
>    int written;
>
>    vd.vbuff.curpos = buf;
>    vd.vbuff.endpos = buf + APR_BUCKET_BUFF_SIZE;
>    vd.b = b;
>    vd.flusher = &flush;
>    vd.ctx = ctx;
>    vd.cbuff = buf;
>
>    written = apr_vformatter(brigade_flush, &vd.vbuff, fmt, va);
>
>    if (written == -1) {
>      return -1;
>    }
>
>    /* tack on null terminator to remaining string */
>    *(vd.vbuff.curpos) = '\0';
>
>    /* write out what remains in the buffer */
>    return apr_brigade_write(b, flush, ctx, buf, vd.vbuff.curpos - buf);
> }
>
> The function apr_vformatter() uses the buffer "buf" to format the string.
> This function in turn uses the macro INS_CHAR to add characters to the
> buffer.  INS_CHAR is defined like this:
>
> #define INS_CHAR(c, sp, bep, cc)                    \
> {                                                   \
>    if (sp) {                                       \
>        if (sp >= bep) {                            \
>            vbuff->curpos = sp;                     \
>            if (flush_func(vbuff))                  \
>                return -1;                          \
>            sp = vbuff->curpos;                     \
>            bep = vbuff->endpos;                    \
>        }                                           \
>        *sp++ = (c);                                \
>    }                                               \
>    cc++;                                           \
> }
>
> So, when the macro is executed to add a new character to the buffer and the
> buffer is full, the flush function is called to make room for the new
> character, and then the character is added.  Of course, if the buffer has
> room for exactly one more character, it is not flushed, the character is
> added, and the current position of the buffer is at its end (which is
> actually one byte beyond the space allocated for the buffer).
>
> After the call to apr_vformatter(), there will be stuff in the buffer.  In
> the special case above, the buffer may be perfectly full (perhaps after
> having been flushed one or more times, but still full now).  Then, without
> checking for that condition, this line is executed:
>
>    /* tack on null terminator to remaining string */
>    *(vd.vbuff.curpos) = '\0';
>
> Uh-oh.  Buffer overflow!
>
> Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
> apr_brigade_vprintf() like so:
>
>    char buf[APR_BUCKET_BUFF_SIZE + 1]
>
> (Note the "+ 1" to make room for that pesky NULL byte.)
>
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write?  Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data?  Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".


I agree; it won't have the terminating '\0' once it is written to the
brigade, and apr_brigade_write() doesn't need it.

Mime
View raw message