Commit Graph

6209 Commits

Author SHA1 Message Date
Philip Withnall
5054b48b7c gdbusmessage: Limit recursion of variants in D-Bus messages
This is the analogue of commit 7c4e6e9fbe, but applied to the
`GDBusMessage` parser, which does its own top-level parsing of the
variant format in D-Bus messages.

Previously, this code allowed arbitrary recursion of variant containers,
which could lead to a stack overflow. Now, that recursion is limited to
64 levels, as per the D-Bus specification:

https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-signature

This includes a new unit test.

oss-fuzz#14870

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-11-18 17:53:55 +00:00
Philip Withnall
de2236584d tests: Tidy up test case naming in gdbus-serialization test
Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-11-15 10:12:59 +00:00
Philip Withnall
96d792197a gdbusmessage: Move variable initialisation to declaration time
Tidies up the code a bit, but introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-11-15 10:12:59 +00:00
Alex Samorukov
1bb1bcd814 Add NOTE_REVOKE to the list of the monitoring events 2019-11-11 17:59:31 +00:00
Philip Withnall
963786f608 Merge branch 'wip/smcv/diagnose-bind-failure' into 'master'
gsocket: Improve diagnostics on bind() failure

See merge request GNOME/glib!1213
2019-11-04 16:59:16 +00:00
Philip Withnall
4d65f32703 Merge branch 'wip/smcv/cross-ld' into 'master'
Make ld executable configurable

See merge request GNOME/glib!1209
2019-11-04 16:26:24 +00:00
Simon McVittie
e08dffb71b gsocket: Improve diagnostics on bind() failure
This is in an attempt to diagnose GNOME/glib#1912.

Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-11-04 15:09:15 +00:00
Simon McVittie
81936ca580 Make ld executable configurable
Tools like this should be configurable in a cross or native file. In
particular, if we are cross-compiling (with an executable wrapper like
qemu-arm), the build system ld is not necessarily able to manipulate
host system objects.

Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-11-04 15:08:33 +00:00
Simon McVittie
49eccfbe70 gdbus-server-auth test: Include gcredentialsprivate.h
Otherwise we'll never test the EXTERNAL-only mode, because that relies
on testing the private macros
G_CREDENTIALS_UNIX_CREDENTIALS_MESSAGE_SUPPORTED and
G_CREDENTIALS_SOCKET_GET_CREDENTIALS_SUPPORTED.

Fixes: 9f962ebe "Add a test for GDBusServer authentication"
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-11-04 15:07:23 +00:00
Simon McVittie
7021b84f10 gdbus-peer: Specifically listen on 127.0.0.1
This bypasses any issues we might have with containers where IPv6 is
returned by name resolution (particularly since GNOME/glib!616) but
doesn't necessarily actually work.

This comes at a minor test-coverage cost: we don't test GDBusServer's
default behaviour when told to listen on "tcp:" or "nonce-tcp:", and
on systems where IPv6 is available, we don't test it. If we want to
do those, we should perhaps do them in separate tests, and disable
those tests when binding to ::1 doesn't work.

Mitigates: GNOME/glib#1912
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-11-04 13:31:14 +00:00
Philip Withnall
9b8ff414c1 Merge branch 'no-more-automatic-GUnixFDList' into 'master'
Revert "gdbus-codegen: emit GUnixFDLists if an arg has type 'h'"

See merge request GNOME/glib!1171
2019-10-31 13:42:55 +00:00
Mattias Bengtsson
4aaeac5b3c gdbus-codegen: Safer header guards
Whitelist a safe set of characters for use in header guards instead of
maintaining a (growing) blacklist.

The whitelist is intentionally short since reading up on all
peculiarities of the C and C++ standard for identifiers is not my idea
of fun. :)

Fixes #1379
2019-10-31 12:50:30 +00:00
Philip Withnall
623bdfd7ab Merge branch 'wip/smcv/gdbus-tests' into 'master'
Fix GDBus test failures on non-Linux (in particular FreeBSD)

Closes #1920 and #1921

See merge request GNOME/glib!1197
2019-10-31 12:38:12 +00:00
Philip Withnall
1503547766 Merge branch 'param-value-default' into 'master'
Allow using an empty GValue with g_param_value_set_default()

