All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] read --echo=[yes|no|wildcard]
@ 2008-02-10 13:16 Robert Millan
  2008-02-10 13:56 ` Isaac Dupree
  2008-02-10 20:16 ` [PATCH] read --echo=[yes|no|wildcard] Yoshinori K. Okuji
  0 siblings, 2 replies; 15+ messages in thread
From: Robert Millan @ 2008-02-10 13:16 UTC (permalink / raw)
  To: grub-devel

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


Adds a parameter to define echoing behaviour in read.  Then one can use
--echo=no or --echo=wildcard to make it suitable for reading passwords.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: read_echoing.diff --]
[-- Type: text/x-diff, Size: 2824 bytes --]

2008-02-10  Robert Millan  <rmh@aybabtu.com>

	* commands/read.c (options): New struct.
	(grub_getline): Receive a hook as argument, that will be invoked
	for each character read instead of directly printing it.  This lets
	the caller choose which action is appropiate.
	Echo the final newline.
	(grub_cmd_read): Parse --echo parameter to determine echoing behaviour.
	Pass a hook to grub_getline() that implements behaviour determined by
	--echo.
	(grub_read_init): Pass `options' parameter to grub_register_command().

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/commands/read.c ./commands/read.c
--- ../grub2/commands/read.c	2008-02-02 21:35:08.000000000 +0100
+++ ./commands/read.c	2008-02-09 22:46:56.000000000 +0100
@@ -24,8 +24,14 @@
 #include <grub/term.h>
 #include <grub/types.h>
 
+static const struct grub_arg_option options[] =
+  {
+    {"echo", 'e', 0, "whether to echo user input", "[yes|no|wildcard]", ARG_TYPE_STRING},
+    {0, 0, 0, 0, 0, 0}
+  };
+
 static char *
-grub_getline (void)
+grub_getline (void (*hook) (char c))
 {
   int i;
   char *line;
@@ -39,8 +45,7 @@ grub_getline (void)
   while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
     {
       line[i] = grub_getkey ();
-      if (grub_isprint (line[i]))
-	grub_putchar (line[i]);
+      hook (line[i]);
       i++;
       tmp = grub_realloc (line, 1 + i + sizeof('\0'));
       if (! tmp)
@@ -51,14 +56,53 @@ grub_getline (void)
       line = tmp;
     }
   line[i] = '\0';
+  grub_putchar ('\n');
 
   return line;
 }
 
+enum
+{
+  ECHO_YES,
+  ECHO_NO,
+  ECHO_WILDCARD,
+};
+
 static grub_err_t
-grub_cmd_read (struct grub_arg_list *state UNUSED, int argc, char **args)
+grub_cmd_read (struct grub_arg_list *state, int argc, char **args)
 {
-  char *line = grub_getline ();
+  char *line;
+  int echo = ECHO_YES;
+
+  if (state[0].set)
+    {
+      grub_printf ("%s\n", state[0].arg);
+      if (! grub_strcmp (state[0].arg, "no"))
+	echo = ECHO_NO;
+      else if (! grub_strcmp (state[0].arg, "wildcard"))
+	echo = ECHO_WILDCARD;
+      else if (grub_strcmp (state[0].arg, "yes"))
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid argument");
+    }
+
+  auto void hook (char);
+  void hook (char c)
+  {
+    if (! grub_isprint (c))
+      return;
+
+    switch (echo)
+      {
+      case ECHO_YES:
+	grub_putchar (c);
+	break;
+      case ECHO_WILDCARD:
+	grub_putchar ('*');
+	break;
+      }
+  }
+
+  line = grub_getline (hook);
   if (! line)
     return grub_errno;
   if (argc > 0)
@@ -72,7 +116,7 @@ grub_cmd_read (struct grub_arg_list *sta
 GRUB_MOD_INIT(read)
 {
   grub_register_command ("read", grub_cmd_read, GRUB_COMMAND_FLAG_CMDLINE,
-			 "read [ENVVAR]", "Set variable with user input", 0);
+			 "read [ENVVAR]", "Set variable with user input", options);
 }
 
 GRUB_MOD_FINI(read)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 13:16 [PATCH] read --echo=[yes|no|wildcard] Robert Millan
@ 2008-02-10 13:56 ` Isaac Dupree
  2008-02-10 15:22   ` Robert Millan
  2008-02-10 20:16 ` [PATCH] read --echo=[yes|no|wildcard] Yoshinori K. Okuji
  1 sibling, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 13:56 UTC (permalink / raw)
  To: The development of GRUB 2

