| Message ID | 1429662311.78270.1471387188671.JavaMail.zimbra@raptorengineeringinc.com |
|---|---|
| State | Superseded |
| Headers | show |
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); >
-----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-----
-----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-----
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);
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(-)