[cfe-dev] vprintf(3) and "format string is not a string literal"

[Shantonu Sen]

On Dec 16, 2007, at 9:08 PM, Ted Kremenek wrote:

> Hi Shantonu,
>
> We probably should make these kinds of warnings optional.

See below

>  In  case you are not aware, format-string attacks are an extremely  
> serious and often exploited attack vector for a hacker to compromise  
> a system, and have been the source of much grief. Clearly the  
> logmessage function in your example code snippet is a possible  
> exception because it serves as a wrapper to vprintf (so this warning  
> could be considered a false positive).  If the format string checker  
> was more powerful (i.e., it performed an inter-procedural program  
> analysis), it should similarly flag casual uses of the logmessage  
> function where the format string is not specified with a string  
> constant.

There would still be cases where you register a function like  
logmessage() with a system library API as a callback, so LLVM would  
not know either at compile time or runtime what the callers are.

> Your question specifically regarding vprintf is an interesting one.   
> Clearly vprintf was designed with the idea of passing in arguments  
> from some other source, be it via parameters from a caller of the  
> current function or pulled from a data structure.  Depending on how  
> careful you are, this can be okay (such as with the logmessage  
> function), but in general it's just a bad idea.  The va_list  
> argument to vprintf (and friends) allows a tremendous amount of  
> flexibility in how these functions are used (specifically be a  
> dynamically specified argument list almost always implies a  
> dynamically specified format string); the horribly consequence is  
> that this code is very difficult to check statically for  
> correctness, and can be the source of awful security holes and other  
> bugs when these functions aren't used properly.  Unfortunately,  
> people often underestimate how easy it is to screw up how these  
> functions are used, either when they are called directly or called  
> indirectly via wrappers.

I think what I'd like the eventual behavior to be is:
1) Functions that call printf(3) directly should have the warning  
enabled
2) Functions that call vprintf(3) and all callers are known, enable  
the warning
3) If the function calling vprintf(3) is itself annotated with  
something like GCC's __attribute__((__format__ ... )), make it the  
responsibility of the second-order caller to verify arguments
4) Otherwise disable it.

For comparison, GCC has:
        -Wformat-nonliteral
            If -Wformat is specified, also warn if the format string  
is not a
            string literal and so cannot be checked, unless the format  
function
            takes its format arguments as a "va_list".


> Ultimately, we should probably make warnings like these an option.   
> People can then decide their own policy on when such warnings are  
> emitted.

How about this:
$ clang -std=c99 a.c
a.c:10:23: warning: format string is not a string literal (potentially  
insecure)
         ret = vprintf(fmt, ap);
               ~~~~~~~ ^
1 diagnostic generated.
$ clang -std=c99 a.c -Wno-format-nonliteral
$

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.cpp.diff
Type: application/octet-stream
Size: 1109 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20071217/10b3d431/attachment.obj>
-------------- next part --------------



Shantonu

> Ted
>
> On Dec 16, 2007, at 8:03 PM, Shantonu Sen wrote:
>
>> I don't understand the following warning:
>>
>> $ cat a.c
>> #include <stdarg.h>
>> #include <stdio.h>
>>
>> int logmessage(int loglevel, char const *fmt, ...) {
>>    int ret = 0;
>>    va_list ap;
>>
>>    if (loglevel > 1) {
>>        va_start(ap, fmt);
>>        ret = vprintf(fmt, ap);
>>        va_end(ap);
>>    }
>>    return ret;
>> }
>> $ clang -std=c99 a.c
>> a.c:10:23: warning: format string is not a string literal  
>> (potentially
>> insecure)
>>        ret = vprintf(fmt, ap);
>>              ~~~~~~~ ^
>> 1 diagnostic generated.
>>
>> This seems counter-intuitive to the point of the vprintf(3) API,  
>> which
>> is to pass the format string and arguments from its caller
>> (logmessage()) in this case. When would vprintf(3) ever realistically
>> be called with a string literal? There seems to be test cases and
>> explicit code for this, so I'm guessing this is intentional, but I
>> don't quite understand why...
>>
>> Shantonu Sen
>> ssen at apple.com
>>
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>