[PATCH] broken ESC navigation if authentication is used

[PATCH] broken ESC navigation if authentication is used

[Florian Kaiser]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

Hi,

we are using grub2 with authentication enabled and multiple submenus.
Unfortunately it is not possible to return to a previous menu with ESC without 
triggering a superuser password prompt. This is not the desired behavior in 
my opinion.
I attached a patch to this email, which removes the password prompt when 
pressing escape.



Thank you very much in advance

Florian Kaiser
Microkernel Systems
tel +49 89 991950 - 0
florian_kaiser@genua.de

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
genua
Gesellschaft für Netzwerk - und Unix-Administration mbH
Domagkstr. 7, D-85551 Kirchheim. http://www.genua.de
Tel: (089) 99 19 50-0, Fax: (089) 99 19 50 - 999

Geschäftsführer: Dr. Magnus Harlander, Dr. Michaela Harlander,
Bernhard Schneck. Amtsgericht München HRB 98238
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~





[-- Attachment #2: 0001-grub-fix-broken-authorization-behavior-when-pressing.patch --]
[-- Type: text/x-diff, Size: 1476 bytes --]

From 1c4129d2f1751ae387fc5fd4073d70a3fa80f1e2 Mon Sep 17 00:00:00 2001
From: Florian Kaiser <fkaiser@genua.de>
Date: Tue, 9 Jun 2015 19:01:03 +0200
Subject: [PATCH] grub: fix broken authorization behavior when pressing ESC

This change removes a superfluous authorization request,
which prevented the user from navigating backwards through
the menu tree using the ESC Key.

Change-Id: Ia4cd9ce45241e70758a6eeac4a2b33a9d73c0c6e
---
 contrib/grub2/grub-core/normal/menu.c |   25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/contrib/grub2/grub-core/normal/menu.c b/contrib/grub2/grub-core/normal/menu.c
index b47991a..27978a6 100644
--- a/contrib/grub2/grub-core/normal/menu.c
+++ b/contrib/grub2/grub-core/normal/menu.c
@@ -886,27 +886,10 @@ show_menu (grub_menu_t menu, int nested, int autobooted)
 grub_err_t
 grub_show_menu (grub_menu_t menu, int nested, int autoboot)
 {
-  grub_err_t err1, err2;
+  grub_err_t err;
 
-  while (1)
-    {
-      err1 = show_menu (menu, nested, autoboot);
-      autoboot = 0;
-      grub_print_error ();
-
-      if (grub_normal_exit_level)
-	break;
-
-      err2 = grub_auth_check_authentication (NULL);
-      if (err2)
-	{
-	  grub_print_error ();
-	  grub_errno = GRUB_ERR_NONE;
-	  continue;
-	}
-
-      break;
-    }
+  err = show_menu (menu, nested, autoboot);
+  grub_print_error ();
 
-  return err1;
+  return err;
 }
-- 
1.7.10.4

Re: [PATCH] broken ESC navigation if authentication is used

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Please fix mime type. I can't view your attachment on the phone
Le 10 juin 2015 19:00, "Florian Kaiser" <florian_kaiser@genua.de> a écrit :

> Hi,
>
> we are using grub2 with authentication enabled and multiple submenus.
> Unfortunately it is not possible to return to a previous menu with ESC
> without
> triggering a superuser password prompt. This is not the desired behavior in
> my opinion.
> I attached a patch to this email, which removes the password prompt when
> pressing escape.
>
>
>
> Thank you very much in advance
>
> Florian Kaiser
> Microkernel Systems
> tel +49 89 991950 - 0
> florian_kaiser@genua.de
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> genua
> Gesellschaft für Netzwerk - und Unix-Administration mbH
> Domagkstr. 7, D-85551 Kirchheim. http://www.genua.de
> Tel: (089) 99 19 50-0, Fax: (089) 99 19 50 - 999
>
> Geschäftsführer: Dr. Magnus Harlander, Dr. Michaela Harlander,
> Bernhard Schneck. Amtsgericht München HRB 98238
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>

[-- Attachment #2: Type: text/html, Size: 1864 bytes --]

Re: [PATCH] broken ESC navigation if authentication is used

[Andrei Borzenkov]

В Wed, 10 Jun 2015 18:29:59 +0200
Florian Kaiser <florian_kaiser@genua.de> пишет:

> Hi,
> 
> we are using grub2 with authentication enabled and multiple submenus.
> Unfortunately it is not possible to return to a previous menu with ESC without 
> triggering a superuser password prompt. This is not the desired behavior in 
> my opinion.
> I attached a patch to this email, which removes the password prompt when 
> pressing escape.
> 

Looks OK; I'm not sure why this was needed in the first place - it does
not look like it is even possible to exit primary menu. 

Vladimir, OK to commit?

Re: [PATCH] broken ESC navigation if authentication is used

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

This patch may allow to escape to shell if menu was called from context
without menu entries. This may happen inadvertently I.a. when using
configfile. You need to add an additional parameter to indicate whether
it's OK to break from menu
Le 10 juin 2015 21:32, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :

> В Wed, 10 Jun 2015 18:29:59 +0200
> Florian Kaiser <florian_kaiser@genua.de> пишет:
>
> > Hi,
> >
> > we are using grub2 with authentication enabled and multiple submenus.
> > Unfortunately it is not possible to return to a previous menu with ESC
> without
> > triggering a superuser password prompt. This is not the desired behavior
> in
> > my opinion.
> > I attached a patch to this email, which removes the password prompt when
> > pressing escape.
> >
>
> Looks OK; I'm not sure why this was needed in the first place - it does
> not look like it is even possible to exit primary menu.
>
> Vladimir, OK to commit?
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 1611 bytes --]

Re: [PATCH] broken ESC navigation if authentication is used

[Andrei Borzenkov]

В Wed, 10 Jun 2015 21:35:51 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> This patch may allow to escape to shell if menu was called from context
> without menu entries. This may happen inadvertently I.a. when using
> configfile. You need to add an additional parameter to indicate whether
> it's OK to break from menu

Could you explain? Grub does

grub_enter_normal
  grub_normal_execute
    grub_show_menu
  grub_cmdline_run

if after processing config file there are no menu entries we do not
even call grub_show_menu. And even if we do, after return from it there
is mandatory authentication in grub_cmdline_run.

I see how it could happen in original commit when authentication was
added, but I miss code path that cause it now. 

> Le 10 juin 2015 21:32, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :
> 
> > В Wed, 10 Jun 2015 18:29:59 +0200
> > Florian Kaiser <florian_kaiser@genua.de> пишет:
> >
> > > Hi,
> > >
> > > we are using grub2 with authentication enabled and multiple submenus.
> > > Unfortunately it is not possible to return to a previous menu with ESC
> > without
> > > triggering a superuser password prompt. This is not the desired behavior
> > in
> > > my opinion.
> > > I attached a patch to this email, which removes the password prompt when
> > > pressing escape.
> > >
> >
> > Looks OK; I'm not sure why this was needed in the first place - it does
> > not look like it is even possible to exit primary menu.
> >
> > Vladimir, OK to commit?
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

Re: [PATCH] broken ESC navigation if authentication is used

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On 11.06.2015 05:55, Andrei Borzenkov wrote:
> В Wed, 10 Jun 2015 21:35:51 +0200
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> 
>> This patch may allow to escape to shell if menu was called from context
>> without menu entries. This may happen inadvertently I.a. when using
>> configfile. You need to add an additional parameter to indicate whether
>> it's OK to break from menu
> 
> Could you explain? Grub does
> 
> grub_enter_normal
>   grub_normal_execute
>     grub_show_menu
>   grub_cmdline_run
> 
> if after processing config file there are no menu entries we do not
> even call grub_show_menu. And even if we do, after return from it there
> is mandatory authentication in grub_cmdline_run.
> 
Imagine something like following:
grub.cfg:
# Use another config file
configfile grub2.cfg
grub2.cfg:
superusers=root
....
Then pressing escape would lead you to the parent context where there is
no password protection.
Question is whether this is a misconfiguration on grub.cfg side (i.a.
should have been source, not configfile) or something to deal on code side.
> I see how it could happen in original commit when authentication was
> added, but I miss code path that cause it now. 
> 
>> Le 10 juin 2015 21:32, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :
>>
>>> В Wed, 10 Jun 2015 18:29:59 +0200
>>> Florian Kaiser <florian_kaiser@genua.de> пишет:
>>>
>>>> Hi,
>>>>
>>>> we are using grub2 with authentication enabled and multiple submenus.
>>>> Unfortunately it is not possible to return to a previous menu with ESC
>>> without
>>>> triggering a superuser password prompt. This is not the desired behavior
>>> in
>>>> my opinion.
>>>> I attached a patch to this email, which removes the password prompt when
>>>> pressing escape.
>>>>
>>>
>>> Looks OK; I'm not sure why this was needed in the first place - it does
>>> not look like it is even possible to exit primary menu.
>>>
>>> Vladimir, OK to commit?
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH] broken ESC navigation if authentication is used

