All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] password command implementation
@ 2007-08-05 18:38 Julien RANC
  2007-08-07 10:12 ` Marco Gerards
  0 siblings, 1 reply; 6+ messages in thread
From: Julien RANC @ 2007-08-05 18:38 UTC (permalink / raw)
  To: grub-devel, julien.ranc


[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]

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.

-- 
Julien RANC
julien.ranc@gmail.com

[-- Attachment #1.2: patch_1.txt --]
[-- Type: text/plain, Size: 19980 bytes --]

--- vanilla_grub2/normal/md5.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2/normal/md5.c	2007-08-04 12:26:22.000000000 +0200
@@ -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;
+}
+
+
--- vanilla_grub2/normal/password.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2/normal/password.c	2007-08-05 16:48:31.000000000 +0200
@@ -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.
+ *
+ *  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
+ *  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>
+
+/**
+ * The global password, used as default.
+ */
+static grub_password_t global_password = 0;
+
+
+/* Declare private functions */
+int check_plain_text(const char *entered, const char *correct);
+int check_md5(const char *entered, const char *correct);
+
+grub_err_t
+grub_password_command(struct grub_arg_list *state,
+		      int                  argc,
+		      char                 **args)
+{
+  if (argc <= 0)
+    goto fail;			/* too few arguments */
+  
+  if (argc > 2)
+    goto fail;			/* too many arguments */
+
+  if (global_password == 0) 
+    {
+      global_password = grub_malloc(sizeof(global_password));
+      global_password->password = 0;
+      global_password->type = GRUB_PASSWORD_TYPE_UNSUPPORTED;
+      global_password->next_file = 0;
+      global_password->authenticated = 0;
+    }
+
+  /* Set the new password */
+  if ( global_password->password != 0) 
+      grub_free(global_password->password);
+  global_password->password = grub_strdup(args[0]);
+
+  /* set the next file to load */
+  if ( global_password->next_file != 0) 
+      grub_free(global_password->next_file);
+  if ( argc > 1) 
+      global_password->next_file = grub_strdup(args[1]);
+  else 
+      global_password->next_file = 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;
+
+  global_password->authenticated = 0;
+
+  /* OK, done */
+  return GRUB_ERR_NONE;
+
+  
+ fail:
+  return GRUB_ERR_INVALID_COMMAND;
+}
+  
+
+
+grub_password_t 
+grub_password_create_password(grub_password_type_t type, 
+			      const char           *password,
+			      const char           *next_file)
+{
+  if ( password == 0 )
+    goto fail;
+
+  if ( next_file == 0 )
+    goto fail;
+
+  if ( type == GRUB_PASSWORD_TYPE_UNSUPPORTED )
+    goto fail;
+  
+  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;
+
+  return passwd;
+
+ fail:
+  return 0;
+}
+
+void 
+grub_password_free_password(grub_password_t password)
+{
+  if (password == 0)
+    goto finished;
+
+  if (password->password != 0)
+    grub_free(password->password);
+
+  if (password->next_file != 0)
+    grub_free(password->next_file);
+
+  grub_free(password);
+
+ finished:
+  return;
+}
+
+int 
+grub_password_is_password_set(void)
+{
+  return (global_password != 0);
+}
+
+int 
+grub_password_is_user_authenticated(grub_password_t password)
+{
+  grub_password_t passwd;
+  int result;
+
+  if ( password != 0 )
+    passwd = password;
+  else
+    passwd = global_password;
+
+  if ( passwd == 0 )
+    {
+      /* No password set : let's say it is OK */
+      result = 1;
+    }
+  else 
+    {
+      result = passwd->authenticated;
+    }
+
+  return result;
+}
+
+int 
+grub_password_check_password(const char      *entered, 
+			     grub_password_t password) 
+{
+  int check_result = -1;
+  grub_password_t checked_passwd;
+
+  /* Sets the password we check against */
+  if ( password != 0 ) 
+      checked_passwd = password;
+  else 
+      checked_passwd = global_password;
+
+  /* If no password set nor passed, OK */
+  if (checked_passwd == 0) {
+    check_result = 0;
+    goto end_check;
+  }
+
+  /* Perform actual check */
+  switch ( checked_passwd->type ) 
+    {
+    case (GRUB_PASSWORD_TYPE_PLAIN_TEXT):
+      check_result = check_plain_text(entered, 
+				      checked_passwd->password);
+      break;
+
+    case (GRUB_PASSWORD_TYPE_MD5):
+      check_result = check_md5(entered,
+			       checked_passwd->password);
+      break;
+
+    default:
+      /* Unknown password type, or unsupported */
+      check_result = -1;
+    }
+
+  if (check_result == 0)
+    checked_passwd->authenticated = 1;
+
+
+ end_check:
+  return check_result;
+}
+
+char*
+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);
+}
+
+/* Start of private functions */
+
+int
+check_plain_text(const char *entered,
+		 const char *correct)
+{
+  int result;
+  result = grub_strcmp(entered, correct);
+  if (result != 0)
+    result = -1;
+
+  return result;
+}
+
+int
+check_md5(const char *entered,
+	  const char *correct)
+{
+  int result;
+  result = grub_md5_check_password(entered, correct);
+  return result;
+}
+
+
+
--- vanilla_grub2/include/grub/md5.h	1970-01-01 01:00:00.000000000 +0100
+++ grub2/include/grub/md5.h	2007-08-04 12:44:46.000000000 +0200
@@ -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)
--- vanilla_grub2/include/grub/password.h	1970-01-01 01:00:00.000000000 +0100
+++ grub2/include/grub/password.h	2007-08-05 16:49:06.000000000 +0200
@@ -0,0 +1,147 @@
+/* password.h - Header file for password management */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2003,2005  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 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.
+ */
+ 
+#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
+
+/* 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 */
+  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}
+  };
+
+/* 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
+  */
+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
+ *
+ * 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
+ */
+grub_password_t 
+grub_password_create_password(grub_password_type_t type, 
+			      const char           *password,
+			      const char           *next_file);
+
+/**
+ * @bried Free a grub_password_t structure
+ *
+ * @param[in] password the password
+ */
+void
+grub_password_free_password(grub_password_t password); 
+
+
+/**
+ * @brief Check is a global password is set
+ *
+ * @returns 0 if no password is set
+ * @returns 1 if a password is set
+ */
+int 
+grub_password_is_password_set(void);
+
+/**
+ * @brief Check if the user has correctly authentificated with a given password
+ *
+ * @param[in] password : the password to check. If set to 0, the global
+ * password is used instead.
+ *
+ * @returns 0 if user is not authenticated
+ * @returns 1 if user is authenticated
+ * @returns 1 if no password passed and no global password is set
+ */
+int 
+grub_password_is_user_authenticated(grub_password_t password);
+
+/**
+ * @brief Check a user password
+ * 
+ * @param[in] entered the user entered password
+ * @param[in] password the password agains which the user input
+ * will be checked. If set to 0, the global password, if set, will
+ * be used.
+ *
+ * @returns 0 if the user entered password is correct, or if no password
+ * had been set AND no password had been specified.
+ * @returns -1 if the user password is wrong.
+ */
+int 
+grub_password_check_password(const char      *entered, 
+			     grub_password_t password);
+
+
+/**
+ * @brief get the next file specified with the global password
+ *
+ * @returns the next file to jump to. The buffer needs to be freed
+ * after use...
+ * @returns 0 if user is not authenticated, or no file given
+ */
+char*
+grub_password_get_next_file(void);
+
+#endif /* !GRUB_PASSWORD_H  */
+	

[-- Attachment #1.3: patch_2.txt --]
[-- Type: text/plain, Size: 6836 bytes --]

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
--- conf/i386-efi.rmk	2 Aug 2007 19:12:52 -0000	1.19
+++ conf/i386-efi.rmk	5 Aug 2007 15:18:51 -0000
@@ -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
diff -u -p -r1.86 i386-pc.rmk
--- conf/i386-pc.rmk	2 Aug 2007 19:12:52 -0000	1.86
+++ conf/i386-pc.rmk	5 Aug 2007 15:18:52 -0000
@@ -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
diff -u -p -r1.69 powerpc-ieee1275.rmk
--- conf/powerpc-ieee1275.rmk	2 Aug 2007 19:12:52 -0000	1.69
+++ conf/powerpc-ieee1275.rmk	5 Aug 2007 15:18:52 -0000
@@ -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
diff -u -p -r1.17 sparc64-ieee1275.rmk
--- conf/sparc64-ieee1275.rmk	2 Jul 2007 20:38:01 -0000	1.17
+++ conf/sparc64-ieee1275.rmk	5 Aug 2007 15:18:52 -0000
@@ -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
diff -u -p -r1.19 command.c
--- normal/command.c	21 Jul 2007 23:32:29 -0000	1.19
+++ normal/command.c	5 Aug 2007 15:18:52 -0000
@@ -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
diff -u -p -r1.18 menu.c
--- normal/menu.c	21 Jul 2007 23:32:29 -0000	1.18
+++ normal/menu.c	5 Aug 2007 15:18:52 -0000
@@ -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 )
+	{
+	  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.");
+	}
       if (nested)
 	grub_printf ("\n\
       ESC to return previous menu.");
@@ -408,13 +420,46 @@ run_menu (grub_menu_t menu, int nested)
 	      break;
 	      
 	    case 'c':
+	      if ( grub_password_is_user_authenticated(0) != 1 ) 
+		{
+		  if (ask_and_check_password() == 0)
+		    goto proceed_cmdline;
+		  goto refresh;
+		}
+	      
+	    proceed_cmdline:
 	      grub_cmdline_run (1);
 	      goto refresh;
 
 	    case 'e':
+	      if ( grub_password_is_user_authenticated(0) != 1 ) 
+		{
+		  if (ask_and_check_password() == 0)
+		    goto proceed_edition;
+		  goto refresh;
+		}
+		
+	    proceed_edition:
 	      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... */
+		      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;
+
 	    default:
 	      break;
 	    }
@@ -427,6 +472,38 @@ run_menu (grub_menu_t menu, int nested)
   return -1;
 }
 
+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 */
+  
+  if (grub_password_check_password(entered, 0) == 0) 
+    goto success;
+  
+ failed:
+  return -1;
+  
+ success:
+  return 0;
+  
+}
+
 /* Run a menu entry.  */
 static void
 run_menu_entry (grub_menu_entry_t entry)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3112 bytes --]

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

* Re: [PATCH] password command implementation
  2007-08-05 18:38 [PATCH] password command implementation Julien RANC
@ 2007-08-07 10:12 ` Marco Gerards
  2007-08-07 12:17   ` Julien Ranc
  0 siblings, 1 reply; 6+ messages in thread
From: Marco Gerards @ 2007-08-07 10:12 UTC (permalink / raw)
  To: The development of GRUB 2

Julien RANC <julien.ranc@gmail.com> writes:

Hi,

> 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... :-)

> 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.

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

> 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.

> 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.

> -- 
> Julien RANC
> julien.ranc@gmail.com
> --- vanilla_grub2/normal/md5.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/normal/md5.c	2007-08-04 12:26:22.000000000 +0200
> @@ -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?

> +  /* 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.

> +/* 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?

> +  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.

> +      for (n = 4; n-- > 0;)

Are you sure this is right?

> --- vanilla_grub2/normal/password.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/normal/password.c	2007-08-05 16:48:31.000000000 +0200
> @@ -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?

> + *  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
> + *  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.

> +/**
> + * The global password, used as default.
> + */

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

> +static grub_password_t global_password = 0;

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

> +/* 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.

> +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.

> +{
> +  if (argc <= 0)
> +    goto fail;			/* too few arguments */

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

> +  if (argc > 2)
> +    goto fail;			/* too many arguments */

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

> +  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?

> +  /* 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? :)

> +  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".

> +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.

> +{
> +  if ( password == 0 )
> +    goto fail;
> +
> +  if ( next_file == 0 )
> +    goto fail;

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

> +  if ( type == GRUB_PASSWORD_TYPE_UNSUPPORTED )
> +    goto fail;

How could this happen?

> +  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.

> +void 
> +grub_password_free_password(grub_password_t password)

How about just "grub_password_free"?

> +{
> +  if (password == 0)
> +    goto finished;

> +  if (password->password != 0)
> +    grub_free(password->password);

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

> +  if (password->next_file != 0)
> +    grub_free(password->next_file);

Same here.

> +int 
> +grub_password_is_password_set(void)
> +{
> +  return (global_password != 0);
> +}

Can't this be checked by the called instead?

> +int 
> +grub_password_is_user_authenticated(grub_password_t password)

Please add a space.

> +{
> +  grub_password_t passwd;
> +  int result;
> +
> +  if ( password != 0 )

Please use:

if (password != 0)

Can you do this for all the if-statements?

> +    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?

> +    }
> +  else 
> +    {
> +      result = passwd->authenticated;
> +    }

No need for these parenthesis.

> +  /* If no password set nor passed, OK */
> +  if (checked_passwd == 0) {
> +    check_result = 0;
> +    goto end_check;

Just return 0 here.

> +char*

char *

> +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.

> +/* Start of private functions */
> +

Useless whiteline.

> +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.

> --- vanilla_grub2/include/grub/md5.h	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/include/grub/md5.h	2007-08-04 12:44:46.000000000 +0200
> @@ -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?

> + *  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);

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

> +/* 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?

> --- vanilla_grub2/include/grub/password.h	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/include/grub/password.h	2007-08-05 16:49:06.000000000 +0200
> @@ -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?

> + *  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.
> + */
> + 
> +#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?

> +/* 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 */
> +  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.

> +/* 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.

> +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
> + *
> + * 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.

> 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
> --- conf/i386-efi.rmk	2 Aug 2007 19:12:52 -0000	1.19
> +++ conf/i386-efi.rmk	5 Aug 2007 15:18:51 -0000
> @@ -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

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

> ===================================================================
> RCS file: /sources/grub/grub2/normal/command.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 command.c
> --- normal/command.c	21 Jul 2007 23:32:29 -0000	1.19
> +++ normal/command.c	5 Aug 2007 15:18:52 -0000
> @@ -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);
>  }

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

> Index: normal/menu.c
> ===================================================================
> RCS file: /sources/grub/grub2/normal/menu.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 menu.c
> --- normal/menu.c	21 Jul 2007 23:32:29 -0000	1.18
> +++ normal/menu.c	5 Aug 2007 15:18:52 -0000
> @@ -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 )
> +	{
> +	  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?

>  	    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.

>  	      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... */
> +		      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?

> +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 */
> +  
> +  if (grub_password_check_password(entered, 0) == 0) 
> +    goto success;
> +  
> + failed:
> +  return -1;
> +  
> + success:
> +  return 0;
> +  
> +}

Perhaps this belongs in password.c?

--
Marco




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

* Re: [PATCH] password command implementation
  2007-08-07 10:12 ` Marco Gerards
