I reviewed rygel 0.36.2-5ubuntu1 as checked in to eoan. This isn't a
full security audit, but rather a quick gauge of maintainability.
- rygel is a UPnP AV media server, allowing audio and video to be shared with
other devices. It can also operate as a media renderer which can be controlled
by UPnP controllers.
- rygel is part of the GNOME project.
- rygel is written in vala.
- The website claims it us under active development, but activity seems to have
slowed somewhat this year - since February, the only git commits are mostly
translation updates and what look like minor build / bug fixes.
- Of the 118 issues currently open in gitlab, many of them appear to have been
originally imported from Bugzilla and haven't had much activity since the
migration.
- No CVEs in our database.
- Build dependencies in main, except for:
- libgssdp-1.2-dev (bug 1799977)
- libgupnp-1.2-dev, libgupnp-av-1.0-dev, libgupnp-dlna-2.0-dev (bug 1799974)
- valac (this doesn't create any binary package dependencies)
- The rygel binary package recommends gstreamer1.0-plugins-ugly, which is in
universe.
- I ran coverity, although this mostly highlights problems in C code that is
auto-generated by valac.
- Coverity shows up quite a few unreachable code issues, like this one in
rygel_base_configuration_real_get_interface:
...
g_propagate_error (error, _inner_error0_);
return NULL;
return result;
}
And also rygel_basic_management_test_real_run_co:
...
}
g_object_unref (_data_->_async_result);
return FALSE;
g_task_return_pointer (_data_->_async_result, _data_, NULL);
if (_data_->_state_ != 0) {
while (!_data_->_task_complete_) {
...
These look like vala bugs.
- There's a fd leak in rygel_energy_management_get_mac_and_network_type if fd
== 0. I'm not sure if this is a real issue - it's only possible to hit this
condition if stdin is closed.
- Coverity catches what looks like an obvious null deref here in
rygel_ruih_service_manager_real_constructed in the second call to
block1_data_unref:
...
if (G_UNLIKELY (_inner_error0_ != NULL)) {
_g_object_unref0 (config_dir_file);
block1_data_unref (_data1_);
_data1_ = NULL;
if (_inner_error0_->domain == RYGEL_RUIH_SERVICE_ERROR) {
goto __catch2_rygel_ruih_service_error;
}
if (_inner_error0_->domain == G_IO_ERROR) {
goto __catch2_g_io_error;
}
_g_object_unref0 (config_dir_file);
block1_data_unref (_data1_);
_data1_ = NULL;
_g_free0 (ui_listing_directory);
g_critical ("file %s: line %d: unexpected error: %s (%s, %d)",
__FILE__, __LINE__, _inner_error0_->message, g_quark_to_string
(_inner_error0_->domain), _inner_error0_->code);
g_clear_error (&_inner_error0_);
return;
}
...
I'm not sure if this is actually a vala bug though.
- Rygel.SimpleDataSource.run uses lseek without checking the return value.
- The uint.clamp() call in Rygel.External.Container constuctor generates C code
that does a negative unsigned compare. Would "uint.min(child_count, int.MAX)"
be more appropriate than "child_count.clamp(0, int.MAX)" ?
- Rygel.MediaExport.DVDContainer.constructed parses XML with options that are
vulnerable to External Entity injection, specifically Xml.ParserOptions.NOENT.
See https://cwe.mitre.org/data/definitions/611.html for more context.
- Use of Xml.ParserOptions.RECOVER also triggers another defect in coverity.
These issues may be repeated in other locations.
- Rygel.Tracker.Titles.create_title_for_value performs a negative unsigned
compare when checking the result of get_char_validated, and produces C code
that always evaluates false and could be optimised away. "if (unlikely (c ==
-2)) {" (which I think would expand to the C code "if (G_UNLIKELY (c ==
((gunichar) -2))) {") might be more correct.
- Most of the code is written in vala, which compiles to intermediate C code
and handles memory management (eg freeing and checking whether allocations are
successful) transparently, as long as the memory management GIR annotations are
correct. Memory management is performed via glib + gobject APIs (g_new0,
g_object_new, g_string_new etc).
- Works with files, using glib's GFile API's (at least, in the gst media engine
and media-export plugin).
- Limited logging using g_debug (not logged by default) and g_message, which go
to the user's journal. There doesn't appear to be anything sensitive logged.
- Allows environment variables to override configuration file options.
- No privileged code.
- Doesn't use temporary files.
- No privileged commands.
- No webkit.
- Processes local media files using gstreamer.
- Build logs look ok - just one compiler warning in the build:
CDPATH="${ZSH_VERSION+.}:" && cd . && /usr/bin/valac -H rygel-renderer-gst.h
--library=rygel-renderer-gst-2.6 --gir=Rygel-2.6.gir --enable-experimental
--pkg gupnp-1.2 --pkg gee-0.8 --pkg gupnp-av-1.0 --pkg gstreamer-1.0 --pkg
gstreamer-audio-1.0 --vapidir=/<<PKGBUILDDIR>>/src/librygel-renderer --pkg
rygel-renderer-2.6 --vapidir=/<<PKGBUILDDIR>>/src/librygel-core --pkg
rygel-core-2.6 --vapidir=/<<PKGBUILDDIR>>/src/librygel-core --pkg
rygel-build-config --enable-deprecated --target-glib=2.40 -C
rygel-playbin-player.vala rygel-playbin-renderer.vala
rygel-playbin-player.vala:33.1-33.44: warning: Namespace Playbin does not have
a GIR namespace and version annotation
public errordomain Rygel.Playbin.PlayerError {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Compilation succeeded - 1 warning(s)
- Lintian clean, except for one warning (missing manpage for rygel-preferences).
- Ships a configuration files in /etc/rygel.conf, which can be overridden by a
per-user config. A summary of some of the configuration options:
- The network interface isn't hardcoded (which I think means that rygel
publishes on all interfaces).
- The port that the HTTP server(s) run on is allocated dynamically.
- The log level is set to a sane value.
- Uploading and deletion of media files is disabled.
- Both the media-export and tracker media-server plugins are enabled in
/etc/rygel.conf, although only one can be used at a time. When you use the
sharing panel in gnome-control-center to enable media sharing, it copies
/etc/rygel.conf to ~/.config/rygel.conf and disables the tracker plugin.
- The media-export plugin is configured to export from ~/Music, ~/Pictures
and ~/Videos by default.
- The playbin media renderer is enabled.
- No systemd system services
- There is a systemd user service, although it's not started by default as part
of the user session (gnome-settings-daemon asks systemd to start and stop it as
required, depending on the configuration).
- There is a dbus service.
- No setuid binaries.
- No fs capabilities.
- No privileged commands.
- No sudo fragments.
- No udev rules.
- No cron jobs.
- There are some tests that run in the build (and currently pass).
- The media-export plugin uses sqlite for caching. It makes use of prepared
statements, although in some cases (eg,
Rygel.MediaExport.MediaCache.get_children), the prepared statement comes from a
printf style formatter. The parameters to this are derived from a string which
is provided by client devices (sort criteria - a comma separated list of
columns to sort on) and looks to be filtered and validated sufficiently to be
safe. The individual values are filtered in
Rygel.MediaQueryAction.validate_sort_criteria and then each value is mapped to
a column name in Rygel.MediaExport.MediaCache.map_operand_to_column, with that
function returning an error if any value doesn't map to a column.
- It exports a HTTP server, the availability of which is advertised via SSDP.
Most of the HTTP networking code is implemented in gupnp, for which there is a
separate MIR. Rygel implements 2 UPnP devices (media server and media renderer)
which are described by XML served over HTTP, and which contain a set of service
descriptions. Rygel uses the gupnp API to implement handlers for actions
described by those services.
- It does create an additional HTTP endpoint via the gupnp API for streaming
media files and their metadata. Resources are identified by what looks like an
encoded MD5 hash of the file paths, and those map to MediaObject instances via
a database lookup.
- Spawns subproceses for the purpose of extracting metadata from media files
(from the media-export plugin). Rygel is using glib's GSubprocessLauncher for
this, which does extra things like set the O_CLOEXEC flag on fds that aren't
explicitly requested to be passed to the child process.
- There doesn't appear to be any effort to sandbox or limit the privileges of
the metadata extractor process in any way.
Security team ACK for promoting rygel to main, as long as gstreamer1.0
-plugins-ugly is dropped from recommends and somebody looks at the issue
with the XML parser options to see if there's a real bug there.
** Changed in: rygel (Ubuntu)
Assignee: Ubuntu Security Team (ubuntu-security) => (unassigned)
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1786489
Title:
[MIR] rygel
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/rygel/+bug/1786489/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