[Andrei Borzenkov]

On Sun, Nov 8, 2015 at 11:55 PM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 11.06.2015 05:55, Andrei Borzenkov wrote:
>> В Wed, 10 Jun 2015 21:35:51 +0200
>> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
>>
>>> This patch may allow to escape to shell if menu was called from context
>>> without menu entries. This may happen inadvertently I.a. when using
>>> configfile. You need to add an additional parameter to indicate whether
>>> it's OK to break from menu
>>
>> Could you explain? Grub does
>>
>> grub_enter_normal
>>   grub_normal_execute
>>     grub_show_menu
>>   grub_cmdline_run
>>
>> if after processing config file there are no menu entries we do not
>> even call grub_show_menu. And even if we do, after return from it there
>> is mandatory authentication in grub_cmdline_run.
>>
> Imagine something like following:
> grub.cfg:
> # Use another config file
> configfile grub2.cfg
> grub2.cfg:
> superusers=root
> ....
> Then pressing escape would lead you to the parent context where there is
> no password protection.
> Question is whether this is a misconfiguration on grub.cfg side (i.a.
> should have been source, not configfile) or something to deal on code side.

We already have "nested" argument. We probably can convert this into
enum or bit field to differentiate between new scope
(normal/configfile) or submenu. Does it sound sane?