@ 2007-08-07 12:17   ` Julien Ranc
  2007-08-07 12:45     ` Jordi Mallach
  2007-08-07 19:04     ` Yoshinori K. Okuji
  0 siblings, 2 replies; 6+ messages in thread
From: Julien Ranc @ 2007-08-07 12:17 UTC (permalink / raw)
  To: The development of GRUB 2

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

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 <mgerards@xs4all.nl>:
>
> Julien RANC <julien.ranc@gmail.com> writes:
>
> Hi,
>
> > 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... :-)
>
> > 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.
>
> Why this limitation?  Well, I think it is not that important :)
>
> > 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.
>
> > 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.
>
> > --
> > Julien RANC
> > julien.ranc@gmail.com
> > --- vanilla_grub2/normal/md5.c        1970-01-01 01:00:00.000000000+0100
> > +++ grub2/normal/md5.c        2007-08-04 12:26:22.000000000 +0200
> > @@ -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?
>
> > +  /* 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.
>
> > +/* 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?
>
> > +  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.
>
> > +      for (n = 4; n-- > 0;)
>
> Are you sure this is right?
>
> > --- vanilla_grub2/normal/password.c   1970-01-01 01:00:00.000000000+0100
> > +++ grub2/normal/password.c   2007-08-05 16:48:31.000000000 +0200
> > @@ -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?
>
> > + *  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
> > + *  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.
>
> > +/**
> > + * The global password, used as default.
> > + */
>
> Please use comments like:
> /* The global password, used as default.  */
>
> > +static grub_password_t global_password = 0;
>
> The "= 0" can be left away.  This is done by default.
>
> > +/* 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.
>
> > +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.
>
> > +{
> > +  if (argc <= 0)
> > +    goto fail;                       /* too few arguments */
>
> I prefer comments before the line.  Comments on the same line tend to
> get messy.
>
> > +  if (argc > 2)
> > +    goto fail;                       /* too many arguments */
>
> Please just use return.  Possibly even with a clear error message.
>
> > +  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?
>
> > +  /* 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? :)
>
> > +  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".
>
> > +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.
>
> > +{
> > +  if ( password == 0 )
> > +    goto fail;
> > +
> > +  if ( next_file == 0 )
> > +    goto fail;
>
> I would use (! password) or (password == NULL)
>
> > +  if ( type == GRUB_PASSWORD_TYPE_UNSUPPORTED )
> > +    goto fail;
>
> How could this happen?
>
> > +  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.
>
> > +void
> > +grub_password_free_password(grub_password_t password)
>
> How about just "grub_password_free"?
>
> > +{
> > +  if (password == 0)
> > +    goto finished;
>
> > +  if (password->password != 0)
> > +    grub_free(password->password);
>
> No need to check this, grub_free does this for you.
>
> > +  if (password->next_file != 0)
> > +    grub_free(password->next_file);
>
> Same here.
>
> > +int
> > +grub_password_is_password_set(void)
> > +{
> > +  return (global_password != 0);
> > +}
>
> Can't this be checked by the called instead?
>
> > +int
> > +grub_password_is_user_authenticated(grub_password_t password)
>
> Please add a space.
>
> > +{
> > +  grub_password_t passwd;
> > +  int result;
> > +
> > +  if ( password != 0 )
>
> Please use:
>
> if (password != 0)
>
> Can you do this for all the if-statements?
>
> > +    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?
>
> > +    }
> > +  else
> > +    {
> > +      result = passwd->authenticated;
> > +    }
>
> No need for these parenthesis.
>
> > +  /* If no password set nor passed, OK */
> > +  if (checked_passwd == 0) {
> > +    check_result = 0;
> > +    goto end_check;
>
> Just return 0 here.
>
> > +char*
>
> char *
>
> > +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.
>
> > +/* Start of private functions */
> > +
>
> Useless whiteline.
>
> > +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.
>
> > --- vanilla_grub2/include/grub/md5.h  1970-01-01 01:00:00.000000000+0100
> > +++ grub2/include/grub/md5.h  2007-08-04 12:44:46.000000000 +0200
> > @@ -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?
>
> > + *  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);
>
> No need for the extern keyword, I think this is even wrong.
>
> > +/* 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?
>
> > --- vanilla_grub2/include/grub/password.h     1970-01-01 01:00:
> 00.000000000 +0100
> > +++ grub2/include/grub/password.h     2007-08-05 16:49:06.000000000+0200
> > @@ -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?
>
> > + *  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.
> > + */
> > +
> > +#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?
>
> > +/* 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 */
> > +  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.
>
> > +/* 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.
>
> > +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
> > + *
> > + * 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.
>
> > 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
> > --- conf/i386-efi.rmk 2 Aug 2007 19:12:52 -0000       1.19
> > +++ conf/i386-efi.rmk 5 Aug 2007 15:18:51 -0000
> > @@ -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
>
> I think password should be added as a command (module).
>
> > ===================================================================
> > RCS file: /sources/grub/grub2/normal/command.c,v
> > retrieving revision 1.19
> > diff -u -p -r1.19 command.c
> > --- normal/command.c  21 Jul 2007 23:32:29 -0000      1.19
> > +++ normal/command.c  5 Aug 2007 15:18:52 -0000
> > @@ -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);
> >  }
>
> This belongs in password.c.  Just make a module out of it.
>
> > Index: normal/menu.c
> > ===================================================================
> > RCS file: /sources/grub/grub2/normal/menu.c,v
> > retrieving revision 1.18
> > diff -u -p -r1.18 menu.c
> > --- normal/menu.c     21 Jul 2007 23:32:29 -0000      1.18
> > +++ normal/menu.c     5 Aug 2007 15:18:52 -0000
> > @@ -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 )
> > +     {
> > +       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?
>
> >           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.
>
> >             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... */
> > +                   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?
>
> > +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 */
> > +
> > +  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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Julien RANC
julien.ranc@gmail.com

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

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

* Re: [PATCH] password command implementation
  2007-08-07 12:17   ` Julien Ranc