Robert Millan wrote:
> Adds a parameter to define echoing behaviour in read.  Then one can use
> --echo=no or --echo=wildcard to make it suitable for reading passwords.

I wonder how suitable it is for passwords -- is the memory always erased 
before jumping to e.g. Linux? (and is it important to hide it from the 
prying eyes of the root system? Probably...)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 13:56 ` Isaac Dupree
@ 2008-02-10 15:22   ` Robert Millan
  2008-02-10 16:41     ` Isaac Dupree
  0 siblings, 1 reply; 15+ messages in thread
From: Robert Millan @ 2008-02-10 15:22 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 08:56:18AM -0500, Isaac Dupree wrote:
> Robert Millan wrote:
> >Adds a parameter to define echoing behaviour in read.  Then one can use
> >--echo=no or --echo=wildcard to make it suitable for reading passwords.
> 
> I wonder how suitable it is for passwords -- is the memory always erased 
> before jumping to e.g. Linux? (and is it important to hide it from the 
> prying eyes of the root system? Probably...)

Why?  This suggests that the Linux image you just booted is not trusted, which
I find a bit strange.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 15:22   ` Robert Millan
@ 2008-02-10 16:41     ` Isaac Dupree
  2008-02-10 17:00       ` Robert Millan
  0 siblings, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 16:41 UTC (permalink / raw)
  To: The development of GRUB 2

Robert Millan wrote:
> On Sun, Feb 10, 2008 at 08:56:18AM -0500, Isaac Dupree wrote:
>> Robert Millan wrote:
>>> Adds a parameter to define echoing behaviour in read.  Then one can use
>>> --echo=no or --echo=wildcard to make it suitable for reading passwords.
>> I wonder how suitable it is for passwords -- is the memory always erased 
>> before jumping to e.g. Linux? (and is it important to hide it from the 
>> prying eyes of the root system? Probably...)
> 
> Why?  This suggests that the Linux image you just booted is not trusted, which
> I find a bit strange.

well, suppose it runs for a few months, doesn't happen to overwrite that 
memory, and then someone hacks in and gets root access (unpatched 
security flaws can happen) and then reads the raw memory.  Then the 
local boot **and the password itself** might be unknowingly compromised 
(rather than just probably a hash of the password). (plus you might be 
booting Windows ^_^, or anything really.)  It's generally good practice, 
I think... that GnuPG tries to do, for example? (I could be remembering 
wrong)

-Isaac



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 16:41     ` Isaac Dupree
@ 2008-02-10 17:00       ` Robert Millan
  2008-02-10 18:00         ` Isaac Dupree
  0 siblings, 1 reply; 15+ messages in thread
From: Robert Millan @ 2008-02-10 17:00 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 11:41:35AM -0500, Isaac Dupree wrote:
> Robert Millan wrote:
> >On Sun, Feb 10, 2008 at 08:56:18AM -0500, Isaac Dupree wrote:
> >>Robert Millan wrote:
> >>>Adds a parameter to define echoing behaviour in read.  Then one can use
> >>>--echo=no or --echo=wildcard to make it suitable for reading passwords.
> >>I wonder how suitable it is for passwords -- is the memory always erased 
> >>before jumping to e.g. Linux? (and is it important to hide it from the 
> >>prying eyes of the root system? Probably...)
> >
> >Why?  This suggests that the Linux image you just booted is not trusted, 
> >which
> >I find a bit strange.
> 
> well, suppose it runs for a few months, doesn't happen to overwrite that 
> memory, and then someone hacks in and gets root access (unpatched 
> security flaws can happen) and then reads the raw memory.  Then the 
> local boot **and the password itself** might be unknowingly compromised 
> (rather than just probably a hash of the password). (plus you might be 
> booting Windows ^_^, or anything really.)  It's generally good practice, 
> I think... that GnuPG tries to do, for example? (I could be remembering 
> wrong)

I think that it'd be better to just erase all our environment in
grub_machine_fini() or a similar routine, than to give read specific knowledge
that its data needs this kind of special protection.  Besides, it wouldn't be
that simple since the data is controlled by the user via grub.cfg, not directly
by GRUB.

Anyway, untill we support hashing this doesn't provide any additional security,
since you can get the same from grub.cfg ;-)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 17:00       ` Robert Millan
@ 2008-02-10 18:00         ` Isaac Dupree
  2008-02-10 19:39           ` Robert Millan
  0 siblings, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 18:00 UTC (permalink / raw)
  To: The development of GRUB 2

