search

SWI-Prolog / packages-xpce

The graphics toolkit for SWI-Prolog
16 stars 14 forks source link

ws_legal_display_name #19

Closed koo5 closed 3 years ago

koo5 commented 3 years ago

This function doesn't do what it says.

https://www.x.org/archive/X11R6.8.1/doc/X.7.html says:

     From the user's perspective, every X server has a display name of the form:

hostname:displaynumber.screennumber

This information is used by the application to determine how it should connect to the server and which screen it should use by default (on displays with multiple monitors):

hostname
    The hostname specifies the name of the machine to which the display is physically connected. If the hostname is not given, the most efficient way of communicating to a server on the same machine will be used.

our sscanf string is: "%[a-zA-Z0-9.]:%d.%d"

http://www.cplusplus.com/reference/cstdio/scanf/ says:

Except for n, at least one character shall be consumed by any specifier. Otherwise the match fails, and the scan ends there.

therefore, our sscanf string requires a host, which is wrong. It's actually usual that host is omitted, and this test leads to misleading error message:

 [PCE fatal: @display/display: Failed to connect to X-server at `:0.0': malformed address: :0.0

(https://github.com/SWI-Prolog/packages-xpce/blob/master/src/x11/xdisplay.c#L233)

also note that the maximum number of characters read into host is not limited by LINESIZE, so this could in theory lead to memory corruption.

The original problem is obviously hard to fix with sscanf.

Shall we instead do: sprintf(problem, "Malformed address: %s, or no permission to contact X-server?", theaddress);

JanWielemaker commented 3 years ago

Thanks for spotting. Fixed with dfab7f0bbeb00bc7cce467220fcb8f72cbf634a3 (avoid sscanf() in the test and use snprintf() for the message). Note that this is not a real security issue. It only triggers if you have a ridiculously long value for $DISPLAY. If something manages to manipulate DISPLAY and start swipl the security is already breached.

koo5 commented 3 years ago

In practice i agree, but in theory, i can imagine people start using some new fancy cloud linux desktop service that embeds a token in there, for example. Anyway, thanks.