Bug 94522 - Unsafe vsprintf usage in TestNetscapePlugin
Summary: Unsafe vsprintf usage in TestNetscapePlugin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-20 13:37 PDT by Nate Chapin
Modified: 2012-08-20 16:39 PDT (History)
4 users (show)

See Also:


Attachments
patch (2.63 KB, patch)
2012-08-20 13:41 PDT, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff
patch (2.85 KB, patch)
2012-08-20 14:21 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2012-08-20 13:37:27 PDT
Original report: http://code.google.com/p/chromium/issues/detail?id=141806

This is test-only code, so not marking it as a security issue.  However, it is triggering crashes in ClusterFuzz, so it seems worth it to clean up.

Using vsnprintf and a proper buffer size seems to fix the problem.
Comment 1 Nate Chapin 2012-08-20 13:41:55 PDT
Created attachment 159511 [details]
patch
Comment 2 Adam Barth 2012-08-20 13:47:30 PDT
Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>      char message[2048] = "PLUGIN: ";
> -    vsprintf(message + strlen(message), format, args);
> +    vsnprintf(message + strlen(message), 2040, format, args);

Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.
Comment 3 Nate Chapin 2012-08-20 13:49:04 PDT
(In reply to comment #2)
> (From update of attachment 159511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review
> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
> >      char message[2048] = "PLUGIN: ";
> > -    vsprintf(message + strlen(message), format, args);
> > +    vsnprintf(message + strlen(message), 2040, format, args);
> 
> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.

Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer?

Good point on the '\0'.
Comment 4 Thomas Sepez 2012-08-20 13:51:02 PDT
Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>> +    vsnprintf(message + strlen(message), 2040, format, args);
> 
> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.

Also, so long as your calling strlen(), seems like both of these values should derive from that - as in 
2048 - strlen(message).  But don't call strlen() twice.  sizeof("PLUGIN: ") -1 is also a suitable alternate.
Comment 5 Adam Barth 2012-08-20 13:51:50 PDT
Comment on attachment 159511 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review

>>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66
>>> +    vsnprintf(message + strlen(message), 2040, format, args);
>> 
>> Why 2040 and not 2048 ?  Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions.
> 
> Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer?
> 
> Good point on the '\0'.

I see.  Yeah, that's easy to for dumb people like me to miss.  :)

const size_t messageBufferSize = 2048;
char message[2048] = "PLUGIN: ";
messageLength = strlen(message);
vsnprintf(message + messageLength, messageBufferSize - messageLength, format, args);
message[messageBufferSize - 1] = '\0';
Comment 6 Thomas Sepez 2012-08-20 13:57:24 PDT
> char message[2048] = "PLUGIN: ";
> messageLength = sizeof("PLUGIN: ") - 1;
> vsnprintf(message + messageLength, sizeof(message) - messageLength, format, args);
> message[sizeof(message) - 1] = '\0';
Comment 7 Adam Barth 2012-08-20 14:02:44 PDT
tsepez puts me to shame, as usual :)
Comment 8 Thomas Sepez 2012-08-20 14:13:49 PDT
Still not happy that we say "Plugin" twice.  You should be able to tweak that string or the dimension of the buffer in one place and only one place and still have the code work.  Let's see:

> char message[2048];
> static const char plugin[] = "PLUGIN: ";
> memcpy(message, plugin, sizeof(plugin) - 1);
> vsnprintf(message + sizeof(plugin) - 1, sizeof(message) - (sizeof(plugin) - 1), format, args);
> message[sizeof(message) - 1] = '\0';

But I've not tried it.
Comment 9 Nate Chapin 2012-08-20 14:21:34 PDT
Created attachment 159518 [details]
patch
Comment 10 Adam Barth 2012-08-20 14:32:46 PDT
Comment on attachment 159518 [details]
patch

This isn't my favorite idiom, but whatever man.
Comment 11 Nate Chapin 2012-08-20 14:35:32 PDT
(In reply to comment #10)
> (From update of attachment 159518 [details])
> This isn't my favorite idiom, but whatever man.

The way I wrote the code, or any code that matches .*printf.* ? :)
Comment 12 Adam Barth 2012-08-20 14:38:20 PDT
> The way I wrote the code, or any code that matches .*printf.* ? :)

:)
Comment 13 Adam Barth 2012-08-20 14:39:16 PDT
I think the root of the issue is that .*printf.* are so impossible to use correctly that they make the code that calls them super ugly, regardless of how it's written.
Comment 14 WebKit Review Bot 2012-08-20 16:39:54 PDT
Comment on attachment 159518 [details]
patch

Clearing flags on attachment: 159518

Committed r126089: <http://trac.webkit.org/changeset/126089>
Comment 15 WebKit Review Bot 2012-08-20 16:39:58 PDT
All reviewed patches have been landed.  Closing bug.