Robert Millan wrote:
> I think that it'd be better to just erase all our environment in
> grub_machine_fini() or a similar routine, than to give read specific knowledge
> that its data needs this kind of special protection.  Besides, it wouldn't be
> that simple since the data is controlled by the user via grub.cfg, not directly
> by GRUB.

I wonder if this erasing would take any significant amount of time (in 
which case there would be a reason not to implement that to happen all 
the time)

> Anyway, untill we support hashing this doesn't provide any additional security,
> since you can get the same from grub.cfg ;-)

fairly true, assuming nothing weird happens like grub.cfg being 
thoroughly deleted in the meantime :-)

anyway if a hash is used that takes (by design) around one second on the 
machine (e.g. sha256 repeated thousands? millions? of times), then I 
suppose the time taken to erase the memory used by GRUB would be trivial 
in comparison, assuming(rightly or wrongly) a good implementation...

-Isaac



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 18:00         ` Isaac Dupree
@ 2008-02-10 19:39           ` Robert Millan
  2008-02-10 20:00             ` Isaac Dupree
  0 siblings, 1 reply; 15+ messages in thread
From: Robert Millan @ 2008-02-10 19:39 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 01:00:50PM -0500, Isaac Dupree wrote:
> anyway if a hash is used that takes (by design) around one second on the 
> machine (e.g. sha256 repeated thousands? millions? of times), then I 
> suppose the time taken to erase the memory used by GRUB would be trivial 
> in comparison, assuming(rightly or wrongly) a good implementation...

The problem is not time, it's just to find the right way to do it.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 19:39           ` Robert Millan
@ 2008-02-10 20:00             ` Isaac Dupree
  2008-02-10 20:47               ` [PATCH] erase variable data on user unset Robert Millan
  0 siblings, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 20:00 UTC (permalink / raw)
  To: The development of GRUB 2

Robert Millan wrote:
> On Sun, Feb 10, 2008 at 01:00:50PM -0500, Isaac Dupree wrote:
>> anyway if a hash is used that takes (by design) around one second on the 
>> machine (e.g. sha256 repeated thousands? millions? of times), then I 
>> suppose the time taken to erase the memory used by GRUB would be trivial 
>> in comparison, assuming(rightly or wrongly) a good implementation...
> 
> The problem is not time, it's just to find the right way to do it.

yeah. probably involves thinking about GRUB's allocation and 
deallocation mechanisms, which I don't know anything about and don't 
have time to investigate :-/

-Isaac



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 13:16 [PATCH] read --echo=[yes|no|wildcard] Robert Millan
  2008-02-10 13:56 ` Isaac Dupree
@ 2008-02-10 20:16 ` Yoshinori K. Okuji
  2008-02-10 20:49   ` Robert Millan
  1 sibling, 1 reply; 15+ messages in thread
From: Yoshinori K. Okuji @ 2008-02-10 20:16 UTC (permalink / raw)
  To: The development of GRUB 2

On Sunday 10 February 2008 14:16, Robert Millan wrote:
> Adds a parameter to define echoing behaviour in read.  Then one can use
> --echo=no or --echo=wildcard to make it suitable for reading passwords.

Can you describe how you are planning to use it?

Okuji



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] erase variable data on user unset
  2008-02-10 20:00             ` Isaac Dupree
