Re: Re[2]: 'password' command in GRUB 2?

[Robert Millan]

On Sun, Jul 26, 2009 at 11:29:01PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> +  /* Allowed users. NULL means 'everybody'.  */
> +  const char *users;

This sounds dangerous: it is easy to make a mistake in code that e.g.
removes a user from this list.

The "natural" meaning of an empty OR list is false.  Why not make NULL
mean `nobody' instead?

> +grub_err_t 
> +grub_auth_check_authentication (const char *userlist)
> +{
> +  char login[1024] = {0};

Please avoid arbitrary limits.  If the grub_cmdline_get() API is enforcing
them, then this function is wrong and should be using malloc() instead (like,
say, getline() or asprintf() do).

> +  if (!grub_cmdline_get ("Enter username: ", login, sizeof (login) - 1, 0, 
> 0, 0))
> +    return grub_error (GRUB_ERR_ACCESS_DENIED, "login aborted");
> +
> +  if (!grub_list_iterate (GRUB_AS_LIST (users), hook) || ! cur->callback)
> +    {
> +      /* XXX Show a fake password prompt*/
> +      return grub_error (GRUB_ERR_ACCESS_DENIED, "password incorrect");      
> +    }
> +  err = cur->callback (login, cur->arg);
> +  if (is_authenticated (userlist))
> +    return GRUB_ERR_NONE;
> +  return grub_error (GRUB_ERR_ACCESS_DENIED, "access denied");

Please capitalize user messages.

> @@ -164,6 +165,7 @@ grub_normal_add_menu_entry (int argc, const char **args,
>    int i;
>    struct grub_menu_entry_class *classes_head;  /* Dummy head node for list.  
> */
>    struct grub_menu_entry_class *classes_tail;
> +  char *users = 0;

Please use `NULL' for pointers (see below).

> +  err = grub_auth_check_authentication (0);
> [...]
> +    err = grub_auth_check_authentication (entry->users);

This is an example on why using 0 for pointers is confusing.  When I read
the first line, since I didn't know the declaration of
grub_auth_check_authentication(), this lead me to think its parameter is some
sort of boolean or enum.  Had `NULL' been used instead, I'd inmediately notice
it's actually a data structure.

When you're already familiar with the code, it doesn't matter, but for everyone
else it makes things easier to figure out (which helps lowering the entry
barrier to GRUB hacking).

>  grub_menu_viewer_show_menu (grub_menu_t menu, int nested)
>  {
>    grub_menu_viewer_t cur = get_current_menu_viewer ();
> +  grub_err_t err1, err2;
>    if (!cur)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "No menu viewer available.");
>  
> -  return cur->show_menu (menu, nested);
> +  while (1)
> +    {
> +      err1 = cur->show_menu (menu, nested);
> +      grub_print_error ();
> +
> +      err2 = grub_auth_check_authentication (0);
> +      if (err2)
> +     {
> +       grub_print_error ();
> +       grub_errno = GRUB_ERR_NONE;
> +       continue;
> +     }
> +    }
> + 
> +  return err1;
>  }

How do you exit this loop?  Or is it intentional that it can't be exitted?
(in the latter case, a comment would help).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."