session: Validate the token for correctness #1562
Conversation
whot
force-pushed
the
wip/validate-token
branch
from
3cd75da to
ad83903
Compare
The token is used as part of an object path so it has to meet those requirements. We can't escape it since the caller presumably expects to use the token as-is so where it fails the validity simply error out. Closes: flatpak#1549
whot
force-pushed
the
wip/validate-token
branch
from
ad83903 to
2d68212
Compare
| with pytest.raises(dbus.exceptions.DBusException) as excinfo: | ||
| request.call("CreateSession", options=options) |
|
|
||
| for (i = 0; token[i]; i++) | ||
| { | ||
| if (!g_ascii_isalnum(token[i]) && token[i] != '_') |
There was a problem hiding this comment.
Is _ a valid token? Doesn't gdbus already provide something like this?
There was a problem hiding this comment.
If this is being used in object paths, https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-object-path is the canonical specification.
gdbus probably doesn't have a function for "is this a valid object-path element?", though, just "is this a valid complete object path?" (which is g_variant_is_object_path()).
There was a problem hiding this comment.
You could validate that the result of g_strdup_printf ("/foo/%s", token) is a valid object path, which I think is what we actually require. That's pretty ugly, but is it less ugly than open-coding validation? I don't know.
g_dbus_is_member_name() is almost right, but member names are not allowed to start with digits, whereas object path elements are, so using that would be unnecessarily restrictive.
There was a problem hiding this comment.
Is
_a valid token?
If our only functional requirement is "can we append it to /blah/blah/ and get a valid object path?" then yes it is.
| return FALSE; | ||
| } | ||
|
|
||
| return TRUE; |
There was a problem hiding this comment.
This implementation would return true for the empty string, but "/blah/blah/" + "" is not a syntactically valid object path (an object path must not end with / unless it is exactly /). So we must check for that.
(We are probably appending it to some string in the /org/freedesktop/... namespace rather than literally /foo/ or /blah/blah/, but it's the "shape" that matters.)
|
|
||
| for (i = 0; token[i]; i++) | ||
| { | ||
| if (!g_ascii_isalnum(token[i]) && token[i] != '_') |
There was a problem hiding this comment.
| if (!g_ascii_isalnum(token[i]) && token[i] != '_') | |
| if (!g_ascii_isalnum (token[i]) && token[i] != '_') |
| @@ -311,6 +311,20 @@ xdp_session_authorize_callback (GDBusInterfaceSkeleton *interface, | |||
| return TRUE; | |||
| } | |||
|
|
|||
| static gboolean | |||
| is_valid_token(const char *token) | |||
There was a problem hiding this comment.
| is_valid_token(const char *token) | |
| is_valid_token (const char *token) |
| @@ -335,6 +349,15 @@ xdp_session_initable_init (GInitable *initable, | |||
| return FALSE; | |||
| } | |||
|
|
|||
| if (!is_valid_token(session->token)) | |||
There was a problem hiding this comment.
| if (!is_valid_token(session->token)) | |
| if (!is_valid_token (session->token)) |
| remotedesktop_intf = xdp.get_portal_iface(dbus_con, "RemoteDesktop") | ||
|
|
||
| request = xdp.Request(dbus_con, remotedesktop_intf) | ||
| options = {"session_handle_token": "Invalid-token&"} |
There was a problem hiding this comment.
Please can we specifically test "/foo" and "" here, too? Those are special cases would be easy to get wrong.
for badness in ("Invalid-token&", "/foo", ""):
options = { "session_handle_token": badness }
with pytest.raises (etc.)
...
The token is used as part of an object path so it has to meet those requirements. We can't escape it since the caller presumably expects to use the token as-is so where it fails the validity simply error out.
Closes: #1549