[2/3,V6,revision,2] Disable shell access when lockdown is active
diff mbox

Message ID 1429662311.78270.1471387188671.JavaMail.zimbra@raptorengineeringinc.com
State Superseded
Headers show

Commit Message

Timothy Pearson Aug. 16, 2016, 10:39 p.m. UTC
This patch disables direct command line access when the /etc/pb-lockdown
file is present.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
---
 ui/ncurses/nc-cui.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Comments

Samuel Mendoza-Jonas Aug. 17, 2016, 3:47 a.m. UTC | #1
On Tue, 2016-08-16 at 17:39 -0500, Timothy Pearson wrote:
> This patch disables direct command line access when the /etc/pb-lockdown
> file is present.

Bar a small comment below, this patch is fine - except that I'm not sold
on guaranteeing that you can never reach the console with this patch.
What if petitboot-nc crashes? What if a clever user finds a way to exit
ncurses without hitting the cui_atexit() function? What if, as with all
current users of Petitboot, the user just enters xmon?

How critical is it to your security model that the user (which is most
likely running as root) can not access a shell? If it's necessary this
feels like something that should be handled in, for example, the
buildroot layer.

> 
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> ---
>  ui/ncurses/nc-cui.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c
> index 09b63b0..c2f1c83 100644
> --- a/ui/ncurses/nc-cui.c
> +++ b/ui/ncurses/nc-cui.c
> @@ -25,6 +25,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
> +#include <sys/reboot.h>
>  
>  #include "log/log.h"
>  #include "pb-protocol/pb-protocol.h"
> @@ -47,6 +48,14 @@ extern const struct help_text main_menu_help_text;
>  
>  static struct pmenu *main_menu_init(struct cui *cui);
>  
> +static bool lockdown_active(void)
> +{
> +	bool lockdown = false;
> +	if (access(LOCKDOWN_FILE, F_OK) != -1)
> +		lockdown = true;
> +	return lockdown;
> +}
> +
>  static void cui_start(void)
>  {
>  	initscr();			/* Initialize ncurses. */
> @@ -94,6 +103,13 @@ static void cui_atexit(void)
>  	clear();
>  	refresh();
>  	endwin();
> +
> +	bool lockdown = lockdown_active();
> +
> +	while (lockdown) {
> +		sync();
> +		reboot(RB_AUTOBOOT);
> +	}

If reboot returns with an error, do you want to loop forever, or cancel
exiting with a message to the user?

>  }
>  
>  /**
> @@ -826,6 +842,7 @@ static struct pmenu *main_menu_init(struct cui *cui)
>  	struct pmenu_item *i;
>  	struct pmenu *m;
>  	int result;
> +	bool lockdown = lockdown_active();
>  
>  	m = pmenu_init(cui, 7, cui_on_exit);
>  	if (!m) {
> @@ -869,7 +886,10 @@ static struct pmenu *main_menu_init(struct cui *cui)
>  	i->on_execute = menu_add_url_execute;
>  	pmenu_item_insert(m, i, 5);
>  
> -	i = pmenu_item_create(m, _("Exit to shell"));
> +	if (lockdown)
> +		i = pmenu_item_create(m, _("Reboot"));
> +	else
> +		i = pmenu_item_create(m, _("Exit to shell"));
>  	i->on_execute = pmenu_exit_cb;
>  	pmenu_item_insert(m, i, 6);
>
Timothy Pearson Aug. 17, 2016, 6:37 p.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/16/2016 10:47 PM, Samuel Mendoza-Jonas wrote:
> On Tue, 2016-08-16 at 17:39 -0500, Timothy Pearson wrote:
>> This patch disables direct command line access when the /etc/pb-lockdown
>> file is present.
> 
> Bar a small comment below, this patch is fine - except that I'm not sold
> on guaranteeing that you can never reach the console with this patch.
> What if petitboot-nc crashes? What if a clever user finds a way to exit
> ncurses without hitting the cui_atexit() function? What if, as with all
> current users of Petitboot, the user just enters xmon?

This is why petitboot UI execution is immediately followed by a hard
reboot in the tutorial posted on the Raptor Engineering website.  I will
be looking into buildroot after we get this merged, at the moment I have
enough source trees with local changes and don't need another... ;-)

> How critical is it to your security model that the user (which is most
> likely running as root) can not access a shell? If it's necessary this
> feels like something that should be handled in, for example, the
> buildroot layer.

It is critical, but this change at least will provide some protection if
petitboot's is naively run outside of its security wrapper.  It also
indicates to the user that they should not expect access to a shell.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJXtK7UAAoJEK+E3vEXDOFbcHUIALnitUSFCu+4L3RK5uQVIA4R
bNmpEqaQMZYyXs0qz2V3+P7rZoXorPKOcvDuqcwqGKDyOwHA1FSnI7WE4HEQs44r
LsviT4CyaY8EpEQv4Q3g0jq1w4sVDPK4ptqfB9HK8oo/u1cpivdpKeEuQEjSLAt4
KYUJpP6BRMjw2BtzJwPSe8DtzZ62YLv9EYlDm84ioBn2/MCxp9mA1oAczEV8REqz
DgSnipCQ5IElOxQmUQ+SOj2S1zmeT9LlrysWiHzucI41bZulu71TIY3YpfvTJWN7
g4fivFfSrqDBiOkKHTxqYz6SjqpsZ0j2BzA+9gNmllwRMShsabNgssP3oAXbwko=
=uxCG
-----END PGP SIGNATURE-----
Timothy Pearson Aug. 18, 2016, 9:48 a.m. UTC | #3
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/16/2016 10:47 PM, Samuel Mendoza-Jonas wrote:
> If reboot returns with an error, do you want to loop forever, or cancel
> exiting with a message to the user?

I'm not entirely sure how reboot can fail "out of the blue" (i.e. if
it's known to work on a platform it should probably continue to work on
that platform), but if it were to fail I guess the end user would end up
having to perform a manual reset.

Part of the problem here is that the reboot call is in the general exit
case; trying to move it before UI exit would require major surgery on
the existing UI exit handling code (and still not catch every corner
case, such as a crash in petitboot itself).

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJXtYRoAAoJEK+E3vEXDOFbd/wIAJgXOmeY7YgNCk2ayGpVgZbz
Od+PICV5yhLgWxePSjCz3Xc6lx57t5lgBQB0YPJjtUOd7niXWCWbIAyEcP0n6kcJ
JLqAV9zefhhL3jxFdh1hDbOnWsdovAbZ7etOq8NEqS3LYPG50eUlAANUZP0WQHw5
xtmRflBUBqAtTO+R4CHYU4rH7QsG87Ml0sBg72RGurpsoxn9KfMfTB+xXl7fLoyt
vvQ0Ki/hLtcUYWjrl4ue1KpWnfqYhaJHXcAGvujDmo5Ra+Ajp2f9+oje1s9zA/n8
93CTCC+NsoOUcUWNb0LxKdLIOr9oue0iVB4VmzqBTnQNOKukxZg3uGKZaUfMI50=
=0TbG
-----END PGP SIGNATURE-----

Patch
diff mbox

diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c
index 09b63b0..c2f1c83 100644
--- a/ui/ncurses/nc-cui.c
+++ b/ui/ncurses/nc-cui.c
@@ -25,6 +25,7 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <sys/ioctl.h>
+#include <sys/reboot.h>
 
 #include "log/log.h"
 #include "pb-protocol/pb-protocol.h"
@@ -47,6 +48,14 @@  extern const struct help_text main_menu_help_text;
 
 static struct pmenu *main_menu_init(struct cui *cui);
 
+static bool lockdown_active(void)
+{
+	bool lockdown = false;
+	if (access(LOCKDOWN_FILE, F_OK) != -1)
+		lockdown = true;
+	return lockdown;
+}
+
 static void cui_start(void)
 {
 	initscr();			/* Initialize ncurses. */
@@ -94,6 +103,13 @@  static void cui_atexit(void)
 	clear();
 	refresh();
 	endwin();
+
+	bool lockdown = lockdown_active();
+
+	while (lockdown) {
+		sync();
+		reboot(RB_AUTOBOOT);
+	}
 }
 
 /**
@@ -826,6 +842,7 @@  static struct pmenu *main_menu_init(struct cui *cui)
 	struct pmenu_item *i;
 	struct pmenu *m;
 	int result;
+	bool lockdown = lockdown_active();
 
 	m = pmenu_init(cui, 7, cui_on_exit);
 	if (!m) {
@@ -869,7 +886,10 @@  static struct pmenu *main_menu_init(struct cui *cui)
 	i->on_execute = menu_add_url_execute;
 	pmenu_item_insert(m, i, 5);
 
-	i = pmenu_item_create(m, _("Exit to shell"));
+	if (lockdown)
+		i = pmenu_item_create(m, _("Reboot"));
+	else
+		i = pmenu_item_create(m, _("Exit to shell"));
 	i->on_execute = pmenu_exit_cb;
 	pmenu_item_insert(m, i, 6);