Skip to contentSkip to conversation

password command implementation

PATCH In review Julien RANC 4 participants 6 messages Aug 5, 2007 – Aug 8, 2007

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 password command implementation

Hi all,

Here is my first patch for Grub2, so I hope it won't be too bad, and by advance, all my apologies for the probable errors ;-) So, please, comment gently...

These 2 patch add support for the 'password' command, with the syntax I proposed in my mail from August 1st.

Just a few remarks:
1. if you use several 'password' commands in the grub.cfg, only the last one will be valid.
2. To use MD5 passwords, you _MUST_ put quotes around the password. For example:
password --md5 '$1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/' --> This works fine password --md5 $1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/ --> no quotes, don't work
3. passwords are limited to 64 chars.

The first patch (patch_1.txt) contains the implementation itself of password management, and MD5 routines (based on the MD5 routines from Grub Legacy)

The second patch (patch_2.txt) contains modifications to the module normal to take into account the new command:
- register the command on startup
- add relevant checks and actions in menu.c
It also patches the .rmk files to compile.

I will let you comment on this, and start working on implementing the 'lock' command in the meanwhile. I hope I'll be able to send a first patch for the lock command by the end of the week.

Signature · 2 lines
Julien RANC
[email protected]
normal/md5.c +303 -0
@@ -0,0 +1,303 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* See RFC 1321 for a description of the MD5 algorithm. */
+
+#include <grub/md5.h>
+#include <grub/types.h>
+#include <grub/misc.h>
+
+/* F, G, H and I are basic MD5 functions. */
+#define F(x, y, z) (((x) & (y)) | ((~x) & (z)))
+#define G(x, y, z) (((x) & (z)) | ((y) & (~z)))
+#define H(x, y, z) ((x) ^ (y) ^ (z))
+#define I(x, y, z) ((y) ^ ((x) | (~z)))
+
+/* ROTATE_LEFT rotates x left n bits. */
+#define ROTATE_LEFT(x, n) (((x) << (n)) | ((x >> (32 - (n)))))
+
+static grub_uint32_t initstate[4] =
+{
+  0x67452301, 0xefcdab89, 0x98badcfe, 0x10325476 
+};
+
+static char s1[4] = {  7, 12, 17, 22 };
+static char s2[4] = {  5,  9, 14, 20 };
+static char s3[4] = {  4, 11, 16, 23 };
+static char s4[4] = {  6, 10, 15, 21 };
+
+static grub_uint32_t T[64] =
+{
+  0xd76aa478, 0xe8c7b756, 0x242070db, 0xc1bdceee,
+  0xf57c0faf, 0x4787c62a, 0xa8304613, 0xfd469501,
+  0x698098d8, 0x8b44f7af, 0xffff5bb1, 0x895cd7be,
+  0x6b901122, 0xfd987193, 0xa679438e, 0x49b40821,
+  0xf61e2562, 0xc040b340, 0x265e5a51, 0xe9b6c7aa,
+  0xd62f105d, 0x02441453, 0xd8a1e681, 0xe7d3fbc8,
+  0x21e1cde6, 0xc33707d6, 0xf4d50d87, 0x455a14ed,
+  0xa9e3e905, 0xfcefa3f8, 0x676f02d9, 0x8d2a4c8a,
+  0xfffa3942, 0x8771f681, 0x6d9d6122, 0xfde5380c,
+  0xa4beea44, 0x4bdecfa9, 0xf6bb4b60, 0xbebfbc70,
+  0x289b7ec6, 0xeaa127fa, 0xd4ef3085, 0x04881d05,
+  0xd9d4d039, 0xe6db99e5, 0x1fa27cf8, 0xc4ac5665,
+  0xf4292244, 0x432aff97, 0xab9423a7, 0xfc93a039,
+  0x655b59c3, 0x8f0ccc92, 0xffeff47d, 0x85845dd1,
+  0x6fa87e4f, 0xfe2ce6e0, 0xa3014314, 0x4e0811a1,
+  0xf7537e82, 0xbd3af235, 0x2ad7d2bb, 0xeb86d391
+};
+
+static const char *b64t =
+"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
+
+static grub_uint32_t state[4];
+static unsigned int length;
+static unsigned char buffer[64];
+
+static void
+md5_transform (const unsigned char block[64])
+{
+  int i, j;
+  grub_uint32_t a,b,c,d,tmp;
+  const grub_uint32_t *x = (grub_uint32_t *) block;
+
+  a = state[0];
+  b = state[1];
+  c = state[2];
+  d = state[3];
+
+  /* Round 1 */
+  for (i = 0; i < 16; i++)
+    {
+      tmp = a + F (b, c, d) + grub_le_to_cpu32 (x[i]) + T[i];
+      tmp = ROTATE_LEFT (tmp, s1[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;
+    }
+  /* Round 2 */
+  for (i = 0, j = 1; i < 16; i++, j += 5)
+    {
+      tmp = a + G (b, c, d) + grub_le_to_cpu32 (x[j & 15]) + T[i+16];
+      tmp = ROTATE_LEFT (tmp, s2[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;
+    }
+  /* Round 3 */
+  for (i = 0, j = 5; i < 16; i++, j += 3)
+    {
+      tmp = a + H (b, c, d) + grub_le_to_cpu32 (x[j & 15]) + T[i+32];
+      tmp = ROTATE_LEFT (tmp, s3[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;
+    }
+  /* Round 4 */
+  for (i = 0, j = 0; i < 16; i++, j += 7)
+    {
+      tmp = a + I (b, c, d) + grub_le_to_cpu32 (x[j & 15]) + T[i+48];
+      tmp = ROTATE_LEFT (tmp, s4[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;
+    }
+
+  state[0] += a;
+  state[1] += b;
+  state[2] += c;
+  state[3] += d;
+}
+
+static void
+md5_init(void)
+{
+  grub_memcpy ((char *) state, (char *) initstate, sizeof (initstate));
+  length = 0;
+}
+
+static void
+md5_update (const char *input, int inputlen)
+{
+  int buflen = length & 63;
+  length += inputlen;
+  if (buflen + inputlen < 64) 
+    {
+      grub_memcpy (buffer + buflen, input, inputlen);
+      buflen += inputlen;
+      return;
+    }
+  
+  grub_memcpy (buffer + buflen, input, 64 - buflen);
+  md5_transform (buffer);
+  input += 64 - buflen;
+  inputlen -= 64 - buflen;
+  while (inputlen >= 64)
+    {
+      md5_transform ((const unsigned char*) input);
+      input += 64;
+      inputlen -= 64;
+    }
+  grub_memcpy (buffer, input, inputlen);
+  buflen = inputlen;
+}
+
+static unsigned char *
+md5_final( void )
+{
+  int i, buflen = length & 63;
+
+  buffer[buflen++] = 0x80;
+
+  grub_memset (buffer+buflen, 0, 64 - buflen);
+  if (buflen > 56)
+    {
+      md5_transform (buffer);
+      grub_memset (buffer, 0, 64);
+      buflen = 0;
+    }
+  
+  *(grub_uint32_t *) (buffer + 56) = grub_cpu_to_le32 (8 * length);
+  *(grub_uint32_t *) (buffer + 60) = 0;
+  md5_transform (buffer);
+
+  for (i = 0; i < 4; i++)
+    state[i] = grub_cpu_to_le32 (state[i]);
+  return (unsigned char *) state;
+}
+
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */
+int
+grub_md5_password (const char *key, char *crypted, int check)
+{
+  int keylen = grub_strlen (key);
+  char *salt = crypted + 3; /* skip $1$ header */
+  char *p; 
+  int saltlen;
+  int i, n;
+  unsigned char alt_result[16];
+  unsigned char *digest;
+
+  if (check)
+    {
+      /* If our crypted password isn't 3 chars, then it can't be md5
+	 crypted. So, they don't match.  */
+      if (grub_strlen(crypted) <= 3)
+	return 1;
+      
+      saltlen = grub_strstr (salt, "$") - salt;
+    }
+  else
+    {
+      char *end = grub_strstr (salt, "$");
+      if (end && end - salt < 8)
+	saltlen = end - salt;
+      else
+	saltlen = 8;
+
+      salt[saltlen] = '$';
+    }
+  
+  md5_init ();
+  md5_update (key, keylen);
+  md5_update (salt, saltlen);
+  md5_update (key, keylen);
+  digest = md5_final ();
+  grub_memcpy (alt_result, digest, 16);
+  
+  grub_memcpy ((char *) state, (char *) initstate, sizeof (initstate));
+  length = 0;
+  md5_update (key, keylen);
+  md5_update (crypted, 3 + saltlen); /* include the $1$ header */
+  for (i = keylen; i > 16; i -= 16)
+    md5_update ((char *) alt_result, 16);
+  md5_update ((char *) alt_result, i);
+
+  for (i = keylen; i > 0; i >>= 1)
+    md5_update ((char*) (key + ((i & 1) ? keylen : 0)), 1);
+  digest = md5_final ();
+
+  for (i = 0; i < 1000; i++)
+    {
+      grub_memcpy (alt_result, digest, 16);
+
+      grub_memcpy ((char *) state, (char *) initstate, sizeof (initstate));
+      length = 0;
+      if ((i & 1) != 0)
+	md5_update (key, keylen);
+      else
+	md5_update ((char *) alt_result, 16);
+      
+      if (i % 3 != 0)
+	md5_update (salt, saltlen);
+
+      if (i % 7 != 0)
+	md5_update (key, keylen);
+
+      if ((i & 1) != 0)
+	md5_update ((char *) alt_result, 16);
+      else
+	md5_update (key, keylen);
+      digest = md5_final ();
+    }
+
+  p = salt + saltlen + 1;
+  for (i = 0; i < 5; i++)
+    {
+      unsigned int w = 
+	digest[i == 4 ? 5 : 12+i] | (digest[6+i] << 8) | (digest[i] << 16);
+      for (n = 4; n-- > 0;)
+	{
+	  if (check)
+	    {
+	      if (*p++ != b64t[w & 0x3f])
+		return 1;
+	    }
+	  else
+	    {
+	      *p++ = b64t[w & 0x3f];
+	    }
+	  
+	  w >>= 6;
+	}
+    }
+  {
+    unsigned int w = digest[11];
+    for (n = 2; n-- > 0;)
+      {
+	if (check)
+	  {
+	    if (*p++ != b64t[w & 0x3f])
+	      return 1;
+	  }
+	else
+	  {
+	    *p++ = b64t[w & 0x3f];
+	  }
+	
+	w >>= 6;
+      }
+  }
+
+  if (! check)
+    *p = '\0';
+  
+  return *p;
+}
+
+
normal/password.c +249 -0

249 lines not on this page · raw message

include/grub/md5.h +30 -0
@@ -0,0 +1,30 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */
+extern int grub_md5_password (const char *key, char *crypted, int check);
+
+/* For convenience.  */
+#define grub_md5_check_password(key,crypted)	grub_md5_password((key), (crypted), 1)
+#define grub_md5_make_password(key,crypted)	grub_md5_password((key), (crypted), 0)
include/grub/password.h +147 -0

147 lines not on this page · raw message

Index: conf/i386-efi.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/i386-efi.rmk,v
retrieving revision 1.19

i386-efi.rmk +1 -0
diff -u -p -r1.19 i386-efi.rmk
@@ -109,6 +109,7 @@ normal_mod_SOURCES = normal/arg.c normal
 	normal/completion.c normal/execute.c 		\
 	normal/function.c normal/lexer.c normal/main.c normal/menu.c	\
 	normal/menu_entry.c normal/misc.c grub_script.tab.c 		\
+	normal/password.c normal/md5.c					\
 	normal/script.c normal/i386/setjmp.S
 normal_mod_CFLAGS = $(COMMON_CFLAGS)
 normal_mod_ASFLAGS = $(COMMON_ASFLAGS)

Index: conf/i386-pc.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/i386-pc.rmk,v
retrieving revision 1.86

i386-pc.rmk +1 -0
diff -u -p -r1.86 i386-pc.rmk
@@ -157,6 +157,7 @@ normal_mod_SOURCES = normal/arg.c normal
 	normal/completion.c normal/execute.c		 		\
 	normal/function.c normal/lexer.c normal/main.c normal/menu.c	\
 	normal/menu_entry.c normal/misc.c grub_script.tab.c 		\
+	normal/password.c normal/md5.c					\
 	normal/script.c normal/i386/setjmp.S
 normal_mod_CFLAGS = $(COMMON_CFLAGS)
 normal_mod_ASFLAGS = $(COMMON_ASFLAGS)

Index: conf/powerpc-ieee1275.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/powerpc-ieee1275.rmk,v retrieving revision 1.69

powerpc-ieee1275.rmk +1 -0
diff -u -p -r1.69 powerpc-ieee1275.rmk
@@ -124,6 +124,7 @@ normal_mod_SOURCES = normal/arg.c normal
 	normal/completion.c normal/execute.c		 		\
 	normal/function.c normal/lexer.c normal/main.c normal/menu.c	\
 	normal/menu_entry.c normal/misc.c grub_script.tab.c 		\
+	normal/password.c normal/md5.c					\
 	normal/script.c normal/powerpc/setjmp.S
 normal_mod_CFLAGS = $(COMMON_CFLAGS)
 normal_mod_LDFLAGS = $(COMMON_LDFLAGS)

Index: conf/sparc64-ieee1275.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/sparc64-ieee1275.rmk,v retrieving revision 1.17

sparc64-ieee1275.rmk +1 -0
diff -u -p -r1.17 sparc64-ieee1275.rmk
@@ -156,6 +156,7 @@ normal_mod_SOURCES = normal/arg.c normal
 	normal/completion.c normal/execute.c				\
 	normal/function.c normal/lexer.c normal/main.c normal/menu.c	\
 	normal/menu_entry.c normal/misc.c normal/script.c		\
+	normal/password.c normal/md5.c					\
 	normal/sparc64/setjmp.S						\
 	grub_script.tab.c
 normal_mod_CFLAGS = $(COMMON_CFLAGS)

Index: normal/command.c
===================================================================
RCS file: /sources/grub/grub2/normal/command.c,v
retrieving revision 1.19

command.c +5 -0
diff -u -p -r1.19 command.c
@@ -25,6 +25,7 @@
 #include <grub/dl.h>
 #include <grub/parser.h>
 #include <grub/script.h>
+#include <grub/password.h>
 
 static grub_command_t grub_command_list;
 
@@ -391,4 +392,8 @@ grub_command_init (void)
 
   grub_register_command ("lsmod", lsmod_command, GRUB_COMMAND_FLAG_BOTH,
 			 "lsmod", "Show loaded modules.", 0);
+			 
+  grub_register_command ("password", grub_password_command, GRUB_COMMAND_FLAG_MENU,	
+			 "password [--MD5] PASSWORD [NEXT_FILE]", "Set a password", 
+			 grub_password_arg_options);
 }

Index: normal/menu.c
===================================================================
RCS file: /sources/grub/grub2/normal/menu.c,v
retrieving revision 1.18

menu.c +79 -2

113 lines not on this page · raw message

#1 lore raw

Julien RANC <[email protected]> writes:

Hi,

Quoted 3 lines

Here is my first patch for Grub2, so I hope it won't be too bad, and by advance, all my apologies for the probable errors ;-) So, please, comment gently...

I'll try... :-)

Quoted 2 lines

These 2 patch add support for the 'password' command, with the syntax I proposed in my mail from August 1st.

:-)

Quoted 8 lines

Just a few remarks:
1. if you use several 'password' commands in the grub.cfg, only the last one will be valid.
2. To use MD5 passwords, you _MUST_ put quotes around the password. For example:
password --md5 '$1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/' --> This works fine password --md5 $1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/ --> no quotes, don't work
3. passwords are limited to 64 chars.

Why this limitation? Well, I think it is not that important :)

Quoted 9 lines

The first patch (patch_1.txt) contains the implementation itself of password management, and MD5 routines (based on the MD5 routines from Grub Legacy)

The second patch (patch_2.txt) contains modifications to the module normal to take into account the new command:
- register the command on startup
- add relevant checks and actions in menu.c
It also patches the .rmk files to compile.

I wouldn't mind if this was one patch.

Quoted 3 lines

I will let you comment on this, and start working on implementing the 'lock' command in the meanwhile. I hope I'll be able to send a first patch for the lock command by the end of the week.

Great!

Can you please submit changelog entries with your patches? That makes a review easier and it it required before committing patches.

normal/md5.c line 4

Quoted 10 lines
Signature · 2 lines
Julien RANC
[email protected]
normal/md5.c +4 -0
@@ -0,0 +1,303 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.

This comes from GRUB Legacy?

normal/md5.c line 90

Quoted 7 lines
normal/md5.c +7 -0
+  /* Round 1 */
+  for (i = 0; i < 16; i++)
+    {
+      tmp = a + F (b, c, d) + grub_le_to_cpu32 (x[i]) + T[i];
+      tmp = ROTATE_LEFT (tmp, s1[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;

Please use separate lines. Please do the same below.

normal/md5.c line 184

Quoted 5 lines
normal/md5.c +5 -0
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */

This is quite an awkward interface, IMO. You you use both? Can you make two functions out of this easily?

normal/md5.c line 263

Quoted 5 lines
normal/md5.c +5 -0
+  p = salt + saltlen + 1;
+  for (i = 0; i < 5; i++)
+    {
+      unsigned int w = 
+	digest[i == 4 ? 5 : 12+i] | (digest[6+i] << 8) | (digest[i] << 16);

Please put spaces around the "+". Please put this line on multiple lines. When you use Emacs use "= ( ... );" and let Emacs format these lines for you.

normal/md5.c line 264

Quoted 1 line
normal/md5.c +1 -0
+      for (n = 4; n-- > 0;)

Are you sure this is right?

normal/password.c line 4

Quoted 7 lines
normal/password.c +4 -0
@@ -0,0 +1,249 @@
+/* password.c - Password helper function for other modules */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2004,2005,2006  Free Software Foundation, Inc.

Did you use GRUB Legacy code?

Quoted 12 lines
normal/password.c +12 -0
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software

normal/password.c line 27

Quoted 10 lines
normal/password.c +10 -0
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/dl.h> 
+#include <grub/password.h> 
+#include <grub/err.h> 		/* for error codes */
+#include <grub/mm.h>		/* for memory mngt */
+#include <grub/misc.h>		/* for string manipulation */
+#include <grub/normal.h>
+#include <grub/md5.h>

For comments, please start with a uppercase letter. End with a '.' followed by two spaces before the "*/". Can you check all your comments for this? This applies in general. I think the comments above a bit useless.

normal/password.c line 31

Quoted 3 lines
normal/password.c +3 -0
+/**
+ * The global password, used as default.
+ */

Please use comments like:
/* The global password, used as default. */

normal/password.c line 32

Quoted 1 line
normal/password.c +1 -0
+static grub_password_t global_password = 0;

The "= 0" can be left away. This is done by default.

normal/password.c line 37

Quoted 3 lines
normal/password.c +3 -0
+/* Declare private functions */
+int check_plain_text(const char *entered, const char *correct);
+int check_md5(const char *entered, const char *correct);

What is this? If they are private, better leave the comment away and declare them as static. I am actually quite sure most functions in this file should be static.

normal/password.c line 42

Quoted 4 lines
normal/password.c +4 -0
+grub_err_t
+grub_password_command(struct grub_arg_list *state,
+		      int                  argc,
+		      char                 **args)

Please format this according to the GCS. Thus one space between the type and variable name.

normal/password.c line 45

Quoted 3 lines
normal/password.c +3 -0
+{
+  if (argc <= 0)
+    goto fail;			/* too few arguments */

I prefer comments before the line. Comments on the same line tend to get messy.

normal/password.c line 48

Quoted 2 lines
normal/password.c +2 -0
+  if (argc > 2)
+    goto fail;			/* too many arguments */

Please just use return. Possibly even with a clear error message.

normal/password.c line 52

Quoted 3 lines
normal/password.c +3 -0
+  if (global_password == 0) 
+    {
+      global_password = grub_malloc(sizeof(global_password));

Add a space between grub_malloc and '('. Can you please check this for all function calls?

normal/password.c line 76

Quoted 5 lines
normal/password.c +5 -0
+  /* set the password type */
+  if (state[0].set) 
+      global_password->type = GRUB_PASSWORD_TYPE_MD5;
+  else 
+      global_password->type = GRUB_PASSWORD_TYPE_PLAIN_TEXT;

Why would we actually allow plain text passwords? Do we really want to allow people to harm themselves? :)

normal/password.c line 85

Quoted 8 lines
normal/password.c +8 -0
+  global_password->authenticated = 0;
+
+  /* OK, done */
+  return GRUB_ERR_NONE;
+
+  
+ fail:
+  return GRUB_ERR_INVALID_COMMAND;

This label is not needed. If this just returns, we can do that instead of jumping here. Never return an error without using "grub_error".

normal/password.c line 93

Quoted 4 lines
normal/password.c +4 -0
+grub_password_t 
+grub_password_create_password(grub_password_type_t type, 
+			      const char           *password,
+			      const char           *next_file)

Please fix this formatting. Same for the other functions.

normal/password.c line 99

Quoted 6 lines
normal/password.c +6 -0
+{
+  if ( password == 0 )
+    goto fail;
+
+  if ( next_file == 0 )
+    goto fail;

I would use (! password) or (password == NULL)

normal/password.c line 102

Quoted 2 lines
normal/password.c +2 -0
+  if ( type == GRUB_PASSWORD_TYPE_UNSUPPORTED )
+    goto fail;

How could this happen?

normal/password.c line 109

Quoted 6 lines
normal/password.c +6 -0
+  grub_password_t passwd;
+  passwd = grub_malloc( sizeof(passwd) );
+  passwd->type = type;
+  passwd->password = grub_strdup(password);
+  passwd->next_file = grub_strdup(next_file);
+  passwd->authenticated = 0;

Please check each of these allocations to see if they succeed. If they don't, please deallocate all before the failing one. This is actually how we usually use "fail:". On error you could use grub_password_free_password, I guess.

normal/password.c line 118

Quoted 2 lines
normal/password.c +2 -0
+void 
+grub_password_free_password(grub_password_t password)

How about just "grub_password_free"?

normal/password.c line 124

Quoted 6 lines
normal/password.c +5 -0
+{
+  if (password == 0)
+    goto finished;
 
+  if (password->password != 0)
+    grub_free(password->password);

No need to check this, grub_free does this for you.

normal/password.c line 127

Quoted 2 lines
normal/password.c +2 -0
+  if (password->next_file != 0)
+    grub_free(password->next_file);

Same here.

normal/password.c line 139

Quoted 5 lines
normal/password.c +5 -0
+int 
+grub_password_is_password_set(void)
+{
+  return (global_password != 0);
+}

Can't this be checked by the called instead?

normal/password.c

Quoted 2 lines
normal/password.c +2 -0
+int 
+grub_password_is_user_authenticated(grub_password_t password)

Please add a space.

normal/password.c line 147

Quoted 5 lines
normal/password.c +5 -0
+{
+  grub_password_t passwd;
+  int result;
+
+  if ( password != 0 )

Please use:

if (password != 0)

Can you do this for all the if-statements?

normal/password.c line 155

Quoted 8 lines
normal/password.c +8 -0
+    passwd = password;
+  else
+    passwd = global_password;
+
+  if ( passwd == 0 )
+    {
+      /* No password set : let's say it is OK */
+      result = 1;

Are you sure? Can't this be cought earlier?

normal/password.c line 160

Quoted 5 lines
normal/password.c +5 -0
+    }
+  else 
+    {
+      result = passwd->authenticated;
+    }

No need for these parenthesis.

normal/password.c line 181

Quoted 4 lines
normal/password.c +4 -0
+  /* If no password set nor passed, OK */
+  if (checked_passwd == 0) {
+    check_result = 0;
+    goto end_check;

Just return 0 here.

Quoted 1 line

+char*

char *

normal/password.c line 223

Quoted 13 lines
normal/password.c +13 -0
+grub_password_get_next_file(void)
+{
+  if (global_password == 0)
+    return 0;
+  
+  if (grub_password_is_user_authenticated(0) == 0)
+    return 0;
+  
+  if (global_password->next_file == 0) 
+    return 0;
+  
+  return grub_strdup(global_password->next_file);
+}

Will the callers handle this error? I think it is a bit hard because you also return 0 in other cases, in which there is no grub_errno set.

normal/password.c line 226

Quoted 2 lines
normal/password.c +2 -0
+/* Start of private functions */
+

Useless whiteline.

normal/password.c line 237

Quoted 11 lines
normal/password.c +11 -0
+int
+check_plain_text(const char *entered,
+		 const char *correct)
+{
+  int result;
+  result = grub_strcmp(entered, correct);
+  if (result != 0)
+    result = -1;
+
+  return result;
+}

I think this function is not really needed.

include/grub/md5.h line 4

Quoted 7 lines
include/grub/md5.h +4 -0
@@ -0,0 +1,30 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.

What is the origen of this file?

Quoted 11 lines
include/grub/md5.h +11 -0
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License

include/grub/md5.h line 26

Quoted 10 lines
include/grub/md5.h +10 -0
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */
+extern int grub_md5_password (const char *key, char *crypted, int check);

No need for the extern keyword, I think this is even wrong.

include/grub/md5.h line 30

Quoted 3 lines
include/grub/md5.h +3 -0
+/* For convenience.  */
+#define grub_md5_check_password(key,crypted)	grub_md5_password((key), (crypted), 1)
+#define grub_md5_make_password(key,crypted)	grub_md5_password((key), (crypted), 0)

It would be better to actually make these functions.

Can you please add an include guard?

include/grub/password.h line 4

Quoted 7 lines
include/grub/password.h +4 -0
@@ -0,0 +1,147 @@
+/* password.h - Header file for password management */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005  Free Software Foundation, Inc.

Where was this based on? Shouldn't 2007 be added?

Quoted 13 lines
include/grub/password.h +13 -0
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

include/grub/password.h line 28

Quoted 10 lines
include/grub/password.h +10 -0
+ */
+ 
+#ifndef GRUB_PASSWORD_H
+#define GRUB_PASSWORD_H 1
+ 
+
+#include <grub/arg.h>
+
+/* The max length of the password, in chars */
+#define GRUB_PASSWORD_MAX_LENGTH 64

Is a maximum really required?

Quoted 16 lines
include/grub/password.h +16 -0
+/* the password types */
+enum grub_password_type
+  {
+    GRUB_PASSWORD_TYPE_UNSUPPORTED,
+    GRUB_PASSWORD_TYPE_PLAIN_TEXT,
+    GRUB_PASSWORD_TYPE_MD5
+  };
+typedef enum grub_password_type grub_password_type_t;
+	
+	
+/* structure holding a password */
+struct grub_password
+{
+  char *password;		/* The password.  */
+  grub_password_type_t type;	/* The type.  */
+  char *next_file;		/* The file to jump after auth */

include/grub/password.h line 55

Quoted 10 lines
include/grub/password.h +10 -0
+  int  authenticated;		/* Flag for passwd verification status */
+};
+typedef struct grub_password *grub_password_t;
+
+/* The options available for the 'password' command */
+static const struct grub_arg_option grub_password_arg_options[] =
+  {
+    {"md5", 'm', GRUB_ARG_OPTION_OPTIONAL, "indicates the password is encrypted with MD5", 0, ARG_TYPE_NONE},
+    {0, 0, 0, 0, 0, 0}
+  };

This doesn't belong here and should go into password.c. Please have a look at hello/hello.c how to deal with this.

include/grub/password.h line 65

Quoted 9 lines
include/grub/password.h +9 -0
+/* Helper functions */
+
+/**
+ * @brief The 'password' command itself
+ *
+ * This is the callback to execute a 'password' command from a 
+ * script.
+ * See normal.h for information on the parameters
+  */

We do not use doxygen or so. Please use these '*'s but try to match the commenting style used in GRUB 2.

Quoted 7 lines
include/grub/password.h +7 -0
+grub_err_t
+grub_password_command(struct grub_arg_list *state,
+		      int                  argc,
+		      char                 **args);
+
+/**
+ * @bried Create a grub_password_t structure based on entered params

include/grub/password.h line 82

Quoted 10 lines
include/grub/password.h +10 -0
+ *
+ * This function is helpful for the 'lock' command to generate the
+ * password against which the user password will be checked.
+ *
+ * @param[in] type the password type
+ * @param[in] password the password
+ *
+ * @returns a grub_password_t object properly filled
+ * @returns 0 is invalid params are passed
+ */

Same here.

I think this function should be static so it should not be included in the header file. I think this is also the case for some other functions.

Quoted 15 lines

Index: conf/i386-efi.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/i386-efi.rmk,v
retrieving revision 1.19
diff -u -p -r1.19 i386-efi.rmk

i386-efi.rmk +1 -0
@@ -109,6 +109,7 @@ normal_mod_SOURCES = normal/arg.c normal
 	normal/completion.c normal/execute.c 		\
 	normal/function.c normal/lexer.c normal/main.c normal/menu.c	\
 	normal/menu_entry.c normal/misc.c grub_script.tab.c 		\
+	normal/password.c normal/md5.c					\
 	normal/script.c normal/i386/setjmp.S
 normal_mod_CFLAGS = $(COMMON_CFLAGS)
 normal_mod_ASFLAGS = $(COMMON_ASFLAGS)
Quoted 1 line

Index: conf/i386-pc.rmk

I think password should be added as a command (module).

Quoted 15 lines

===================================================================
RCS file: /sources/grub/grub2/normal/command.c,v
retrieving revision 1.19
diff -u -p -r1.19 command.c

command.c +1 -0
@@ -25,6 +25,7 @@
 #include <grub/dl.h>
 #include <grub/parser.h>
 #include <grub/script.h>
+#include <grub/password.h>
 
 static grub_command_t grub_command_list;
 

command.c grub_command_init (void) · line 399

Quoted 8 lines
command.c +4 -0
@@ -391,4 +392,8 @@ grub_command_init (void)
 
   grub_register_command ("lsmod", lsmod_command, GRUB_COMMAND_FLAG_BOTH,
 			 "lsmod", "Show loaded modules.", 0);
+			 
+  grub_register_command ("password", grub_password_command, GRUB_COMMAND_FLAG_MENU,	
+			 "password [--MD5] PASSWORD [NEXT_FILE]", "Set a password", 
+			 grub_password_arg_options);
 }

This belongs in password.c. Just make a module out of it.

Quoted 24 lines

Index: normal/menu.c
===================================================================
RCS file: /sources/grub/grub2/normal/menu.c,v
retrieving revision 1.18
diff -u -p -r1.18 menu.c

menu.c +5 -1
@@ -24,6 +24,9 @@
 #include <grub/machine/time.h>
 #include <grub/env.h>
 #include <grub/script.h>
+#include <grub/password.h>
+
+int ask_and_check_password(void);
 
 static void
 draw_border (void)
@@ -76,9 +79,18 @@ print_message (int nested, int edit)
       grub_printf ("\n\
       Use the %C and %C keys to select which entry is highlighted.\n",
 		   (grub_uint32_t) GRUB_TERM_DISP_UP, (grub_uint32_t) GRUB_TERM_DISP_DOWN);
-      grub_printf ("\
+      if ( grub_password_is_user_authenticated(0) == 1 )
+	{

menu.c print_message (int nested, int edit) · line 93

Quoted 10 lines
menu.c +8 -0
+	  grub_printf ("\
       Press enter to boot the selected OS, \'e\' to edit the\n\
       commands before booting or \'c\' for a command-line.");
+	}
+      else 
+	{
+	  grub_printf ("\
+      Press \'p\' to enter password and unlock command-line and\n\
+      menu entry edition.");
+	}

Please do not make normal.mod depend on password.c. How about adding a function to register authentication functions? In that case this will all take place elsewhere and normal.mod does not have to include all these authentication functions.

So there will be a call back function (or even a list of those) that could be used to authenticate the user.

This will be more flexible and make it possible to use other ways to authenticate. Perhaps some hardware dongle or so?

menu.c run_menu (grub_menu_t menu, int nested) · line 442

Quoted 9 lines
menu.c +8 -0
 	    case 'e':
+	      if ( grub_password_is_user_authenticated(0) != 1 ) 
+		{
+		  if (ask_and_check_password() == 0)
+		    goto proceed_edition;
+		  goto refresh;
+		}
+		
+	    proceed_edition:

If you use (ask_and_check_password() != 0) you can avoid the label.

Quoted 11 lines
menu.c +8 -1
 	      grub_menu_entry_run (get_entry (menu, first + offset));
 	      goto refresh;
-	      
+
+	    case 'p':
+	      /* Authenticate voluntarily and go to next file is any */
+	      if ( grub_password_is_password_set() != 0 ) 
+		{
+		  if (ask_and_check_password() == 0)
+		    {
+		      /* Check if there is a next file specified... */

menu.c run_menu (grub_menu_t menu, int nested) · line 462

Quoted 10 lines
menu.c +10 -0
+		      char *next_file = grub_password_get_next_file();
+		      if (next_file == 0)
+			goto refresh;
+		      
+		      grub_normal_execute( (const char*) next_file, 1);
+		      goto refresh;
+		    }
+		}
+	      goto refresh;
+

How does this work with files and stuff?

Quoted 21 lines
menu.c +21 -0
+int
+ask_and_check_password(void)
+{
+  int ret;
+  int cmdline_ret;
+  char entered[GRUB_PASSWORD_MAX_LENGTH + 1];
+  grub_memset(entered, 0, sizeof(entered));
+
+  /* Clear password zone, and prompt for password*/
+  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
+  grub_printf ("\
+                                                                        ");
+  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
+  cmdline_ret = grub_cmdline_get("   Password : ",
+				      entered,
+				      GRUB_PASSWORD_MAX_LENGTH,
+				      '*',
+				      0);
+  if (cmdline_ret == 0)
+    goto failed;		/* User taped ESC */
+  

menu.c run_menu (grub_menu_t menu, int nested) · line 505

Quoted 10 lines
menu.c +10 -0
+  if (grub_password_check_password(entered, 0) == 0) 
+    goto success;
+  
+ failed:
+  return -1;
+  
+ success:
+  return 0;
+  
+}

Perhaps this belongs in password.c?

--
Marco

#2 lore raw

First of all, thanks for reviewing my code.

I'll try to make a grouped response for most of your points :

- code formatting will be updated to GCS and with your comments
- md5.c/h comes from Grub Legacy, just a bit adapted to use Grub2 memory
functions (malloc, memset, endianness). Therefore, code works correctly, or at least in a coherent way with respect to Grub Legacy. I can try to make the code clearer if needed... Other code is made from scratch.
- limit to password of 64 chars is to have a max buffer size when asking
for the password. It could be greater if required. But will someone really use >64chars plain text password ?
- plain text passwords are indeed very insecure, but I kept them, as it was
possible in Grub legacy. Should I remove them ?
- dependency between normal.mod and password: if I put the
grub_register_command in password.c, then password.mod depends on normal.mod, but normal.mod also depends on password.mod for password checks in menu.c... I did not find a way to work around this. I am however convinced this would be better. The idea of callbacks seems nice... to be further investigated.

I'll first reformat this code and clean it according to these remarks (and others if more come), and submit a new updated patch, along with the "lock" command that I have now implemented.

2007/8/7, Marco Gerards <[email protected]>:

Quoted 675 lines

Julien RANC <[email protected]> writes:

Hi,

Quoted 3 lines

Here is my first patch for Grub2, so I hope it won't be too bad, and by advance, all my apologies for the probable errors ;-) So, please, comment gently...

I'll try... :-)

Quoted 2 lines

These 2 patch add support for the 'password' command, with the syntax I proposed in my mail from August 1st.

:-)

Quoted 8 lines

Just a few remarks:
1. if you use several 'password' commands in the grub.cfg, only the last one will be valid.
2. To use MD5 passwords, you _MUST_ put quotes around the password. For example:
password --md5 '$1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/' --> This works fine password --md5 $1$WAso4$Z93ELTjxbgLaXvhSF/7cZ/ --> no quotes, don't work
3. passwords are limited to 64 chars.

Why this limitation? Well, I think it is not that important :)

Quoted 9 lines

The first patch (patch_1.txt) contains the implementation itself of password management, and MD5 routines (based on the MD5 routines from Grub Legacy)

The second patch (patch_2.txt) contains modifications to the module normal to take into account the new command:
- register the command on startup
- add relevant checks and actions in menu.c
It also patches the .rmk files to compile.

I wouldn't mind if this was one patch.

Quoted 3 lines

I will let you comment on this, and start working on implementing the 'lock' command in the meanwhile. I hope I'll be able to send a first patch for the lock command by the end of the week.

Great!

Can you please submit changelog entries with your patches? That makes a review easier and it it required before committing patches.

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 line 4

Quoted 10 lines

--
Julien RANC
[email protected]

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 renamed from normal/md5.c 1970-01-01 01:00:00.000000000+0100 +4 -0
@@ -0,0 +1,303 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.

This comes from GRUB Legacy?

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 line 11

Quoted 7 lines
normal/md5.c 2007-08-04 12:26:22.000000000 +0200 renamed from normal/md5.c 1970-01-01 01:00:00.000000000+0100 +7 -0
+  /* Round 1 */
+  for (i = 0; i < 16; i++)
+    {
+      tmp = a + F (b, c, d) + grub_le_to_cpu32 (x[i]) + T[i];
+      tmp = ROTATE_LEFT (tmp, s1[i & 3]);
+      tmp += b;
+      a = d; d = c; c = b; b = tmp;

Please use separate lines. Please do the same below.

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 line 16

Quoted 5 lines
normal/md5.c 2007-08-04 12:26:22.000000000 +0200 renamed from normal/md5.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */

This is quite an awkward interface, IMO. You you use both? Can you make two functions out of this easily?

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 line 21

Quoted 5 lines
normal/md5.c 2007-08-04 12:26:22.000000000 +0200 renamed from normal/md5.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+  p = salt + saltlen + 1;
+  for (i = 0; i < 5; i++)
+    {
+      unsigned int w =
+     digest[i == 4 ? 5 : 12+i] | (digest[6+i] << 8) | (digest[i] <<

16);

Please put spaces around the "+". Please put this line on multiple lines. When you use Emacs use "= ( ... );" and let Emacs format these lines for you.

normal/md5.c 2007-08-04 12:26:22.000000000 +0200 line 22

Quoted 1 line
normal/md5.c 2007-08-04 12:26:22.000000000 +0200 renamed from normal/md5.c 1970-01-01 01:00:00.000000000+0100 +1 -0
+      for (n = 4; n-- > 0;)

Are you sure this is right?

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 4

Quoted 7 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +4 -0
@@ -0,0 +1,249 @@
+/* password.c - Password helper function for other modules */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2004,2005,2006  Free Software Foundation, Inc.

Did you use GRUB Legacy code?

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 5

Quoted 1 line
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +1 -0
+ *  This program is free software; you can redistribute it and/or

modify

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 6

Quoted 1 line
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +1 -0
+ *  it under the terms of the GNU General Public License as published

by

Quoted 10 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +10 -0
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 26

Quoted 10 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +10 -0
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/dl.h>
+#include <grub/password.h>
+#include <grub/err.h>                /* for error codes */
+#include <grub/mm.h>         /* for memory mngt */
+#include <grub/misc.h>               /* for string manipulation */
+#include <grub/normal.h>
+#include <grub/md5.h>

For comments, please start with a uppercase letter. End with a '.' followed by two spaces before the "*/". Can you check all your comments for this? This applies in general. I think the comments above a bit useless.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 29

Quoted 3 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +3 -0
+/**
+ * The global password, used as default.
+ */

Please use comments like:
/* The global password, used as default. */

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 30

Quoted 1 line
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +1 -0
+static grub_password_t global_password = 0;

The "= 0" can be left away. This is done by default.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 33

Quoted 3 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +3 -0
+/* Declare private functions */
+int check_plain_text(const char *entered, const char *correct);
+int check_md5(const char *entered, const char *correct);

What is this? If they are private, better leave the comment away and declare them as static. I am actually quite sure most functions in this file should be static.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 37

Quoted 4 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +4 -0
+grub_err_t
+grub_password_command(struct grub_arg_list *state,
+                   int                  argc,
+                   char                 **args)

Please format this according to the GCS. Thus one space between the type and variable name.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 40

Quoted 3 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +3 -0
+{
+  if (argc <= 0)
+    goto fail;                       /* too few arguments */

I prefer comments before the line. Comments on the same line tend to get messy.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 42

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+  if (argc > 2)
+    goto fail;                       /* too many arguments */

Please just use return. Possibly even with a clear error message.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 45

Quoted 3 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +3 -0
+  if (global_password == 0)
+    {
+      global_password = grub_malloc(sizeof(global_password));

Add a space between grub_malloc and '('. Can you please check this for all function calls?

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 50

Quoted 5 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+  /* set the password type */
+  if (state[0].set)
+      global_password->type = GRUB_PASSWORD_TYPE_MD5;
+  else
+      global_password->type = GRUB_PASSWORD_TYPE_PLAIN_TEXT;

Why would we actually allow plain text passwords? Do we really want to allow people to harm themselves? :)

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 58

Quoted 8 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +8 -0
+  global_password->authenticated = 0;
+
+  /* OK, done */
+  return GRUB_ERR_NONE;
+
+
+ fail:
+  return GRUB_ERR_INVALID_COMMAND;

This label is not needed. If this just returns, we can do that instead of jumping here. Never return an error without using "grub_error".

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 62

Quoted 4 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +4 -0
+grub_password_t
+grub_password_create_password(grub_password_type_t type,
+                           const char           *password,
+                           const char           *next_file)

Please fix this formatting. Same for the other functions.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 68

Quoted 6 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +6 -0
+{
+  if ( password == 0 )
+    goto fail;
+
+  if ( next_file == 0 )
+    goto fail;

I would use (! password) or (password == NULL)

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 70

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+  if ( type == GRUB_PASSWORD_TYPE_UNSUPPORTED )
+    goto fail;

How could this happen?

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 76

Quoted 6 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +6 -0
+  grub_password_t passwd;
+  passwd = grub_malloc( sizeof(passwd) );
+  passwd->type = type;
+  passwd->password = grub_strdup(password);
+  passwd->next_file = grub_strdup(next_file);
+  passwd->authenticated = 0;

Please check each of these allocations to see if they succeed. If they don't, please deallocate all before the failing one. This is actually how we usually use "fail:". On error you could use grub_password_free_password, I guess.

normal/password.c 2007-08-05 16:48:31.000000000 +0200 line 78

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+void
+grub_password_free_password(grub_password_t password)

How about just "grub_password_free"?

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 6 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+{
+  if (password == 0)
+    goto finished;
 
+  if (password->password != 0)
+    grub_free(password->password);

No need to check this, grub_free does this for you.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+  if (password->next_file != 0)
+    grub_free(password->next_file);

Same here.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 5 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+int
+grub_password_is_password_set(void)
+{
+  return (global_password != 0);
+}

Can't this be checked by the called instead?

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+int
+grub_password_is_user_authenticated(grub_password_t password)

Please add a space.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 5 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+{
+  grub_password_t passwd;
+  int result;
+
+  if ( password != 0 )

Please use:

if (password != 0)

Can you do this for all the if-statements?

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 8 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +8 -0
+    passwd = password;
+  else
+    passwd = global_password;
+
+  if ( passwd == 0 )
+    {
+      /* No password set : let's say it is OK */
+      result = 1;

Are you sure? Can't this be cought earlier?

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 5 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +5 -0
+    }
+  else
+    {
+      result = passwd->authenticated;
+    }

No need for these parenthesis.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 4 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +4 -0
+  /* If no password set nor passed, OK */
+  if (checked_passwd == 0) {
+    check_result = 0;
+    goto end_check;

Just return 0 here.

Quoted 1 line

+char*

char *

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 13 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +13 -0
+grub_password_get_next_file(void)
+{
+  if (global_password == 0)
+    return 0;
+
+  if (grub_password_is_user_authenticated(0) == 0)
+    return 0;
+
+  if (global_password->next_file == 0)
+    return 0;
+
+  return grub_strdup(global_password->next_file);
+}

Will the callers handle this error? I think it is a bit hard because you also return 0 in other cases, in which there is no grub_errno set.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 2 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +2 -0
+/* Start of private functions */
+

Useless whiteline.

normal/password.c 2007-08-05 16:48:31.000000000 +0200

Quoted 11 lines
normal/password.c 2007-08-05 16:48:31.000000000 +0200 renamed from normal/password.c 1970-01-01 01:00:00.000000000+0100 +11 -0
+int
+check_plain_text(const char *entered,
+              const char *correct)
+{
+  int result;
+  result = grub_strcmp(entered, correct);
+  if (result != 0)
+    result = -1;
+
+  return result;
+}

I think this function is not really needed.

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 4

Quoted 7 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +4 -0
@@ -0,0 +1,30 @@
+/* md5.c - an implementation of the MD5 algorithm and MD5 crypt */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005,2006,2007  Free Software Foundation, Inc.

What is the origen of this file?

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 6

Quoted 2 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +2 -0
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published

by

Quoted 9 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +9 -0
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 25

Quoted 10 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +10 -0
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+/* If CHECK is true, check a password for correctness. Returns 0
+   if password was correct, and a value != 0 for error, similarly
+   to strcmp.
+   If CHECK is false, crypt KEY and save the result in CRYPTED.
+   CRYPTED must have a salt.  */
+extern int grub_md5_password (const char *key, char *crypted, int

check);

No need for the extern keyword, I think this is even wrong.

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 27

Quoted 2 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +2 -0
+/* For convenience.  */
+#define grub_md5_check_password(key,crypted) grub_md5_password((key),

(crypted), 1)

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 28

Quoted 1 line
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +1 -0
+#define grub_md5_make_password(key,crypted)  grub_md5_password((key),

(crypted), 0)

It would be better to actually make these functions.

Can you please add an include guard?

Quoted 1 line

--- vanilla_grub2/include/grub/password.h 1970-01-01 01:00:

00.000000000 +0100

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 4

Quoted 6 lines

+++ grub2/include/grub/password.h 2007-08-05 16:49:06.000000000+0200

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +4 -0
@@ -0,0 +1,147 @@
+/* password.h - Header file for password management */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005  Free Software Foundation, Inc.

Where was this based on? Shouldn't 2007 be added?

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 6

Quoted 2 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +2 -0
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published

by

Quoted 11 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +11 -0
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 27

Quoted 10 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +10 -0
+ */
+
+#ifndef GRUB_PASSWORD_H
+#define GRUB_PASSWORD_H 1
+
+
+#include <grub/arg.h>
+
+/* The max length of the password, in chars */
+#define GRUB_PASSWORD_MAX_LENGTH 64

Is a maximum really required?

Quoted 7 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +7 -0
+/* the password types */
+enum grub_password_type
+  {
+    GRUB_PASSWORD_TYPE_UNSUPPORTED,
+    GRUB_PASSWORD_TYPE_PLAIN_TEXT,
+    GRUB_PASSWORD_TYPE_MD5
+  };

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 44

Quoted 10 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +10 -0
+typedef enum grub_password_type grub_password_type_t;
+
+
+/* structure holding a password */
+struct grub_password
+{
+  char *password;            /* The password.  */
+  grub_password_type_t type; /* The type.  */
+  char *next_file;           /* The file to jump after auth */
+  int  authenticated;                /* Flag for passwd verification

status */

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 51

Quoted 7 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +7 -0
+};
+typedef struct grub_password *grub_password_t;
+
+/* The options available for the 'password' command */
+static const struct grub_arg_option grub_password_arg_options[] =
+  {
+    {"md5", 'm', GRUB_ARG_OPTION_OPTIONAL, "indicates the password is

encrypted with MD5", 0, ARG_TYPE_NONE},

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 53

Quoted 2 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +2 -0
+    {0, 0, 0, 0, 0, 0}
+  };

This doesn't belong here and should go into password.c. Please have a look at hello/hello.c how to deal with this.

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 62

Quoted 9 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +9 -0
+/* Helper functions */
+
+/**
+ * @brief The 'password' command itself
+ *
+ * This is the callback to execute a 'password' command from a
+ * script.
+ * See normal.h for information on the parameters
+  */

We do not use doxygen or so. Please use these '*'s but try to match the commenting style used in GRUB 2.

Quoted 7 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +7 -0
+grub_err_t
+grub_password_command(struct grub_arg_list *state,
+                   int                  argc,
+                   char                 **args);
+
+/**
+ * @bried Create a grub_password_t structure based on entered params

include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 line 79

Quoted 10 lines
include/grub/md5.h 2007-08-04 12:44:46.000000000 +0200 renamed from include/grub/md5.h 1970-01-01 01:00:00.000000000+0100 +10 -0
+ *
+ * This function is helpful for the 'lock' command to generate the
+ * password against which the user password will be checked.
+ *
+ * @param[in] type the password type
+ * @param[in] password the password
+ *
+ * @returns a grub_password_t object properly filled
+ * @returns 0 is invalid params are passed
+ */

Same here.

I think this function should be static so it should not be included in the header file. I think this is also the case for some other functions.

Quoted 15 lines

Index: conf/i386-efi.rmk
===================================================================
RCS file: /sources/grub/grub2/conf/i386-efi.rmk,v
retrieving revision 1.19
diff -u -p -r1.19 i386-efi.rmk

i386-efi.rmk 5 Aug 2007 15:18:51 -0000 renamed from i386-efi.rmk 2 Aug 2007 19:12:52 -0000 1.19 +1 -0
@@ -109,6 +109,7 @@ normal_mod_SOURCES = normal/arg.c normal
      normal/completion.c normal/execute.c            \
      normal/function.c normal/lexer.c normal/main.c normal/menu.c    \
      normal/menu_entry.c normal/misc.c grub_script.tab.c             \
+     normal/password.c normal/md5.c                                  \
      normal/script.c normal/i386/setjmp.S
 normal_mod_CFLAGS = $(COMMON_CFLAGS)
 normal_mod_ASFLAGS = $(COMMON_ASFLAGS)
Quoted 1 line

Index: conf/i386-pc.rmk

I think password should be added as a command (module).

Quoted 15 lines

===================================================================
RCS file: /sources/grub/grub2/normal/command.c,v
retrieving revision 1.19
diff -u -p -r1.19 command.c

command.c 5 Aug 2007 15:18:52 -0000 renamed from command.c 21 Jul 2007 23:32:29 -0000 1.19 +1 -0
@@ -25,6 +25,7 @@
 #include <grub/dl.h>
 #include <grub/parser.h>
 #include <grub/script.h>
+#include <grub/password.h>
 
 static grub_command_t grub_command_list;
 

command.c 5 Aug 2007 15:18:52 -0000 grub_command_init (void) · line 393

Quoted 2 lines
command.c 5 Aug 2007 15:18:52 -0000 renamed from command.c 21 Jul 2007 23:32:29 -0000 1.19 +0 -0
@@ -391,4 +392,8 @@ grub_command_init (void)
 
   grub_register_command ("lsmod", lsmod_command,

GRUB_COMMAND_FLAG_BOTH,

command.c 5 Aug 2007 15:18:52 -0000 grub_command_init (void) · line 396

Quoted 3 lines
command.c 5 Aug 2007 15:18:52 -0000 renamed from command.c 21 Jul 2007 23:32:29 -0000 1.19 +2 -0
                       "lsmod", "Show loaded modules.", 0);
+
+  grub_register_command ("password", grub_password_command,

GRUB_COMMAND_FLAG_MENU,

command.c 5 Aug 2007 15:18:52 -0000 grub_command_init (void) · line 397

Quoted 1 line
command.c 5 Aug 2007 15:18:52 -0000 renamed from command.c 21 Jul 2007 23:32:29 -0000 1.19 +1 -0
+                      "password [--MD5] PASSWORD [NEXT_FILE]", "Set a

password",

command.c 5 Aug 2007 15:18:52 -0000 grub_command_init (void) · line 399

Quoted 2 lines
command.c 5 Aug 2007 15:18:52 -0000 renamed from command.c 21 Jul 2007 23:32:29 -0000 1.19 +1 -0
+                      grub_password_arg_options);
 }

This belongs in password.c. Just make a module out of it.

Quoted 18 lines

Index: normal/menu.c
===================================================================
RCS file: /sources/grub/grub2/normal/menu.c,v
retrieving revision 1.18
diff -u -p -r1.18 menu.c

menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +3 -0
@@ -24,6 +24,9 @@
 #include <grub/machine/time.h>
 #include <grub/env.h>
 #include <grub/script.h>
+#include <grub/password.h>
+
+int ask_and_check_password(void);
 
 static void
 draw_border (void)

menu.c 5 Aug 2007 15:18:52 -0000 print_message (int nested, int edit) · line 81

Quoted 3 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +0 -0
@@ -76,9 +79,18 @@ print_message (int nested, int edit)
       grub_printf ("\n\
       Use the %C and %C keys to select which entry is highlighted.\n",
                 (grub_uint32_t) GRUB_TERM_DISP_UP, (grub_uint32_t)

GRUB_TERM_DISP_DOWN);

menu.c 5 Aug 2007 15:18:52 -0000 print_message (int nested, int edit) · line 93

Quoted 13 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +10 -1
-      grub_printf ("\
+      if ( grub_password_is_user_authenticated(0) == 1 )
+     {
+       grub_printf ("\
       Press enter to boot the selected OS, \'e\' to edit the\n\
       commands before booting or \'c\' for a command-line.");
+     }
+      else
+     {
+       grub_printf ("\
+      Press \'p\' to enter password and unlock command-line and\n\
+      menu entry edition.");
+     }

Please do not make normal.mod depend on password.c. How about adding a function to register authentication functions? In that case this will all take place elsewhere and normal.mod does not have to include all these authentication functions.

So there will be a call back function (or even a list of those) that could be used to authenticate the user.

This will be more flexible and make it possible to use other ways to authenticate. Perhaps some hardware dongle or so?

menu.c 5 Aug 2007 15:18:52 -0000 print_message (int nested, int edit)

Quoted 9 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +8 -0
          case 'e':
+           if ( grub_password_is_user_authenticated(0) != 1 )
+             {
+               if (ask_and_check_password() == 0)
+                 goto proceed_edition;
+               goto refresh;
+             }
+
+         proceed_edition:

If you use (ask_and_check_password() != 0) you can avoid the label.

Quoted 11 lines
            grub_menu_entry_run (get_entry (menu, first + offset));
            goto refresh;
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +8 -1
-
+
+         case 'p':
+           /* Authenticate voluntarily and go to next file is any */
+           if ( grub_password_is_password_set() != 0 )
+             {
+               if (ask_and_check_password() == 0)
+                 {
+                   /* Check if there is a next file specified... */

menu.c 5 Aug 2007 15:18:52 -0000

Quoted 10 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +10 -0
+                   char *next_file = grub_password_get_next_file();
+                   if (next_file == 0)
+                     goto refresh;
+
+                   grub_normal_execute( (const char*) next_file, 1);
+                   goto refresh;
+                 }
+             }
+           goto refresh;
+

How does this work with files and stuff?

menu.c 5 Aug 2007 15:18:52 -0000

Quoted 12 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +11 -0
+int
+ask_and_check_password(void)
+{
+  int ret;
+  int cmdline_ret;
+  char entered[GRUB_PASSWORD_MAX_LENGTH + 1];
+  grub_memset(entered, 0, sizeof(entered));
+
+  /* Clear password zone, and prompt for password*/
+  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
+  grub_printf ("\

+ ");

Quoted 9 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +9 -0
+  grub_gotoxy (0, GRUB_TERM_HEIGHT - 3);
+  cmdline_ret = grub_cmdline_get("   Password : ",
+                                   entered,
+                                   GRUB_PASSWORD_MAX_LENGTH,
+                                   '*',
+                                   0);
+  if (cmdline_ret == 0)
+    goto failed;             /* User taped ESC */
+

menu.c 5 Aug 2007 15:18:52 -0000

Quoted 10 lines
menu.c 5 Aug 2007 15:18:52 -0000 renamed from menu.c 21 Jul 2007 23:32:29 -0000 1.18 +10 -0
+  if (grub_password_check_password(entered, 0) == 0)
+    goto success;
+
+ failed:
+  return -1;
+
+ success:
+  return 0;
+
+}

Perhaps this belongs in password.c?

--
Marco

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 2 lines
Julien RANC
[email protected]
#3 lore raw
Quoted 2 lines · Julien Ranc

- plain text passwords are indeed very insecure, but I kept them, as it was
possible in Grub legacy. Should I remove them ?

I think there's plenty of people who will have use for plain, insecure passwords.

The first security problem of having access to the grub menu is that in a lot of cases, it is equal to having access to the hardware. That blows up pretty much all of your security measures, if you're not using encrypted filesystems or whatever.

Plain password is easy to beat, but at least it adds a minimal layer of "annoyance" for anyone wanting to boot what they aren't supposed to boot.

Signature · 3 lines
Jordi Mallach Pérez  --  Debian developer     http://www.debian.org/
[email protected]     [email protected]     http://www.sindominio.net/
GnuPG public key information available at http://oskuro.net/
#5 lore raw
In reply to Julien Ranc

Hello,

Quoted 4 lines

- md5.c/h comes from Grub Legacy, just a bit adapted to use Grub2 memory
functions (malloc, memset, endianness). Therefore, code works correctly, or at least in a coherent way with respect to Grub Legacy. I can try to make the code clearer if needed... Other code is made from scratch.

IMO, we should stop using MD5 for the encryption, because it is nowadays considered to be not secure enough. At least SHA1 should be used, although even SHA1 is considered to be vulnerable in near future... You can take an implementation from gnulib.

Quoted 3 lines

- limit to password of 64 chars is to have a max buffer size when asking
for the password. It could be greater if required. But will someone really use >64chars plain text password ?

I don't think so. For me, this is an acceptable limitation.

Quoted 2 lines

- plain text passwords are indeed very insecure, but I kept them, as it
was possible in Grub legacy. Should I remove them ?

Yes. Basically we don't need to keep compatibilty with GRUB Legacy, thus insecure things should be abandoned.

Thanks,
Okuji

#4 lore raw
In reply to Jordi Mallach
Quoted 7 lines · Jordi Mallach
Quoted 2 lines · Julien Ranc

- plain text passwords are indeed very insecure, but I kept them, as it was
possible in Grub legacy. Should I remove them ?

I think there's plenty of people who will have use for plain, insecure passwords.

Quoted 8 lines

The first security problem of having access to the grub menu is that in a lot of cases, it is equal to having access to the hardware. That blows up pretty much all of your security measures, if you're not using encrypted filesystems or whatever.

Plain password is easy to beat, but at least it adds a minimal layer of "annoyance" for anyone wanting to boot what they aren't supposed to boot.

So you want to make it possible to have plain text passwords because it is easier to hack? :-)

--
Marco