The `G_FILE_CREATE_REPLACE_DESTINATION` flag is equivalent to unlinking
the destination file and re-creating it from scratch. That did
previously work, but in the process the code would call `open(O_CREAT)`
on the file. If the file was a dangling symlink, this would create the
destination file (empty). That’s not an intended side-effect, and has
security implications if the symlink is controlled by a lower-privileged
process.
Fix that by not opening the destination file if it’s a symlink, and
adjusting the rest of the code to cope with
- the fact that `fd == -1` is not an error iff `is_symlink` is true,
- and that `original_stat` will contain the `lstat()` results for the
symlink now, rather than the `stat()` results for its target (again,
iff `is_symlink` is true).
This means that the target of the dangling symlink is no longer created,
which was the bug. The symlink itself continues to be replaced (as
before) with the new file — this is the intended behaviour of
`g_file_replace()`.
The behaviour for non-symlink cases, or cases where the symlink was not
dangling, should be unchanged.
Includes a unit test.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2325
Since a following commit is going to add a new test which references
Gitlab, so it’s best to move the URI bases inside the test cases.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
The GDBusConnectionFlags and GDBusServerFlags can affect how we carry
out authentication and authorization, either making it more or less
restrictive, so it's desirable to "fail closed" if a program is compiled
against a new version of GLib but run against an old version.
Signed-off-by: Simon McVittie <smcv@collabora.com>
GByteArray uses guint for storing the length of the byte array, but it
also has a constructor (g_byte_array_new_take) that takes length as a
gsize. gsize may be larger than guint (64 bits for gsize vs 32 bits
for guint). It is possible to call the function with a value greater
than G_MAXUINT, which will result in silent length truncation. This
may happen as a result of unreffing GBytes into GByteArray, so rather
be loud about it.
(Test case tweaked by Philip Withnall.)
(Backport 2.66: Add #include gstrfuncsprivate.h in the test case for
`g_memdup2()`.)
These should never have been allowed; they will result in precondition
failures from the `GKeyFile` later on in the code.
A test will be added for this shortly.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fix an effective regression in commit
7781a9cbd2, which happens when
`convert_path()` is called with a `key` which contains no slashes. In
that case, the `key` is entirely the `basename`.
Prior to commit 7781a9cb, the code worked through a fluke of `i == -1`
cancelling out with the various additions in the `g_memdup()` call, and
effectively resulting in `g_strdup (key)`.
Spotted by Guido Berhoerster.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
The intention here was to assert that the length of the password fits
in a gssize. Passwords more than half the size of virtual memory are
probably excessive.
Fixes: a8b204ff "gtlspassword: Forbid very long TLS passwords"
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 61bb52ec42)
The public API `GIOChannel.line_term_len` is only a `guint`. Ensure that
nul-terminated strings passed to `g_io_channel_set_line_term()` can’t
exceed that length. Use `g_memdup2()` to avoid a warning (`g_memdup()`
is due to be deprecated), but not to avoid a bug, since it’s also
limited to `G_MAXUINT`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
The public API `g_tls_password_set_value_full()` (and the vfunc it
invokes) can only accept a `gssize` length. Ensure that nul-terminated
strings passed to `g_tls_password_set_value()` can’t exceed that length.
Use `g_memdup2()` to avoid an overflow if they’re longer than
`G_MAXUINT` similarly.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Don’t use an `int`, that’s potentially too small. In practical terms,
this is not a problem, since no socket address is going to be that big.
By making these changes we can use `g_memdup2()` without warnings,
though. Fewer warnings is good.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Previously, the code in `convert_path()` could not handle keys longer
than `G_MAXINT`, and would overflow if that was exceeded.
Convert the code to use `gsize` and `g_memdup2()` throughout, and
change from identifying the position of the final slash in the string
using a signed offset `i`, to using a pointer to the character (and
`strrchr()`). This allows the slash to be at any position in a
`G_MAXSIZE`-long string, without sacrificing a bit of the offset for
indicating whether a slash was found.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
This allows it to handle strings up to length `G_MAXSIZE` — previously
it would overflow with such strings.
Update the several copies of it identically.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Previously it was handled as a `gssize`, which meant that if the
`stop_chars` string was longer than `G_MAXSSIZE` there would be an
overflow.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
The members of `URL_COMPONENTS` (`winhttp_file->url`) are `DWORD`s, i.e.
32-bit unsigned integers. Adding to and multiplying them may cause them
to overflow the unsigned integer bounds, even if the result is passed to
`g_memdup2()` which accepts a `gsize`.
Cast the `URL_COMPONENTS` members to `gsize` first to ensure that the
arithmetic is done in terms of `gsize`s rather than unsigned integers.
Spotted by Sebastian Dröge.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Convert all the call sites which use `g_memdup()`’s length argument
trivially (for example, by passing a `sizeof()` or an existing `gsize`
variable), so that they use `g_memdup2()` instead.
In almost all of these cases the use of `g_memdup()` would not have
caused problems, but it will soon be deprecated, so best port away from
it
In particular, this fixes an overflow within `g_bytes_new()`, identified
as GHSL-2021-045 by GHSL team member Kevin Backhouse.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: GHSL-2021-045
Helps: #2319
Convert all the call sites which use `g_memdup()`’s length argument
trivially (for example, by passing a `sizeof()`), so that they use
`g_memdup2()` instead.
In almost all of these cases the use of `g_memdup()` would not have
caused problems, but it will soon be deprecated, so best port away from
it.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Convert all the call sites which use `g_memdup()`’s length argument
trivially (for example, by passing a `sizeof()`), so that they use
`g_memdup2()` instead.
In almost all of these cases the use of `g_memdup()` would not have
caused problems, but it will soon be deprecated, so best port away from
it.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
This will replace the existing `g_memdup()` function for use within
GLib. It has an unavoidable security flaw of taking its `byte_size`
argument as a `guint` rather than as a `gsize`. Most callers will
expect it to be a `gsize`, and may pass in large values which could
silently be truncated, resulting in an undersize allocation compared
to what the caller expects.
This could lead to a classic buffer overflow vulnerability for many
callers of `g_memdup()`.
`g_memdup2()`, in comparison, takes its `byte_size` as a `gsize`.
Spotted by Kevin Backhouse of GHSL.
In GLib 2.68, `g_memdup2()` will be a new public API. In this version
for backport to older stable releases, it’s a new `static inline` API
in a private header, so that use of `g_memdup()` within GLib can be
fixed without adding a new API in a stable release series.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: GHSL-2021-045
Helps: #2319
We're using "setuid" here as shorthand for any elevated privileges
that should make us distrust the caller: setuid, setgid, filesystem
capabilities, more obscure Linux things that set the AT_SECURE flag
(such as certain AppArmor transitions), and their equivalents on
other operating systems. This is fine if we do it consistently, but
I'm about to add a check for whether we are *literally* setuid,
which would be particularly confusing without a rename.
Signed-off-by: Simon McVittie <smcv@collabora.com>
We preallocate buffers that are used after forked. That is because
malloc()/free() are not async-signal-safe and must not be used between
fork() and exec().
However, for the child process that exits without fork, valgrind wrongly
reports these buffers as leaked.
That can be suppressed with "--child-silent-after-fork=yes", but it is
cumbersome.
Work around by trying to allocate the buffers on the stack. At
least in the common cases where the pointers are small enough
so that we can reasonably do that.
If the buffers happen to be large, we still allocate them on the heap
and the problem still happens. Maybe we could have also allocated them
as thread_local, but currently glib doesn't use that.
[smcv: Cosmetic adjustments to address review comments from pwithnall]
For manual test coverage that would reproduce the bug fixed in !1902,
copy /bin/true (or any other harmless executable) to
/usr/bin/spawn-test-helper.
Signed-off-by: Simon McVittie <smcv@collabora.com>
do_exec() and g_execute() rely on being passed a NULL search path
if we intend to avoid searching the PATH, but since the refactoring
in commit 62ce66d4, this was never done. This resulted in some spawn
calls searching the PATH when it was not intended.
Spawn calls that go through the posix_spawn fast-path were unaffected.
The deprecated gtester utility, as used in GTK 3, relies on the
ability to run an executable from the current working directory by
omitting the G_SPAWN_SEARCH_PATH flag. This *mostly* worked, because
our fallback PATH ends with ".". However, if an executable of the
same name existed in /usr/bin or /bin, it would run that instead of the
intended test: in particular, GTK 3's build-time tests failed if
ImageMagick happens to be installed, because gtester would accidentally
run display(1) instead of testsuite/gdk/display.
Fixes: 62ce66d4 "gspawn: Don’t use getenv() in async-signal-safe context"
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977961