>> I see how it could happen in original commit when authentication was
>> added, but I miss code path that cause it now.
>>
>>> Le 10 juin 2015 21:32, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :
>>>
>>>> В Wed, 10 Jun 2015 18:29:59 +0200
>>>> Florian Kaiser <florian_kaiser@genua.de> пишет:
>>>>
>>>>> Hi,
>>>>>
>>>>> we are using grub2 with authentication enabled and multiple submenus.
>>>>> Unfortunately it is not possible to return to a previous menu with ESC
>>>> without
>>>>> triggering a superuser password prompt. This is not the desired behavior
>>>> in
>>>>> my opinion.
>>>>> I attached a patch to this email, which removes the password prompt when
>>>>> pressing escape.
>>>>>
>>>>
>>>> Looks OK; I'm not sure why this was needed in the first place - it does
>>>> not look like it is even possible to exit primary menu.
>>>>
>>>> Vladimir, OK to commit?
>>>>

Re: [PATCH] broken ESC navigation if authentication is used

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

08.11.2015 23:55, Vladimir 'φ-coder/phcoder' Serbinenko пишет:
> On 11.06.2015 05:55, Andrei Borzenkov wrote:
>> В Wed, 10 Jun 2015 21:35:51 +0200
>> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
>>
>>> This patch may allow to escape to shell if menu was called from context
>>> without menu entries. This may happen inadvertently I.a. when using
>>> configfile. You need to add an additional parameter to indicate whether
>>> it's OK to break from menu
>>
>> Could you explain? Grub does
>>
>> grub_enter_normal
>>    grub_normal_execute
>>      grub_show_menu
>>    grub_cmdline_run
>>
>> if after processing config file there are no menu entries we do not
>> even call grub_show_menu. And even if we do, after return from it there
>> is mandatory authentication in grub_cmdline_run.
>>
> Imagine something like following:
> grub.cfg:
> # Use another config file
> configfile grub2.cfg
> grub2.cfg:
> superusers=root
> ....
> Then pressing escape would lead you to the parent context where there is
> no password protection.
> Question is whether this is a misconfiguration on grub.cfg side (i.a.
> should have been source, not configfile) or something to deal on code side.


OK what about attached patch? it moves authentication where it belongs - 
to return from nested configfile.

