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 
- 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 

- 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 
    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 
        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_);

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.

  [MIR] rygel

To manage notifications about this bug go to:

ubuntu-bugs mailing list

Reply via email to