[squid-dev] [PATCH][v2] snprintf result used without validating its range

[Yuriy M. Kaminskiy]

Amos Jeffries <squid3 at treenet.co.nz> writes:

> On 10/02/2016 6:25 a.m., Yuriy M. Kaminskiy wrote:
>> In several cases, snprintf result was used without validating its range.
>> 
>> When formatted string would overflow buffer or error happens, snprintf
>> will return either value larger than buffer size, or -1. In both cases,
>> if you add this value to pointer (or similar), bad things will happen.
>> 
>> Pattern to watch for: =.*snprintf
>> 
>> I have not verified if any of this is exploitable. In some cases, I was
>> not sure about proper error handling (watch for XXX comments).
>> 
>> Patches compile-tested (however, only on linux/x86/gcc49 and in default
>> configuration).
>> 
>> 
>
> I've merge this one immediately:
>
>> squid-3.5.13-fix-typo.patch
>> Index: squid-3.5.13/src/ip/QosConfig.cc
>
> The rest are going to take a bit of reviw for portability and other
> compilers. I have vague recollections of something about that -1 and
> portability when I looked into it years ago.

Only "issue about -1" I'm aware about, in very ancient glibc (2.0.6 and
earlier), snprintf returned -1 on overflow (instead of "wanted size of
buffer"). But in all places that I've changed, existing code did not
handle error case *at all* (and would be broken badly if snprintf
returned -1). With my patch, it either ignores errors in a safe way, or
return error status to callers if possible.

========================================================

Alex Rousskov <rousskov at measurement-factory.com> writes:
> On 02/09/2016 10:25 AM, Yuriy M. Kaminskiy wrote:
>> +        xstrncpy(buf, "snprintf error in logfilePrintf\n", sizeof(buf));
> We should not write error messages to access.log. When an overflow

Changed.

> happens: If all logfilePrintf() callers cannot meaningfully handle the
> error anyway, then we should just log the error message to cache.log and
> return from logfilePrintf(). Otherwise, a more complex solution is needed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5.13-check-snprintf-value.v2.patch
Type: text/x-diff
Size: 17142 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160210/ed778cc0/attachment-0001.patch>