Bug#802211: RFC: wip patch to force sulogin on locked root accounts

[Felipe Sateler]

On Thu, Oct 12, 2017 at 7:07 PM, Stijn van Drongelen  wrote:
> Hi,
>
>> TL;DR...
>
> Yes, I knew about that risk. But shorter messages have failed to draw
> attention to a supposedly "important" bug.
>
> I expected that the original patch was good to go, but I somehow forgot
> that a 720+ day old patch is subject to bitrot. Please correct me if
> I'm wrong, but wouldn't we now need (at least) three patches now to fix
> this problem?
>
> * We have to patch src/sulogin-shell/rescue.service.in and
>   src/sulogin-shell/emergency.service.in in version 232 (the version in
>   stretch).
> * We have to patch src/sulogin-shell/systemd-sulogin-shell.in in
>   version 234 (the version currently in stretch-backports and buster).
> * We have to patch src/sulogin-shell/sulogin-shell.c in version 235
>   (the version currently in sid).
>
> I can probably find some more time in the coming week to contribute
> to a solution

Excellent. I would suggest though to fix this first for 235, and
upstream. That way whatever solution is implemented for stretch[1] can
be designed with compatibility with buster in mind.


[1] If the release team ACKs the change, too.

>. I do have a few comments in advance:
>
> 1) I want to stress that I believe that the only correct solution is
>an unconditional --force. Patches that do just that do not reduce
>the boot security of any plausible installation in any way, and are
>also much easier to implement correctly. The points below can be
>mostly ignored if we do this.
> 2) The "You are in rescue/emergency mode" message has to change
>depending on the setting: the mentioned login prompt won't be there
>when using --force.

It is my understanding that sulogin --force will still ask for
password if getpwnam works.

> 3) I think it's much simpler to build two separate binaries (one that
>forces, one that doesn't) instead of dynamically adjusting
>a command line and a message based on the value of an environment
>variable.

This has the drawback of requiring to modify ExecStart, and thus risk
becoming incompatible if the sulogin wrapper changes interface.

> 4) If you think it's likely (not merely possible) that supplying other
>options to sulogin will become necessary, then I think it makes more
>sense to pass sulogin-shell's arguments to sulogin and use getenv()
>to decide the label of the mode (rescue/emergency/potato), rather
>than the other way around.
>

I don't have an opinion here.

-- 

Saludos,
Felipe Sateler

___
Pkg-systemd-maintainers mailing list
Pkg-systemd-maintainers@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-systemd-maintainers

Bug#802211: RFC: wip patch to force sulogin on locked root accounts

[Stijn van Drongelen]

Hi,

> TL;DR...

Yes, I knew about that risk. But shorter messages have failed to draw
attention to a supposedly "important" bug.

I expected that the original patch was good to go, but I somehow forgot
that a 720+ day old patch is subject to bitrot. Please correct me if
I'm wrong, but wouldn't we now need (at least) three patches now to fix
this problem?

* We have to patch src/sulogin-shell/rescue.service.in and
  src/sulogin-shell/emergency.service.in in version 232 (the version in
  stretch).
* We have to patch src/sulogin-shell/systemd-sulogin-shell.in in
  version 234 (the version currently in stretch-backports and buster).
* We have to patch src/sulogin-shell/sulogin-shell.c in version 235
  (the version currently in sid).

I can probably find some more time in the coming week to contribute
to a solution. I do have a few comments in advance:

1) I want to stress that I believe that the only correct solution is
   an unconditional --force. Patches that do just that do not reduce
   the boot security of any plausible installation in any way, and are
   also much easier to implement correctly. The points below can be
   mostly ignored if we do this.
2) The "You are in rescue/emergency mode" message has to change
   depending on the setting: the mentioned login prompt won't be there
   when using --force.
3) I think it's much simpler to build two separate binaries (one that
   forces, one that doesn't) instead of dynamically adjusting
   a command line and a message based on the value of an environment
   variable.
