password command implementation
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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 inmenu.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.rmki386-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.19command.ccommand.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.18menu.cmenu.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
-0case '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
-1grub_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
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 thegrub_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 inmenu.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/ormodify
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 publishedby
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 Softwarenormal/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_mallocand '('. 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_freedoes 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_errnoset.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 publishedby
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 Licenseinclude/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, intcheck);
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.h1970-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.h2007-08-05 16:49:06.000000000+0200include/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 publishedby
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 64Is 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 verificationstatus */
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 isencrypted 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 athello/hello.chow 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 paramsinclude/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.rmki386-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.19command.ccommand.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 apassword",
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.18menu.cmenu.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
-0case '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]
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/
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
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