#829 'set mouse mouseformat' directly passing user defined format to 'snprintf()'

[Hiroki Motoyoshi]

In the current implementation of the command 'set mouse mouseformat USER-DEFINED-FORMAT', the format string provided by the user is passed directly to 'snprintf()' without any validation. While this typically does not cause issues when a well-formed format string is used, this approach is considered vulnerable and is not recommended (see Uncontrolled Format Strings).

Fortunately, gnuplot provides an internal function called 'f_sprintf()'. For this issue, since we only need to format two numeric values, x and y, it is preferable to use the internal 'f_sprintf()' function instead of the system's 'snprintf()'. The 'f_sprintf()' function is aware of the number of arguments and offers greater robustness against malformed format strings, as the formatting process is strictly controlled.

I have created a trial patch based on this approach.

[Ethan Merritt]

I worry about having mouse-tracking involve the evaluation stack. Since mouse tracking is asynchronous with the general program flow, I think there would be a possibility of stack corruption if both the main thread and the mouse-tracking thread were pushing and popping operations on the stack. I realize that if this is really a problem, it already exists in principle when you attach commands to a hot key using the bind command. But responding to a bound hotkey is a rare, single-shot, operation compared to continuous mouse tracking. I worry that the only reason we don't see corruption from hotkey operations is that the main program is usually quiescent while the user is interacting with the plot window, and hotkey operations cannot (I think) interrupt each other.

On the other hand, I do not buy the argument that deliberately malformed format strings are a "vulnerability" for an application like gnuplot. Normal operation of gnuplot already allows a user to read/write arbitrary files and issue system shell commands. If that user wants to trash their own files or crash the program, they don't need to do anything as complex as constructing a malformed format string. It would of course be a "vulnerability" If the user could trash someone else's files, but in that case the real security flaw would lie somewhere deeper in the operating system and would remain a vulnerability even if the gnuplot source code were changed.

[Ethan Merritt]

I have been trying to either confirm or disprove my concern that calling f_sprintf() from inside mouse handling might corrupt the evaluation stack. So far I have not been able to trigger any visible failure while testing. Furthermore an explicit test for stack depth added to the new mouse code never detects being called while the stack is active.

So at least tentatively I conclude that the patch is safe, or at least unlikely to trigger an error in practice.

As to the patch itself - If the program is going to call into its own formatting routines rather than using sprintf() directly, wouldn't it make more sense to call f_gprintf() rather than f_sprintf()? That way it could handle coordinates in time/date format as well as purely numeric output.

[Hiroki Motoyoshi]

Thank you for your considerations.

On the other hand, I do not buy the argument that deliberately malformed format strings are a "vulnerability" for an application like gnuplot.

I'm not entirely sure whether this constitutes a true vulnerability in gnuplot. However, I can point out that it is an incorrect usage of the 'snprintf' function. Because, the behavior is undefined if the number of format specifiers in the format string exceeds the number of arguments provided, and depends on the implementation of the C standard library.

This is an extreme example: the following script causes a segmentation fault on my environment (macOS).

NARGS=1000

set term wxt
fmt = ""; do for [k=1:NARGS] {fmt = fmt."%g " }
set mouse mouseformat fmt
plot 0

The NARGS value that will trigger the SEGV may depend on the stack size. This is not a problem with gnuplot's implementation itself, but rather an error that results from using user-provided format strings without validation.

As to the patch itself - If the program is going to call into its own formatting routines rather than using sprintf() directly, wouldn't it make more sense to call f_gprintf() rather than f_sprintf()? That way it could handle coordinates in time/date format as well as purely numeric output.

It would be ideal if that were possible, but unlike f_sprintf(), isn't f_gprintf() a function that formats only a single value?

[Ethan Merritt]

Yes, gprintf() formats only a single value. So it would have to be called twice, once for x and once for y. Several approaches come to mind.

It would be possible to look for two places in the format string where gprintf() should be called. Upside: This would preserve backward compatibility. Downside: I'm not sure how messy the code would be. The existing f_sprintf() code does this and you can see that's a lot of code. This case would be simpler, however, since we know there are exactly two values and they are each to be passed to gprintf().

Alternatively one could change the command syntax to be something like
set mouse mouseformat "xformat" {"yformat"}
If only one string is given, use it twice: once for x and once for y. Append the two fragments before printing. Upside: Easy to code. Downside: Not exactly backward compatible. Maybe it would have to be a new mouseformat option rather than a replacement for the old option 7.

[Hiroki Motoyoshi]

If the implementation with gprintf format support turns out to be too complicated, perhaps it would be better to recommend using 'set mouse mouseformat function ...' in scenarios where gprintf is desired.

set mouse mouseformat function sprintf("%s,%s",gprintf("%h",x),gprintf("%h",y))

[Ethan Merritt]

Ugh. I was completely wrong. gprintf() does not handle time formats. Worse, with that wrong thought in my head I revised the documentation to say (or at least imply) that it does handle time formats. I will go undo the damage I did to the documentation before getting back to this.

[Ethan Merritt]

OK. It's commit a2ec5006 in the development branch