@ 2008-02-10 20:47               ` Robert Millan
  2008-02-10 21:00                 ` Robert Millan
  2008-02-10 21:31                 ` Isaac Dupree
  0 siblings, 2 replies; 15+ messages in thread
From: Robert Millan @ 2008-02-10 20:47 UTC (permalink / raw)
  To: The development of GRUB 2

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

On Sun, Feb 10, 2008 at 03:00:31PM -0500, Isaac Dupree wrote:
> Robert Millan wrote:
> >On Sun, Feb 10, 2008 at 01:00:50PM -0500, Isaac Dupree wrote:
> >>anyway if a hash is used that takes (by design) around one second on the 
> >>machine (e.g. sha256 repeated thousands? millions? of times), then I 
> >>suppose the time taken to erase the memory used by GRUB would be trivial 
> >>in comparison, assuming(rightly or wrongly) a good implementation...
> >
> >The problem is not time, it's just to find the right way to do it.
> 
> yeah. probably involves thinking about GRUB's allocation and 
> deallocation mechanisms, which I don't know anything about and don't 
> have time to investigate :-/

This should address your concern.  As to why I propose to put this in unset
command rather than kernel, since GRUB itself doesn't have any mechanisms
where a variable would contain sensible information, I think it's better to
protect user variables only.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: unset.diff --]
[-- Type: text/x-diff, Size: 979 bytes --]

2008-02-10  Robert Millan  <rmh@aybabtu.com>

	* normal/command.c (unset_command): Erase the contents of the variable
	we're about to unset, before actually unsetting it.

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/normal/command.c ./normal/command.c
--- ../grub2/normal/command.c	2007-07-22 01:32:29.000000000 +0200
+++ ./normal/command.c	2008-02-10 21:42:44.000000000 +0100
@@ -274,10 +274,19 @@ static grub_err_t
 unset_command (struct grub_arg_list *state __attribute__ ((unused)),
 	       int argc, char **args)
 {
+  char *value;
+
   if (argc < 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "no environment variable specified");
 
+  value = grub_env_get (args[0]);
+
+  /* Users may store sensitive information in their variables (e.g. passwords),
+     so erase its content here when they choose to unset them.  */
+  if (value)
+    grub_memset (value, 0, grub_strlen (value));
+
   grub_env_unset (args[0]);
   return 0;
 }

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] read --echo=[yes|no|wildcard]
  2008-02-10 20:16 ` [PATCH] read --echo=[yes|no|wildcard] Yoshinori K. Okuji
@ 2008-02-10 20:49   ` Robert Millan
  0 siblings, 0 replies; 15+ messages in thread
From: Robert Millan @ 2008-02-10 20:49 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 09:16:59PM +0100, Yoshinori K. Okuji wrote:
> On Sunday 10 February 2008 14:16, Robert Millan wrote:
> > Adds a parameter to define echoing behaviour in read.  Then one can use
> > --echo=no or --echo=wildcard to make it suitable for reading passwords.
> 
> Can you describe how you are planning to use it?

This doesn't work yet, because of the lexer bug I mentioned in the other thread,
but roughly:

menuentry "LOCK" {
  set menu_lock=1
}

menuentry "UNLOCK" {
  echo -n "Password: "
  read --echo=wildcard password
  if test $password=grubrocks ; then
    unset menu_lock
  fi
}

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] erase variable data on user unset
  2008-02-10 20:47               ` [PATCH] erase variable data on user unset Robert Millan
@ 2008-02-10 21:00                 ` Robert Millan
  2008-02-10 21:31                 ` Isaac Dupree
  1 sibling, 0 replies; 15+ messages in thread
From: Robert Millan @ 2008-02-10 21:00 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 09:47:38PM +0100, Robert Millan wrote:
> 
> This should address your concern.  As to why I propose to put this in unset
> command rather than kernel, since GRUB itself doesn't have any mechanisms
> where a variable would contain sensible information, I think it's better to
> protect user variables only.

(for size reasons of course)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] erase variable data on user unset
  2008-02-10 20:47               ` [PATCH] erase variable data on user unset Robert Millan
  2008-02-10 21:00                 ` Robert Millan
