Re: [PATCH v2] settings: Resolve path if hostname is a sym-link

[Joel Holdsworth <joel holdsworth vcatechnology com>]

> Hi Joel,

Hi


> > diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
> > index f6f8c37..fc2eb29 100644
> > --- a/src/settings/nm-settings.c
> > +++ b/src/settings/nm-settings.c
> > @@ -1532,11 +1532,11 @@ write_hostname (NMSettingsPrivate *priv,
> > const char *hostname)
> >         char *hostname_eol;
> >         gboolean ret;
> >         gs_free_error GError *error = NULL;
> > -       const char *file = priv->hostname.file;
> > +       char *file = g_strdup(priv->hostname.file), *link_path;
>
> Whitespace between "g_strdup" and "(".

Yup, ok I can fix that.


> This now leaks @file.
>
> Better do:
>
>          const char *file = priv->hostname.file;
>          gs_free char *link_path = NULL;
>
>
> and later:
>
>         if (   lstat (file, &file_stat) == 0
>              && S_ISLNK  (file_stat.st_mode)
>              && (link_path = g_file_read_link (file, NULL)))
>              file = link_path;
>
> which also avoid the additional copy.

Good point. I will resubmit with these things fixed.


> Anyway, why do we event want this? It's not clear to me that this is
> desired behavior.
>
> If NM is configured to overwrite /etc/hostname, with /etc/hostname
> symlinking to somewhere else (e.g. /var/run/some-daemon/hostname), then
> I don't think that NM should overwrite the file owned by "some-daemon"
> but always rewrite /etc/hostname.
>
> Note that for /etc/resolv.conf, NM only writes to
> /var/run/NetworkManager/resolv.conf without redirecting the symlink in
> /etc.
> Maybe we should do something similar with /etc/hostname, but then it
> seems to me that hostnamed is the future to manage the hostname.

On my (embedded) system /etc is stored in squashfs and therefore isn't 
writable. This is necessary because the majority of the config files in 
that location are set to a fixed configuration, and the few dynamic 
files are sym-links to /var/lib (non-volatile) or /run (volatile).

Every other package I've integrated is able to read/write through 
sym-links. The problem here is that g_file_set_contents will attempt to 
create a temp-file in /etc then rename that file to /etc/hostname for an 
atomic write. So it's not just a problem that NM will attempt to destroy 
a sym-link, it also tries to write into a read-only directory.

In it's current state NetworkManager pretty much requires /etc to be 
writable, which isn't really feasible/desirable in our case.

The first version of my patch would attempt to recursively follow 
sym-links. In the second version I discarded this approach because it 
could be used to create an infinite sym-link loop, or it could be used 
to write arbitrary data to arbitrary files if an attacker could control 
the path within a second-level link (e.g. /etc/hostname -> 
/path/to/compromised/sym-link -> /path/to/something/critical ).

In this second version of the patch, NM will only follow one level of 
link, and on non-broken systems /etc/hostname is only replaceable by 
privileged users.

If you find this unacceptable, then what do you suggest as an 
alternative? It seems rather overkill to add a runtime config for this. 
I guess I could add another option to the selection in 
--with-hostname-persistt=default|suse|gentoo . Though is that any better 
than simply following the symbolic link?

Thank you for your feedback.

Joel