Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
From: bugzilla@apache.org
To: tomcat-dev@jakarta.apache.org
Subject: DO NOT REPLY [Bug 37332]  New:  -
    bounded buffer bug: incorrect use of [v]snprintf()
Message-Id: <20051102133409.106735BA@ajax.apache.org>
Date: Wed,  2 Nov 2005 14:34:09 +0100 (CET)

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=37332>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=37332

           Summary: bounded buffer bug: incorrect use of [v]snprintf()
           Product: Tomcat 5
           Version: 5.5.9
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P3
         Component: Native:JK
        AssignedTo: tomcat-dev@jakarta.apache.org
        ReportedBy: Joerg-Cyril.Hoehle@t-systems.com


Hi,

jk_log() in
jakarta-tomcat-connectors-1.2.14.1-src/jk/native/common/jk_util.c:jk_log
jakarta-tomcat-5.5.9-src/jakarta-tomcat-connectors/jk/native/common/jk_util.c
does not correctly protect its temporary buffer.

First, from a style & review perspective, I would like to say I prefer
to see a single function (and even whole files) use a consistent style
for acessing buffer parts. Currently, jk_log() uses both &buf[used]
and buf+used when calling v/s/nprintf().
But that has nothing to do with the bug.

Actually, there are several bugs involving use of snprintf()-style functions:

1. snprintf(&buf[used], HUGE_BUFFER_SIZE, "%s (%d): ",...); (line 341)
It should be BUFFER_SIZE-used, since the buffer is no more empty.

2. used += snprintf(&buf[used], ...);
3. if (used < 0) ...
4. local return without free()

snprintf() is documented (as the manpage notes, "since glibc 2.1 these
functions follow the C99 standard") to return "the number of characters
... which would have been written if enough space had been available."

As a result, used may grow larger than BUFFER_SIZE, and
BUFFER_SIZE-used can become negative, or rather, since snprintf()
accepts n as size_t, which is typedef'd as an unsigned long on many a
system, it becomes a really huge positive entity, in effect
 a) suppressing the boundary check for subsequent calls,
 b) yielding an out of bound buf[used] for subsequent calls.

The code must instead detect the buffer full condition and stop
calling snprintf() style functions, e.g. by premature return from the
function. Note that doing so is against many a style guide, since it
typically omits freeing resources at the end of the function body, e.g.
#ifdef NETWARE free(buf)
is not being called when exiting in lines 322 and 345 of jk_util.c.

>From a robustness point of view, and to favour a consistent style, I'd
like to recommend the following style:
 used = 0;
 used += snprintf(buf+used,SIZE-used,fmt,...); // with glibc >= 2.1
 if (used >= SIZE) break_safely;
 used += snprintf(buf+used,SIZE-used,fmt,...); // with glibc >= 2.1

Alternatively, the local break or exit could be replaced by nested
 if (used < SIZE) { 
   more_snprintf(...);
   if (used < SIZE) ...
 }
but deep nesting, again, is against many a coding style.
[I digress, please contact me if you are interested in coding style
 that attempts to consider both security *and* not feel like bondage.]

If it is not sure that glibc >= 2.1 is used, then the code should use
an intermediate return value and detect the -1 return value. Using
 used += snprintf(); if (used<0) ...
as currently done in jk_log() is *not* correct, since all that happens
is that `used' gets decremented by 1.

Actually, my preference goes to printf()-style functions oriented
towards BSD's strlcat() and strlcpy(), where the limit argument
indicates the total buffer size instead of the max. number of bytes to
copy.
http://www.usenix.org/events/usenix99/millert.html
http://developer.apple.com/documentation/Darwin/Reference/ManPages/man3/strlcpy.3.html

One would then use a very readable and fool-proof sequence of
 slprintf(buf,SIZE,fmt,...);
 slprintf(buf,SIZE,fmt,...);
 slprintf(buf,SIZE,fmt,...);
and not worry about the tiny efficiency loss of having to seek past the
\0-terminator that strlen() and str[ln]cat() suffer (in theory O(nxm)
instead of O(n)).
BTW, jk_util.c already suffers such penalty by using
            strcat(buf, jk_level_werbs[level]);
            used += 8;
instead of the slightly more efficient
	strcpy(buf+used,...);

>From a security, reliability and maintenance point of view, I very
much favour readability and not having to worry about keeping track of
the number of used elements, over optimized, hard to read code that
must maintain correct buf+used values and contains local exits to stop
appending when the buffer is full.

Maybe this bug can be misused since an overflow may be caused by a
very long filename supplied to Tomcat, but I did not investigate further.

I found this group of bugs while performing a code inspection on
behalf of BSI, the german Federal Office for Information Security.
 -- Bundesamt f�r Sicherheit in der Informationstechnik.
http://www.bsi.de

BSI endorses the use of Open Source Software. A report of our
activities covering installation, code and penetration tests of Tomcat
will be published by BSI in the internet at the end of our review
project.

Regards,
 J�rg H�hle.
T-Systems International, Solution Center Testing & Security
http://www.t-systems.com

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org