4) If you think it's likely (not merely possible) that supplying other
   options to sulogin will become necessary, then I think it makes more
   sense to pass sulogin-shell's arguments to sulogin and use getenv()
   to decide the label of the mode (rescue/emergency/potato), rather
   than the other way around.

Regards,
Stijn van Drongelen

___
Pkg-systemd-maintainers mailing list
Pkg-systemd-maintainers@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-systemd-maintainers

Bug#802211: RFC: wip patch to force sulogin on locked root accounts

[Felipe Sateler]

Hi all,

On Thu, Oct 12, 2017 at 9:21 AM, Andreas Henriksson  wrote:
> Hello,
>
> I'm attaching a completely untested patch against systemd packaging git.
>
> @Stijn van Drongelen :
> Maybe you can offer to test it (and maybe even finish it up)?
>
> (Please note how I *intentionally* isn't setting a patch tag since
> the patch is both untested and *unfinished*. This is mostly a RFC
> if this method would be considered acceptable.)

Indeed, it is lacking some error checking.

I think the overall idea is sane, however I think the
systemd-sulogin-shell patch should go upstream. I rewrote the previous
small shell wrapper in C precisely so that these sort of patches have
a better chance at acceptance upstream. I have just a few comments on
the patch:

1. Error checking on the strv_* operations is missing.
2. I would have a single `fork_wait` call point, and have
`sulogin_cmdline_args = sulogin_cmdline` when the envvar is not
present or empty.
3. I'm not sure if it is best to have a single SULOGIN_ARGS envvar or
multiple SULOGIN_FORCE, SULOGIN_OTHER_ARG flags. The strv_split
operation is naive in that then arguments with spaces can't be passed.
OTOH, sulogin does not accept any argument where spaces make sense, so
it doesn't have a practical impact here.

Only comment 1 really needs to be addressed before presenting
upstream, as 2 and 3 are more stylistic and upstream might have
different preferences than me.

>
> Rather than shipping the dropins in /lib/systemd/system they
> maybe should be installed in /etc/systemd/system instead (as
> conffiles) to easier allow the sysadmin to remove them.
> (Or even ship commented-out under secure-by-default mantra.)
>
> Personally I don't really see much point in this. Why would you
> expect passwordless root shells to be handed out if you lock
> the root account?

I do. For many (most?) computers, physical access means game lost
security-wise, as you can just disassemble the box and get the hard
drive. Making the rescue and emergency shells unusable in the (now
default?) passwordless-root environments d-i generates is not very
user friendly. So I think d-i should generate this snippet in /etc if
the root account was not configured.

-- 

Saludos,
Felipe Sateler

___
Pkg-systemd-maintainers mailing list
Pkg-systemd-maintainers@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-systemd-maintainers

Bug#802211: RFC: wip patch to force sulogin on locked root accounts

[Andreas Henriksson]

Hello,

I'm attaching a completely untested patch against systemd packaging git.

@Stijn van Drongelen :
Maybe you can offer to test it (and maybe even finish it up)?

(Please note how I *intentionally* isn't setting a patch tag since
the patch is both untested and *unfinished*. This is mostly a RFC
if this method would be considered acceptable.)

Rather than shipping the dropins in /lib/systemd/system they
maybe should be installed in /etc/systemd/system instead (as
conffiles) to easier allow the sysadmin to remove them.
(Or even ship commented-out under secure-by-default mantra.)

Personally I don't really see much point in this. Why would you
expect passwordless root shells to be handed out if you lock
the root account?

If you only consider default debian installations getting a
root shell is as easy as adding init=/bin/sh in grub to kernel
command line.

(If you also consider secure boot environments, you likely don't
want to hand out passwordless root shells by default...)

The only thing I can really sympathise with is Ubuntu the root
account is always locked and the user never gets to choose, but
they have already implemented their own solution suitable for
their usecase (but IMHO not suitable for Debian).

Regards,
Andreas Henriksson
>From 00085bca85faaf62c352ad1e1d5301a705fe9bc3 Mon Sep 17 00:00:00 2001
From: Andreas Henriksson 
Date: Thu, 12 Oct 2017 14:05:42 +0200
Subject: [PATCH] WIP: untested changes for #802211