See merge request GNOME/glib!1186
2019-10-31 10:22:57 +00:00
Philip Withnall
15818926b3 glocalfileinfo: Fix minor leak on error handling path for xattrs
Spotted by `scan-build`.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-30 15:35:15 +00:00
Philip Withnall
465b4dadf3 tests: Use g_assert_*() rather than g_assert() in live-g-file test
g_assert() is compiled out by `G_DISABLE_ASSERT` and doesn’t give such
useful messages on failure.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-30 15:19:34 +00:00
Philip Withnall
c25e8ec1f2 tests: Drop pointless assignments
Or turn them into proper error checks. This shuts up some `scan-build`
warnings.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-30 15:19:34 +00:00
Philip Withnall
dd5132e041 tests: Tidy up GError assignment
No need to clear it to NULL before every time it’s used, since we assert
that it’s never set.

This introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-30 15:19:34 +00:00
Simon McVittie
2b1e706b2f gdbus-server-auth test: Create temporary directory for Unix socket
This avoids failure to listen on the given address on non-Linux Unix
kernels, where abstract sockets do not exist and so unix:tmpdir is
equivalent to unix:dir.

To avoid bugs like this one recurring, run most of these tests using
the unix:dir address type, where Linux is equivalent to other Unix
kernels; just do one unix:tmpdir test, to check that we still
interoperate with libdbus when using abstract sockets on Linux.

Resolves: GNOME/glib#1920
Fixes: 9f962ebe "Add a test for GDBusServer authentication"
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-29 16:36:19 +00:00
Simon McVittie
bab277fd50 gdbus-peer test: Use unix:dir address if exact format doesn't matter
Previously, we used unix:tmpdir, except in tests that verify that a
particular address type works (notably unix:dir). Now we use unix:dir
most of the time, and unix:tmpdir gets its own test instead.

This helps to ensure that the tests continue to work on non-Linux Unix
kernels, where abstract sockets do not exist and so unix:tmpdir is
equivalent to unix:dir, even in the common case where the developer has
only tried the test on Linux.

Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-29 16:35:46 +00:00
Simon McVittie
7c2e4095f4 gdbus-peer test: Stop GDBusServer before tearing down temporary directory
Otherwise, since GNOME/glib!1193, the listening socket won't be deleted,
and if we are not using abstract sockets (for example on *BSD), g_rmdir
will fail with ENOTEMPTY.

Fixes: 8e32b8e8 "gdbusserver: Delete socket and nonce file when stopping server"
Resolves: GNOME/glib#1921
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-29 16:35:06 +00:00
Simon McVittie
e1b7b1ac16 gdbus-peer test: Improve diagnostics if g_rmdir fails
Helps: GNOME/glib#1921
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-29 16:35:04 +00:00
Simon McVittie
4e8d058828 Merge branch '1912-gdbus-peer-test' into 'master'
tests: Isolate directories in gdbus-peer test

Closes #1912

See merge request GNOME/glib!1192
2019-10-29 08:37:22 +00:00
Simon McVittie
714f9f92b7 Merge branch '1318-dbus-server-race' into 'master'
gdbusserver: Keep a strong reference to the server in callbacks

Closes #1318

See merge request GNOME/glib!1193
2019-10-29 08:28:56 +00:00
Philip Withnall
0c07e672a2 gdbusserver: Keep a strong reference to the server in callbacks
The `on_run()` function could be executed in any worker thread from the
`GThreadedSocketListener`, but didn’t previously hold a strong reference
to the `GDBusServer`, which meant the server could be finalised in
another thread while `on_run()` was still running.

This was not ideal.

Hold a strong reference to the `GDBusServer` while the socket listener
is listening, i.e. between every paired call to `g_dbus_server_start()`
and `g_dbus_server_stop()`.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #1318
2019-10-28 20:47:04 +00:00
Philip Withnall
8e32b8e87f gdbusserver: Delete socket and nonce file when stopping server
Rather than when finalising it. They should be automatically recreated
if the server is re-started.