@ 2008-02-10 21:31                 ` Isaac Dupree
  2008-02-10 21:38                   ` Isaac Dupree
  1 sibling, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 21:31 UTC (permalink / raw)
  To: The development of GRUB 2

Robert Millan wrote:
> On Sun, Feb 10, 2008 at 03:00:31PM -0500, Isaac Dupree wrote:
>> Robert Millan wrote:
>>> On Sun, Feb 10, 2008 at 01:00:50PM -0500, Isaac Dupree wrote:
>>>> anyway if a hash is used that takes (by design) around one second on the 
>>>> machine (e.g. sha256 repeated thousands? millions? of times), then I 
>>>> suppose the time taken to erase the memory used by GRUB would be trivial 
>>>> in comparison, assuming(rightly or wrongly) a good implementation...
>>> The problem is not time, it's just to find the right way to do it.
>> yeah. probably involves thinking about GRUB's allocation and 
>> deallocation mechanisms, which I don't know anything about and don't 
>> have time to investigate :-/
> 
> This should address your concern.  As to why I propose to put this in unset
> command rather than kernel, since GRUB itself doesn't have any mechanisms
> where a variable would contain sensible information, I think it's better to
> protect user variables only.

okay, is the idea that the script should explicitly unset sensitive 
variables, or are they all automatically unset upon boot? (if "unset" 
command is loaded?)

Anyway, thanks for looking into this!

-Isaac



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] erase variable data on user unset
  2008-02-10 21:31                 ` Isaac Dupree
@ 2008-02-10 21:38                   ` Isaac Dupree
  2008-02-10 21:53                     ` Robert Millan
  0 siblings, 1 reply; 15+ messages in thread
From: Isaac Dupree @ 2008-02-10 21:38 UTC (permalink / raw)
  To: The development of GRUB 2

on second thought, if grub is going to be able to boot kernels that are 
on encrypted partitions, the password might go more places in grub 
and/or have to be retained until boot time, depending how it works...



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] erase variable data on user unset
  2008-02-10 21:38                   ` Isaac Dupree
@ 2008-02-10 21:53                     ` Robert Millan
  0 siblings, 0 replies; 15+ messages in thread
From: Robert Millan @ 2008-02-10 21:53 UTC (permalink / raw)
  To: The development of GRUB 2

On Sun, Feb 10, 2008 at 04:38:37PM -0500, Isaac Dupree wrote:
> on second thought, if grub is going to be able to boot kernels that are 
> on encrypted partitions, the password might go more places in grub 
> and/or have to be retained until boot time, depending how it works...

Not until boot time, just up to the load command.  I guess you mean
something like:

echo -n "Password: "
read password
lvm_somecommand $password
linux (lvm-device)/boot/linux.img
unset password
boot

Here, the lvm module would be responsible for its own copy of the password.  It
does already know that sensitive information is being handled, so when
grub_lvm_fini() is called, it'll erase it.

The other copy is in the environment, so user clears it after loading linux,
because our env handler doesn't know that some variables might contain
sensitive info.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2008-02-10 21:55 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-10 13:16 [PATCH] read --echo=[yes|no|wildcard] Robert Millan
2008-02-10 13:56 ` Isaac Dupree
2008-02-10 15:22   ` Robert Millan
2008-02-10 16:41     ` Isaac Dupree
2008-02-10 17:00       ` Robert Millan
2008-02-10 18:00         ` Isaac Dupree
2008-02-10 19:39           ` Robert Millan
2008-02-10 20:00             ` Isaac Dupree
2008-02-10 20:47               ` [PATCH] erase variable data on user unset Robert Millan
2008-02-10 21:00                 ` Robert Millan
2008-02-10 21:31                 ` Isaac Dupree
2008-02-10 21:38                   ` Isaac Dupree
2008-02-10 21:53                     ` Robert Millan
2008-02-10 20:16 ` [PATCH] read --echo=[yes|no|wildcard] Yoshinori K. Okuji
2008-02-10 20:49   ` Robert Millan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.