Maybe would be better if the dropins where installed in
/etc/systemd/system (i.e. as a conffile) to allow admin to more easily
remove it.. or maybe even the dropins should be commented out
by default as shipped (under the secure-by-default mantra)?
---
 .../extra/units/emergency.service.d/sulogin.conf   |  3 ++
 debian/extra/units/rescue.service.d/sulogin.conf   |  1 +
 debian/patches/debian/sulogin-allow-args.patch | 41 ++
 debian/patches/series  |  1 +
 4 files changed, 46 insertions(+)
 create mode 100644 debian/extra/units/emergency.service.d/sulogin.conf
 create mode 12 debian/extra/units/rescue.service.d/sulogin.conf
 create mode 100644 debian/patches/debian/sulogin-allow-args.patch

diff --git a/debian/extra/units/emergency.service.d/sulogin.conf b/debian/extra/units/emergency.service.d/sulogin.conf
new file mode 100644
index 0..99691cda4
--- /dev/null
+++ b/debian/extra/units/emergency.service.d/sulogin.conf
@@ -0,0 +1,3 @@
+# emergency.service and rescue.service dropin, see #802211
+[Service]
+Environment=SULOGIN_ARGS=--force
diff --git a/debian/extra/units/rescue.service.d/sulogin.conf b/debian/extra/units/rescue.service.d/sulogin.conf
new file mode 12
index 0..73a838abe
--- /dev/null
+++ b/debian/extra/units/rescue.service.d/sulogin.conf
@@ -0,0 +1 @@
+../emergency.service.d/sulogin.conf
\ No newline at end of file
diff --git a/debian/patches/debian/sulogin-allow-args.patch b/debian/patches/debian/sulogin-allow-args.patch
new file mode 100644
index 0..825c6ddb9
--- /dev/null
+++ b/debian/patches/debian/sulogin-allow-args.patch
@@ -0,0 +1,41 @@
+WIP: completely untested (and unfinished).
+
+Allow setting sulogin arguments via rescue.service and emergency.service
+dropins that contains Environment=SULOGIN_ARGS=--force. This is useful
+to allow passwordless root login even when root account is disabled
+(which is the case when you don't give the password in debian-installer
+and also every ubuntu installation).
+
+See https://bugs.debian.org/802211
+
+--- a/src/sulogin-shell/sulogin-shell.c
 b/src/sulogin-shell/sulogin-shell.c
+@@ -89,6 +89,7 @@
+ 
+ int main(int argc, char *argv[]) {
+ static const char* const sulogin_cmdline[] = {SULOGIN, NULL};
++char *env_sulogin_args;
+ int r;
+ 
+ log_set_target(LOG_TARGET_AUTO);
+@@ -97,7 +98,19 @@
+ 
+ print_mode(argc > 1 ? argv[1] : "");
+ 
+-fork_wait(sulogin_cmdline);
++env_sulogin_args = getenv("SULOGIN_ARGS");
++if (env_sulogin_args) {
++		char **sulogin_cmdline_args;
++
++	   	sulogin_cmdline_args = strv_split(env_sulogin_args, " ");
++		// FIXME: if (... == NULL) ...
++		strv_push_prepend(_cmdline_args, SULOGIN);
++
++		fork_wait(sulogin_cmdline_args);
++
++		strv_free_free(sulogin_cmdline_args);
++} else
++fork_wait(sulogin_cmdline);
+ 
+ r = start_default_target();
+ 
diff --git a/debian/patches/series b/debian/patches/series
index 508749322..bbba7a39d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -24,3 +24,4 @@ debian/Add-env-variable-for-machine-ID-path.patch
 debian/Mark-test-timesync-as-manual.patch
 debian/Avoid-requiring-a-kvm-system-group.patch
 debian/Revert-tests-when-running-a-manager-object-in-a-test-migr.patch
+debian/sulogin-allow-args.patch
-- 
2.11.0

___
Pkg-systemd-maintainers mailing list
Pkg-systemd-maintainers@lists.alioth.debian.org