|
| From: | Julien Ranc |
| Subject: | Re: [PATCH] password command implementation |
| Date: | Tue, 7 Aug 2007 14:17:16 +0200 |
Julien RANC <address@hidden> 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
> address@hidden
> --- 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
address@hidden
http://lists.gnu.org/mailman/listinfo/grub-devel
| [Prev in Thread] | Current Thread | [Next in Thread] |