@ 2007-08-07 12:45     ` Jordi Mallach
  2007-08-08 14:57       ` Marco Gerards
  2007-08-07 19:04     ` Yoshinori K. Okuji
  1 sibling, 1 reply; 6+ messages in thread
From: Jordi Mallach @ 2007-08-07 12:45 UTC (permalink / raw)
  To: grub-devel

On Tue, Aug 07, 2007 at 02:17:16PM +0200, Julien Ranc wrote:
>  - 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.

-- 
Jordi Mallach Pérez  --  Debian developer     http://www.debian.org/
jordi@sindominio.net     jordi@debian.org     http://www.sindominio.net/
GnuPG public key information available at http://oskuro.net/



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

* Re: [PATCH] password command implementation
  2007-08-07 12:17   ` Julien Ranc
  2007-08-07 12:45     ` Jordi Mallach
@ 2007-08-07 19:04     ` Yoshinori K. Okuji
  1 sibling, 0 replies; 6+ messages in thread
From: Yoshinori K. Okuji @ 2007-08-07 19:04 UTC (permalink / raw)
  To: The development of GRUB 2

Hello,

>  - 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.

>  - 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.

>  - 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



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

* Re: [PATCH] password command implementation
  2007-08-07 12:45     ` Jordi Mallach
@ 2007-08-08 14:57       ` Marco Gerards
  0 siblings, 0 replies; 6+ messages in thread
From: Marco Gerards @ 2007-08-08 14:57 UTC (permalink / raw)
  To: The development of GRUB 2

Jordi Mallach <jordi@gnu.org> writes:

> On Tue, Aug 07, 2007 at 02:17:16PM +0200, Julien Ranc wrote:
>>  - 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.

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

--
Marco




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

end of thread, other threads:[~2007-08-08 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-05 18:38 [PATCH] password command implementation Julien RANC
2007-08-07 10:12 ` Marco Gerards
2007-08-07 12:17   ` Julien Ranc
2007-08-07 12:45     ` Jordi Mallach
2007-08-08 14:57       ` Marco Gerards
2007-08-07 19:04     ` Yoshinori K. Okuji

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.