Hi Tim,
[Dislaimer: not the maintainer here, just looked at it briefly]
On Mon, Jan 06, 2020 at 02:44:48PM +0100, Tim Duesterhus wrote:
> Package: tinyproxy
> Version: 1.10.0-2
> Severity: critical
> Justification: breaks unrelated software
>
> Dear Maintainer,
>
> * What led up to the situation?
>
> I configured tinyproxy without a PidFile.
>
> * What exactly did you do (or not do) that was effective (or
> ineffective)?
>
> I removed the PidFile configuration option from tinyproxy.conf
>
> * What was the outcome of this action?
>
> The next run of logrotate changed the owner and group of my root
> directory (`/`) to tinyproxy:tinyproxy.
>
> * What outcome did you expect instead?
>
> I expected that not to happen.
>
> Example demonstrating the issue in a fresh VM:
>
> root@debian-2gb-fsn1-1:~# stat /
> File: /
> Size: 4096 Blocks: 8 IO Block: 4096 directory
> Device: 801h/2049d Inode: 2 Links: 18
> Access: (0755/drwxr-xr-x) Uid: ( 0/ root) Gid: ( 0/ root)
> Access: 2019-12-08 05:11:02.514309382 +0100
> Modify: 2020-01-06 01:51:41.524000000 +0100
> Change: 2020-01-06 01:51:41.524000000 +0100
> Birth: -
> root@debian-2gb-fsn1-1:~# apt-get install -yyyyqqqq tinyproxy
> Selecting previously unselected package tinyproxy-bin.
> (Reading database ... 35006 files and directories currently installed.)
> Preparing to unpack .../tinyproxy-bin_1.10.0-2_amd64.deb ...
> Unpacking tinyproxy-bin (1.10.0-2) ...
> Selecting previously unselected package tinyproxy.
> Preparing to unpack .../tinyproxy_1.10.0-2_all.deb ...
> Unpacking tinyproxy (1.10.0-2) ...
> Setting up tinyproxy-bin (1.10.0-2) ...
> Setting up tinyproxy (1.10.0-2) ...
> Created symlink /etc/systemd/system/multi-user.target.wants/tinyproxy.service
> → /lib/systemd/system/tinyproxy.service.
> Processing triggers for man-db (2.8.5-2) ...
> Processing triggers for systemd (241-7~deb10u2) ...
> root@debian-2gb-fsn1-1:~# grep PidFile /etc/tinyproxy/tinyproxy.conf
> # PidFile: Write the PID of the main tinyproxy thread to this file so it
> PidFile "/run/tinyproxy/tinyproxy.pid"
> root@debian-2gb-fsn1-1:~# sed -i '/PidFile/d' /etc/tinyproxy/tinyproxy.conf
> root@debian-2gb-fsn1-1:~# grep PidFile /etc/tinyproxy/tinyproxy.conf
> root@debian-2gb-fsn1-1:~# systemctl start logrotate
> root@debian-2gb-fsn1-1:~# sed -i 's/2020/2019/g' /var/lib/logrotate/status
> root@debian-2gb-fsn1-1:~# systemctl start logrotate
> root@debian-2gb-fsn1-1:~# stat /
> File: /
> Size: 4096 Blocks: 8 IO Block: 4096 directory
> Device: 801h/2049d Inode: 2 Links: 18
> Access: (0755/drwxr-xr-x) Uid: ( 106/tinyproxy) Gid: ( 112/tinyproxy)
> Access: 2019-12-08 05:11:02.514309382 +0100
> Modify: 2020-01-06 01:51:41.524000000 +0100
> Change: 2020-01-06 01:53:05.254019354 +0100
> Birth: -
>
> Note that tinyproxy does not start up with this configuration, because systemd
> expects the PidFile to appear. For the machine where I noticed this issue I
> also
> adjusted the systemd unit to be of `Type=simple`.
>
> While this configuration might not be common and not encountered by the
> average
> user it introduced a possible security hole in my system and even if this
> might
> not be fully exploitable by the `tinyproxy` user it breaks systemd-tmpfiles:
>
> Jan 06 01:57:53 debian-2gb-fsn1-1 systemd-tmpfiles[282]: Detected unsafe path
> transition / → /var during canonicalization of /var.
I think the whole problem is placed at multiple levels. You seem to
use systemd as init, but when lgorotate comes into action, the
postrotate is
invoke-rc.d --quiet tinyproxy reload > /dev/null
As tinyproxy.service can't reload the service through the .service
file, it will fallback into reloading tinyproxy via the init-script:
[...]
# assert pidfile directory and permissions
if [ "$1" != "stop" ]; then
if [ -f "$CONFIG" ]; then
USER=$(grep -i '^User[[:space:]]' "$CONFIG" | awk '{print $2}')
GROUP=$(grep -i '^Group[[:space:]]' "$CONFIG" | awk '{print $2}')
PIDFILE=$(grep -i '^PidFile[[:space:]]' "$CONFIG" | awk '{print $2}' |\
sed -e 's/"//g')
PIDDIR=`dirname "$PIDFILE"`
if [ -n "$PIDDIR" -a "$PIDDIR" != "/run" ]; then
if [ ! -d "$PIDDIR" ]; then
mkdir "$PIDDIR"
fi
if [ "$USER" ]; then
chown "$USER" "$PIDDIR"
fi
if [ "$GROUP" ]; then
chgrp "$GROUP" "$PIDDIR"
fi
fi
fi
fi
[...]
PIDDIR will be ".", and the chown and chgrp will be applied to the
current working directory.
The current working directory is changed to /, and thus the chown and
chgrp call upon '.' will affect /.
>
> Thus I feel the severity of `critical` is justified for this bug report.
I'm not sure this is warranted, but I will leave this to the decision
of the maintainer :)
Regards,
Salvatore