This is important for ensuring that all externally visible behaviour of
the `GDBusServer` is synchronised with calls to
g_dbus_server_{start,stop}(). Finalisation of the server object could
happen an arbitrarily long time after g_dbus_server_stop() is called.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1318
2019-10-28 20:47:04 +00:00
Philip Withnall
d44e00fb98 Merge branch 'issue1831' into 'master'
Avoid race condition authenticating GDBusServer with libdbus client (#1831)

Closes #1831

See merge request GNOME/glib!1176
2019-10-28 20:44:30 +00:00
Philip Withnall
6fb38c3f25 tests: Isolate directories in gdbus-peer test
So that the tests all end up using separate `.dbus-keyring` directories,
and hence not racing to create and acquire lock files, use
`G_TEST_OPTION_ISOLATE_DIRS` to ensure they all run in separate
disposable directories.

This has the added benefit of meaning they don’t touch the developer’s
actual `$HOME` directory.

This reduces the false-failure rate of `gdbus-peer` by a factor of 9 for
me on my local machine.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #1912
2019-10-28 20:36:51 +00:00
Philip Withnall
833579d982 tests: Move main loop and test GUID into test functions in gdbus-peer
There’s actually no need for them to be global or reused between unit
tests, so move them inside the test functions.

This is one step towards eliminating shared state between the unit
tests.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1912
2019-10-28 20:17:07 +00:00
Philip Withnall
9df8d76c97 gdbusauthmechanismsha1: Create .dbus-keyrings directory recursively
If the directory is overridden, for example when running tests, the
parent directory of `.dbus-keyrings` (i.e. the fake `$HOME` directory)
might not exist. Create it automatically.

This should realistically not have an effect on non-test code.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1912
2019-10-28 20:17:07 +00:00
Philip Withnall
ef3eec8a28 gdbusauthmechanismsha1: Remove unnecessary g_warning() calls
These can be hit in the tests (if multiple tests run in parallel are
racing for `~/.dbus-keyrings/org_gtk_gdbus_general.lock` for a prolonged
period) and will cause spurious test failures due to the use of
`G_DEBUG=fatal-warnings`.

Instead, allow the error messages to be inspected programmatically.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1912
2019-10-28 20:17:07 +00:00
Simon McVittie
9f962ebeac Add a test for GDBusServer authentication
In particular, if libbdus is available, we test interoperability with
a libdbus client: see GNOME/glib#1831. Because that issue describes a
race condition, we do each test repeatedly to try to hit the failing
case.

Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-28 19:56:47 +00:00
Simon McVittie
ee502dbbe8 GDBus: prefer getsockopt()-style credentials-passing APIs
Conceptually, a D-Bus server is really trying to determine the credentials
of (the process that initiated) a connection, not the credentials that
the process had when it sent a particular message. Ideally, it does
this with a getsockopt()-style API that queries the credentials of the
connection's initiator without requiring any particular cooperation from
that process, avoiding a class of possible failures.

The leading '\0' in the D-Bus protocol is primarily a workaround
for platforms where the message-based credentials-passing API is
strictly better than the getsockopt()-style API (for example, on
FreeBSD, SCM_CREDS includes a process ID but getpeereid() does not),
or where the getsockopt()-style API does not exist at all. As a result
libdbus, the reference implementation of D-Bus, does not implement
Linux SCM_CREDENTIALS at all - it has no reason to do so, because the
SO_PEERCRED socket option is equally informative.

This change makes GDBusServer on Linux more closely match the behaviour
of libdbus.

In particular, GNOME/glib#1831 indicates that when a libdbus client
connects to a GDBus server, recvmsg() sometimes yields a SCM_CREDENTIALS
message with cmsg_data={pid=0, uid=65534, gid=65534}. I think this is
most likely a race condition in the early steps to connect:

        client           server
    connect
                         accept
    send '\0' <- race -> set SO_PASSCRED = 1
                         receive '\0'

If the server wins the race:

        client           server
    connect
                         accept
                         set SO_PASSCRED = 1
    send '\0'
                         receive '\0'

then everything is fine. However, if the client wins the race:

        client           server
    connect
                         accept
    send '\0'
                         set SO_PASSCRED = 1
                         receive '\0'

then the kernel does not record credentials for the message containing
'\0' (because SO_PASSCRED was 0 at the time). However, by the time the
server receives the message, the kernel knows that credentials are
desired. I would have expected the kernel to omit the credentials header
in this case, but it seems that instead, it synthesizes a credentials
structure with a dummy process ID 0, a dummy uid derived from
/proc/sys/kernel/overflowuid and a dummy gid derived from
/proc/sys/kernel/overflowgid.

In an unconfigured GDBusServer, hitting this race condition results in
falling back to DBUS_COOKIE_SHA1 authentication, which in practice usually
succeeds in authenticating the peer's uid. However, we encourage AF_UNIX
servers on Unix platforms to allow only EXTERNAL authentication as a
security-hardening measure, because DBUS_COOKIE_SHA1 relies on a series
of assumptions including a cryptographically strong PRNG and a shared
home directory with no write access by others, which are not necessarily
true for all operating systems and users. EXTERNAL authentication will
fail if the server cannot determine the client's credentials.

In particular, this caused a regression when CVE-2019-14822 was fixed
in ibus, which appears to be resolved by this commit. Qt clients
(which use libdbus) intermittently fail to connect to an ibus server
(which uses GDBusServer), because ibus no longer allows DBUS_COOKIE_SHA1
authentication or non-matching uids.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Closes: https://gitlab.gnome.org/GNOME/glib/issues/1831
2019-10-28 19:56:00 +00:00
Simon McVittie
1485a97d80 credentials: Invalid Linux struct ucred means "no information"
On Linux, if getsockopt SO_PEERCRED is used on a TCP socket, one
might expect it to fail with an appropriate error like ENOTSUP or
EPROTONOSUPPORT. However, it appears that in fact it succeeds, but
yields a credentials structure with pid 0, uid -1 and gid -1. These
are not real process, user and group IDs that can be allocated to a
real process (pid 0 needs to be reserved to give kill(0) its documented
special semantics, and similarly uid and gid -1 need to be reserved for
setresuid() and setresgid()) so it is not meaningful to signal them to
high-level API users.

An API user with Linux-specific knowledge can still inspect these fields
via g_credentials_get_native() if desired.

Similarly, if SO_PASSCRED is used to receive a SCM_CREDENTIALS message
on a receiving Unix socket, but the sending socket had not enabled
SO_PASSCRED at the time that the message was sent, it is possible
for it to succeed but yield a credentials structure with pid 0, uid
/proc/sys/kernel/overflowuid and gid /proc/sys/kernel/overflowgid. Even
if we were to read those pseudo-files, we cannot distinguish between
the overflow IDs and a real process that legitimately has the same IDs
(typically they are set to 'nobody' and 'nogroup', which can be used
by a real process), so we detect this situation by noticing that
pid == 0, and to save syscalls we do not read the overflow IDs from
/proc at all.

This results in a small API change: g_credentials_is_same_user() now
returns FALSE if we compare two credentials structures that are both
invalid. This seems like reasonable, conservative behaviour: if we cannot
prove that they are the same user, we should assume they are not.

Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-28 19:55:47 +00:00
Simon McVittie
ef1035d9d8 gcredentialsprivate: Document the various private macros
Signed-off-by: Simon McVittie <smcv@collabora.com>
2019-10-28 19:54:08 +00:00
Philip Withnall
2d2e96dc51 tests: Use objcopy from the cross-compilation file, if configured
Otherwise we’ll end up using the host’s `objcopy`, which will output
object files in the wrong format.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #1916
2019-10-28 12:08:48 +00:00
Emmanuele Bassi
ca1dbb38d8 tests: Do not init the default value
The call to g_param_value_set_default() will do that for us, now.
2019-10-26 14:06:31 +01:00
Bastien Nocera
0d3b1d55e9 gio: Fix typo in URL
Left-over quote in URL.
2019-10-25 15:09:08 +02:00
Sebastian Dröge
611ea6e805 Merge branch '1836-dbus-connection-docs' into 'master'
gdbusconnection: Clarify nullability in a documentation comment

Closes #1836

See merge request GNOME/glib!1003
2019-10-22 07:48:13 +00:00
Philip Withnall
b7e84fb903 testfilemonitor: Fix a trivial leak in the test
Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1910
2019-10-18 17:02:57 +01:00
Will Thompson
43697d6b99 Revert "gdbus-codegen: emit GUnixFDLists if an arg has type 'h'"
This reverts commit 4aba03562b, preserving
the new tests but adjusting them to assert that the old behaviour is
restored.

As expected, there were a few projects which broke because of this.
Unfortunately, in one case the breakage crosses a project boundary:
sysprof ships D-Bus introspection XML, which is consumed by mutter and
passed through gdbus-codegen.

Since sysprof cannot add this annotation without breaking its existing
users, a warning is also not appropriate.

https://gitlab.gnome.org/GNOME/jhbuild/issues/41
https://gitlab.gnome.org/GNOME/sysprof/issues/17
https://gitlab.gnome.org/GNOME/glib/issues/1726
2019-10-18 10:30:00 +01:00
Philip Withnall
592a13b483 glocalfilemonitor: Keep a weak ref to the monitor in GFileMonitorSource
Previously we were keeping a pointer to the `GFileMonitor` in a
`GFileMonitorSource` instance, but since we weren’t keeping a strong
reference, that `GFileMonitor` instance could be finalised from another
thread at any point while the source was referring to it. Not good.

Use a weak reference, and upgrade it to a strong reference whenever the
`GFileMonitorSource` is referring to the file monitor.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1903
2019-10-11 22:31:24 +01:00
Philip Withnall
5b07fc98e0 gdesktopappinfo: Cancel file monitor when resetting a DesktopFileDir
It’s not enough to unref the monitor, since the GLib worker thread might
still hold a reference to it.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Helps: #1903
2019-10-11 22:31:24 +01:00
Philip Withnall
bffe058550 gdesktopappinfo: Allocate DesktopFileDir structs dynamically
`DesktopFileDir` pointers are passed around between threads: they are
initially created on the main thread, but a pointer to them is passed to
the GLib worker thread in the file monitor callback
(`desktop_file_dir_changed()`).

Accordingly, the `DesktopFileDir` objects either have to be
 (1) immutable;
 (2) reference counted; or
 (3) synchronised between the two threads
to avoid one of them being used by one thread after being freed on
another. Option (1) changed with commit 99bc33b6 and is no longer an
option. Option (3) would mean blocking the main thread on the worker
thread, which would be hard to achieve and is against the point of
having a worker thread. So that leaves option (2), which is implemented
here.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #1903
2019-10-11 22:31:24 +01:00
Milan Crha
822f8bae9e Fix use-after-free when calling g_dbus_connection_flush_sync()
When the _g_dbus_worker_flush_sync() schedules the 'data' and releases
the worker->write_lock, it is possible for the GDBus worker thread thread
to finish the D-Bus call and acquire the worker->write_lock before
the _g_dbus_worker_flush_sync() re-acquires it in the if (data != NULL) body.
When that happens, the ostream_flush_cb() increases the worker->write_num_messages_flushed
and then releases the worker->write_lock. The write lock is reacquired by
the _g_dbus_worker_flush_sync(), which sees that the while condition is satisfied,
thus it doesn't enter the loop body and immediately clears the data members and
frees the data structure itself. The ostream_flush_cb() is still ongoing, possibly
inside flush_data_list_complete(), where it accesses the FlushData, which can be
in any stage of being freed.

Instead, add an explicit boolean flag indicating when the flush is truly finished.

Closes #1896
2019-10-10 14:55:20 +00:00
Patrick Griffis
ea99872e45 Always resolve localhost to loopback address
This always resolves "localhost" to a loopback address which
has security benefits such as preventing a malicious dns server
redirecting local connections and allows software to assume
it is a secure hostname.

This is being adopted by web browsers:

- https://w3c.github.io/webappsec-secure-contexts/
- https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/RC9dSw-O3fE/E3_0XaT0BAAJ
- 8da2a80724
- https://bugs.webkit.org/show_bug.cgi?id=171934
- https://tools.ietf.org/html/draft-west-let-localhost-be-localhost-06
2019-10-10 14:32:18 +01:00
Patrick Griffis
01acb8907f tests: Don't block mainloop for delays in gnetworkaddress tests 2019-10-10 14:32:18 +01:00
Philip Withnall
8492df9f34 gdbusaddress: Validate the noncefile attribute of nonce-tcp addresses
Doing this mostly to fix a compiler warning about tautological
assignments on Android.

See the D-Bus specification:
https://dbus.freedesktop.org/doc/dbus-specification.html#transports-nonce-tcp-sockets

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-08 13:53:01 +01:00
Philip Withnall
e99003841a gdbusaddress: Collapse two translatable strings into one
Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-08 13:51:57 +01:00
Philip Withnall
e257e17d2e inotify: Fix some enum comparisons to integers
`-1` isn’t a valid member of the enum, so cast to `int` first. This
fixes a compiler warning on Android.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
2019-10-08 13:49:40 +01:00