[-- Attachment #2: submenu-auth.diff --]
[-- Type: text/x-patch, Size: 3069 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] normal: allow return from submenu without authentication

This change removes a superfluous authorization request,
which prevented the user from navigating backwards through
the menu tree using the ESC Key.

To preserve current semantic, require authentication when returning
from menu in nested configfile.

Based on patch suggested by Florian Kaiser <fkaiser@genua.de>
Also-By: Florian Kaiser <fkaiser@genua.de>

---
 grub-core/normal/main.c | 21 ++++++++++++++++++---
 grub-core/normal/menu.c | 34 ++--------------------------------
 include/grub/normal.h   |  2 +-
 3 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index 78a70a8..dc675ae 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -287,9 +287,24 @@ grub_normal_execute (const char *config, int nested, int batch)
 	{
 
 	  grub_boot_time ("Entering menu");
-	  grub_show_menu (menu, nested, 0);
-	  if (nested)
-	    grub_normal_free_menu (menu);
+	  while (1)
+	    {
+	      grub_show_menu (menu, nested, 0);
+	      if (grub_normal_exit_level)
+		break;
+	      if (nested)
+		{
+		  if (grub_auth_check_authentication (NULL))
+		    {
+		      grub_print_error ();
+		      grub_errno = GRUB_ERR_NONE;
+		      grub_wait_after_message ();
+		      continue;
+		    }
+		  grub_normal_free_menu (menu);
+		  break;
+		}
+	    }
 	}
     }
 }
diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index 719e2fb..d93c19e 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -852,8 +852,8 @@ static struct grub_menu_execute_callback execution_callback =
   .notify_failure = notify_execution_failure
 };
 
-static grub_err_t
-show_menu (grub_menu_t menu, int nested, int autobooted)
+void
+grub_show_menu (grub_menu_t menu, int nested, int autobooted)
 {
   while (1)
     {
@@ -879,34 +879,4 @@ show_menu (grub_menu_t menu, int nested, int autobooted)
       if (autobooted)
 	break;
     }
-
-  return GRUB_ERR_NONE;
-}
-
-grub_err_t
-grub_show_menu (grub_menu_t menu, int nested, int autoboot)
-{
-  grub_err_t err1, err2;
-
-  while (1)
-    {
-      err1 = show_menu (menu, nested, autoboot);
-      autoboot = 0;
-      grub_print_error ();
-
-      if (grub_normal_exit_level)
-	break;
-
-      err2 = grub_auth_check_authentication (NULL);
-      if (err2)
-	{
-	  grub_print_error ();
-	  grub_errno = GRUB_ERR_NONE;
-	  continue;
-	}
-
-      break;
-    }
-
-  return err1;
 }
diff --git a/include/grub/normal.h b/include/grub/normal.h
index 218cbab..ae2e0dd 100644
--- a/include/grub/normal.h
+++ b/include/grub/normal.h
@@ -112,7 +112,7 @@ void grub_print_message_indented (const char *msg, int margin_left,
 				  struct grub_term_output *term);
 void
 grub_menu_text_register_instances (int entry, grub_menu_t menu, int nested);
-grub_err_t
+void
 grub_show_menu (grub_menu_t menu, int nested, int autobooted);
 
 /* Defined in `handler.c'.  */
-- 
tg: (c899d9f..) u/submenu-without-authentication (depends on: master)

Escape to CLI (was: Re: [PATCH] broken ESC navigation if authentication is used)

[Andrei Borzenkov]

10.06.2015 19:29, Florian Kaiser пишет:
> Hi,
>
> we are using grub2 with authentication enabled and multiple submenus.
> Unfortunately it is not possible to return to a previous menu with ESC without
> triggering a superuser password prompt. This is not the desired behavior in
> my opinion.
> I attached a patch to this email, which removes the password prompt when
> pressing escape.
>

Actually I could not even reproduce it at first. The problem is, new 
submenu opens new context and unless `superusers' is exported as soon as 
we enter submenu no authentication is required at all (including 
entering CLI).

I am not sure whether this is intentional, but this is definitely 
unexpected. And due to complete lack of description of variable scoping 
in GRUB it is also undocumented.

Options are

- simply document variable scoping and mention it in authentication 
description

- propagate superusers to submenus, rely on normal export in nested 
config. Still needs documentation what export actually does.

- export superusers by default and document it in description.