Re: [SECURITY PATCH 100/117] kern/parser: Fix a stack buffer overflow
Hi, Sorry for taking a while to look at this. On 10/06/2021 12:55, Paul Menzel wrote: > Dear Daniel, dear Chris, > > > Am 02.03.21 um 19:01 schrieb Daniel Kiper: >> From: Chris Coulson >> >> grub_parser_split_cmdline() expands variable names present in the >> supplied >> command line in to their corresponding variable contents and uses a 1 kiB >> stack buffer for temporary storage without sufficient bounds checking. If >> the function is called with a command line that references a variable >> with >> a sufficiently large payload, it is possible to overflow the stack >> buffer via tab completion, corrupt the stack frame and potentially >> control execution. >> >> Fixes: CVE-2020-27749 >> >> Reported-by: Chris Coulson >> Signed-off-by: Chris Coulson >> Signed-off-by: Darren Kenny >> Reviewed-by: Daniel Kiper >> --- >> grub-core/kern/parser.c | 110 >> +--- >> 1 file changed, 67 insertions(+), 43 deletions(-) >> >> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c >> index e010eaa1f..6ab7aa427 100644 >> --- a/grub-core/kern/parser.c >> +++ b/grub-core/kern/parser.c >> @@ -18,6 +18,7 @@ >> */ >> #include >> +#include >> #include >> #include >> #include >> @@ -107,8 +108,8 @@ check_varstate (grub_parser_state_t s) >> } >> -static void >> -add_var (char *varname, char **bp, char **vp, >> +static grub_err_t >> +add_var (grub_buffer_t varname, grub_buffer_t buf, >> grub_parser_state_t state, grub_parser_state_t newstate) >> { >> const char *val; >> @@ -116,31 +117,41 @@ add_var (char *varname, char **bp, char **vp, >> /* Check if a variable was being read in and the end of the name >> was reached. */ >> if (!(check_varstate (state) && !check_varstate (newstate))) >> - return; >> + return GRUB_ERR_NONE; >> - *((*vp)++) = '\0'; >> - val = grub_env_get (varname); >> - *vp = varname; >> + if (grub_buffer_append_char (varname, '\0') != GRUB_ERR_NONE) >> + return grub_errno; >> + >> + val = grub_env_get ((const char *) grub_buffer_peek_data (varname)); >> + grub_buffer_reset (varname); >> if (!val) >> - return; >> + return GRUB_ERR_NONE; >> /* Insert the contents of the variable in the buffer. */ >> - for (; *val; val++) >> - *((*bp)++) = *val; >> + return grub_buffer_append_data (buf, val, grub_strlen (val)); >> } >> -static void >> -terminate_arg (char *buffer, char **bp, int *argc) >> +static grub_err_t >> +terminate_arg (grub_buffer_t buffer, int *argc) >> { >> - if (*bp != buffer && *((*bp) - 1) != '\0') >> - { >> - *((*bp)++) = '\0'; >> - (*argc)++; >> - } >> + grub_size_t unread = grub_buffer_get_unread_bytes (buffer); >> + >> + if (unread == 0) >> + return GRUB_ERR_NONE; >> + >> + if (*(const char *) grub_buffer_peek_data_at (buffer, unread - 1) >> == '\0') >> + return GRUB_ERR_NONE; >> + >> + if (grub_buffer_append_char (buffer, '\0') != GRUB_ERR_NONE) >> + return grub_errno; >> + >> + (*argc)++; >> + >> + return GRUB_ERR_NONE; >> } >> static grub_err_t >> -process_char (char c, char *buffer, char **bp, char *varname, char **vp, >> +process_char (char c, grub_buffer_t buffer, grub_buffer_t varname, >> grub_parser_state_t state, int *argc, >> grub_parser_state_t *newstate) >> { >> @@ -153,12 +164,13 @@ process_char (char c, char *buffer, char **bp, >> char *varname, char **vp, >> * not describe the variable anymore, write the variable to >> * the buffer. >> */ >> - add_var (varname, bp, vp, state, *newstate); >> + if (add_var (varname, buffer, state, *newstate) != GRUB_ERR_NONE) >> + return grub_errno; >> if (check_varstate (*newstate)) >> { >> if (use) >> - *((*vp)++) = use; >> + return grub_buffer_append_char (varname, use); >> } >> else if (*newstate == GRUB_PARSER_STATE_TEXT && >> state != GRUB_PARSER_STATE_ESC && grub_isspace (use)) >> @@ -167,10 +179,10 @@ process_char (char c, char *buffer, char **bp, >> char *varname, char **vp, >> * Don't add more than one argument if multiple >> * spaces are used. >> */ >> - terminate_arg (buffer, bp, argc); >> + return terminate_arg (buffer, argc); >> } >> else if (use) >> - *((*bp)++) = use; >> + return grub_buffer_append_char (buffer, use); >> return GRUB_ERR_NONE; >> } >> @@ -181,19 +193,22 @@ grub_parser_split_cmdline (const char *cmdline, >> int *argc, char ***argv) >> { >> grub_parser_state_t state = GRUB_PARSER_STATE_TEXT; >> - /* XXX: Fixed size buffer, perhaps this buffer should be dynamically >> - allocated. */ >> - char buffer[1024]; >> - char *bp = buffer; >> + grub_buffer_t buffer, varname; >> char *rd = (char *) cmdline; >> char *rp = rd; >> - char varname[200]; >> - char *vp = varname; >> - char *args; >> int i; >> *argc = 0; >> *argv =
Re: [SECURITY PATCH 100/117] kern/parser: Fix a stack buffer overflow
Dear Daniel, dear Chris, Am 10.06.21 um 13:55 schrieb Paul Menzel: Am 02.03.21 um 19:01 schrieb Daniel Kiper: From: Chris Coulson grub_parser_split_cmdline() expands variable names present in the supplied command line in to their corresponding variable contents and uses a 1 kiB stack buffer for temporary storage without sufficient bounds checking. If the function is called with a command line that references a variable with a sufficiently large payload, it is possible to overflow the stack buffer via tab completion, corrupt the stack frame and potentially control execution. Fixes: CVE-2020-27749 Reported-by: Chris Coulson Signed-off-by: Chris Coulson Signed-off-by: Darren Kenny Reviewed-by: Daniel Kiper --- grub-core/kern/parser.c | 110 +--- 1 file changed, 67 insertions(+), 43 deletions(-) diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c index e010eaa1f..6ab7aa427 100644 --- a/grub-core/kern/parser.c +++ b/grub-core/kern/parser.c @@ -18,6 +18,7 @@ */ #include +#include #include #include #include @@ -107,8 +108,8 @@ check_varstate (grub_parser_state_t s) } -static void -add_var (char *varname, char **bp, char **vp, +static grub_err_t +add_var (grub_buffer_t varname, grub_buffer_t buf, grub_parser_state_t state, grub_parser_state_t newstate) { const char *val; @@ -116,31 +117,41 @@ add_var (char *varname, char **bp, char **vp, /* Check if a variable was being read in and the end of the name was reached. */ if (!(check_varstate (state) && !check_varstate (newstate))) - return; + return GRUB_ERR_NONE; - *((*vp)++) = '\0'; - val = grub_env_get (varname); - *vp = varname; + if (grub_buffer_append_char (varname, '\0') != GRUB_ERR_NONE) + return grub_errno; + + val = grub_env_get ((const char *) grub_buffer_peek_data (varname)); + grub_buffer_reset (varname); if (!val) - return; + return GRUB_ERR_NONE; /* Insert the contents of the variable in the buffer. */ - for (; *val; val++) - *((*bp)++) = *val; + return grub_buffer_append_data (buf, val, grub_strlen (val)); } -static void -terminate_arg (char *buffer, char **bp, int *argc) +static grub_err_t +terminate_arg (grub_buffer_t buffer, int *argc) { - if (*bp != buffer && *((*bp) - 1) != '\0') - { - *((*bp)++) = '\0'; - (*argc)++; - } + grub_size_t unread = grub_buffer_get_unread_bytes (buffer); + + if (unread == 0) + return GRUB_ERR_NONE; + + if (*(const char *) grub_buffer_peek_data_at (buffer, unread - 1) == '\0') + return GRUB_ERR_NONE; + + if (grub_buffer_append_char (buffer, '\0') != GRUB_ERR_NONE) + return grub_errno; + + (*argc)++; + + return GRUB_ERR_NONE; } static grub_err_t -process_char (char c, char *buffer, char **bp, char *varname, char **vp, +process_char (char c, grub_buffer_t buffer, grub_buffer_t varname, grub_parser_state_t state, int *argc, grub_parser_state_t *newstate) { @@ -153,12 +164,13 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * not describe the variable anymore, write the variable to * the buffer. */ - add_var (varname, bp, vp, state, *newstate); + if (add_var (varname, buffer, state, *newstate) != GRUB_ERR_NONE) + return grub_errno; if (check_varstate (*newstate)) { if (use) - *((*vp)++) = use; + return grub_buffer_append_char (varname, use); } else if (*newstate == GRUB_PARSER_STATE_TEXT && state != GRUB_PARSER_STATE_ESC && grub_isspace (use)) @@ -167,10 +179,10 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * Don't add more than one argument if multiple * spaces are used. */ - terminate_arg (buffer, bp, argc); + return terminate_arg (buffer, argc); } else if (use) - *((*bp)++) = use; + return grub_buffer_append_char (buffer, use); return GRUB_ERR_NONE; } @@ -181,19 +193,22 @@ grub_parser_split_cmdline (const char *cmdline, int *argc, char ***argv) { grub_parser_state_t state = GRUB_PARSER_STATE_TEXT; - /* XXX: Fixed size buffer, perhaps this buffer should be dynamically - allocated. */ - char buffer[1024]; - char *bp = buffer; + grub_buffer_t buffer, varname; char *rd = (char *) cmdline; char *rp = rd; - char varname[200]; - char *vp = varname; - char *args; int i; *argc = 0; *argv = NULL; + + buffer = grub_buffer_new (1024); + if (buffer == NULL) + return grub_errno; + + varname = grub_buffer_new (200); + if (varname == NULL) + goto fail; + do { if (rp == NULL || *rp == '\0') @@ -219,7 +234,7 @@ grub_parser_split_cmdline (const char *cmdline, { grub_parser_state_t newstate; - if (process_char (*rp, buffer, , varname, , state, argc, + if (process_char (*rp, buffer, varname, state, argc, ) !=
Re: [SECURITY PATCH 100/117] kern/parser: Fix a stack buffer overflow
Dear Daniel, dear Chris, Am 02.03.21 um 19:01 schrieb Daniel Kiper: From: Chris Coulson grub_parser_split_cmdline() expands variable names present in the supplied command line in to their corresponding variable contents and uses a 1 kiB stack buffer for temporary storage without sufficient bounds checking. If the function is called with a command line that references a variable with a sufficiently large payload, it is possible to overflow the stack buffer via tab completion, corrupt the stack frame and potentially control execution. Fixes: CVE-2020-27749 Reported-by: Chris Coulson Signed-off-by: Chris Coulson Signed-off-by: Darren Kenny Reviewed-by: Daniel Kiper --- grub-core/kern/parser.c | 110 +--- 1 file changed, 67 insertions(+), 43 deletions(-) diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c index e010eaa1f..6ab7aa427 100644 --- a/grub-core/kern/parser.c +++ b/grub-core/kern/parser.c @@ -18,6 +18,7 @@ */ #include +#include #include #include #include @@ -107,8 +108,8 @@ check_varstate (grub_parser_state_t s) } -static void -add_var (char *varname, char **bp, char **vp, +static grub_err_t +add_var (grub_buffer_t varname, grub_buffer_t buf, grub_parser_state_t state, grub_parser_state_t newstate) { const char *val; @@ -116,31 +117,41 @@ add_var (char *varname, char **bp, char **vp, /* Check if a variable was being read in and the end of the name was reached. */ if (!(check_varstate (state) && !check_varstate (newstate))) -return; +return GRUB_ERR_NONE; - *((*vp)++) = '\0'; - val = grub_env_get (varname); - *vp = varname; + if (grub_buffer_append_char (varname, '\0') != GRUB_ERR_NONE) +return grub_errno; + + val = grub_env_get ((const char *) grub_buffer_peek_data (varname)); + grub_buffer_reset (varname); if (!val) -return; +return GRUB_ERR_NONE; /* Insert the contents of the variable in the buffer. */ - for (; *val; val++) -*((*bp)++) = *val; + return grub_buffer_append_data (buf, val, grub_strlen (val)); } -static void -terminate_arg (char *buffer, char **bp, int *argc) +static grub_err_t +terminate_arg (grub_buffer_t buffer, int *argc) { - if (*bp != buffer && *((*bp) - 1) != '\0') -{ - *((*bp)++) = '\0'; - (*argc)++; -} + grub_size_t unread = grub_buffer_get_unread_bytes (buffer); + + if (unread == 0) +return GRUB_ERR_NONE; + + if (*(const char *) grub_buffer_peek_data_at (buffer, unread - 1) == '\0') +return GRUB_ERR_NONE; + + if (grub_buffer_append_char (buffer, '\0') != GRUB_ERR_NONE) +return grub_errno; + + (*argc)++; + + return GRUB_ERR_NONE; } static grub_err_t -process_char (char c, char *buffer, char **bp, char *varname, char **vp, +process_char (char c, grub_buffer_t buffer, grub_buffer_t varname, grub_parser_state_t state, int *argc, grub_parser_state_t *newstate) { @@ -153,12 +164,13 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * not describe the variable anymore, write the variable to * the buffer. */ - add_var (varname, bp, vp, state, *newstate); + if (add_var (varname, buffer, state, *newstate) != GRUB_ERR_NONE) +return grub_errno; if (check_varstate (*newstate)) { if (use) - *((*vp)++) = use; +return grub_buffer_append_char (varname, use); } else if (*newstate == GRUB_PARSER_STATE_TEXT && state != GRUB_PARSER_STATE_ESC && grub_isspace (use)) @@ -167,10 +179,10 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * Don't add more than one argument if multiple * spaces are used. */ - terminate_arg (buffer, bp, argc); + return terminate_arg (buffer, argc); } else if (use) -*((*bp)++) = use; +return grub_buffer_append_char (buffer, use); return GRUB_ERR_NONE; } @@ -181,19 +193,22 @@ grub_parser_split_cmdline (const char *cmdline, int *argc, char ***argv) { grub_parser_state_t state = GRUB_PARSER_STATE_TEXT; - /* XXX: Fixed size buffer, perhaps this buffer should be dynamically - allocated. */ - char buffer[1024]; - char *bp = buffer; + grub_buffer_t buffer, varname; char *rd = (char *) cmdline; char *rp = rd; - char varname[200]; - char *vp = varname; - char *args; int i; *argc = 0; *argv = NULL; + + buffer = grub_buffer_new (1024); + if (buffer == NULL) +return grub_errno; + + varname = grub_buffer_new (200); + if (varname == NULL) +goto fail; + do { if (rp == NULL || *rp == '\0') @@ -219,7 +234,7 @@ grub_parser_split_cmdline (const char *cmdline, { grub_parser_state_t newstate; - if (process_char (*rp, buffer, , varname, , state, argc, + if (process_char (*rp, buffer, varname, state, argc,
[SECURITY PATCH 100/117] kern/parser: Fix a stack buffer overflow
From: Chris Coulson grub_parser_split_cmdline() expands variable names present in the supplied command line in to their corresponding variable contents and uses a 1 kiB stack buffer for temporary storage without sufficient bounds checking. If the function is called with a command line that references a variable with a sufficiently large payload, it is possible to overflow the stack buffer via tab completion, corrupt the stack frame and potentially control execution. Fixes: CVE-2020-27749 Reported-by: Chris Coulson Signed-off-by: Chris Coulson Signed-off-by: Darren Kenny Reviewed-by: Daniel Kiper --- grub-core/kern/parser.c | 110 +--- 1 file changed, 67 insertions(+), 43 deletions(-) diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c index e010eaa1f..6ab7aa427 100644 --- a/grub-core/kern/parser.c +++ b/grub-core/kern/parser.c @@ -18,6 +18,7 @@ */ #include +#include #include #include #include @@ -107,8 +108,8 @@ check_varstate (grub_parser_state_t s) } -static void -add_var (char *varname, char **bp, char **vp, +static grub_err_t +add_var (grub_buffer_t varname, grub_buffer_t buf, grub_parser_state_t state, grub_parser_state_t newstate) { const char *val; @@ -116,31 +117,41 @@ add_var (char *varname, char **bp, char **vp, /* Check if a variable was being read in and the end of the name was reached. */ if (!(check_varstate (state) && !check_varstate (newstate))) -return; +return GRUB_ERR_NONE; - *((*vp)++) = '\0'; - val = grub_env_get (varname); - *vp = varname; + if (grub_buffer_append_char (varname, '\0') != GRUB_ERR_NONE) +return grub_errno; + + val = grub_env_get ((const char *) grub_buffer_peek_data (varname)); + grub_buffer_reset (varname); if (!val) -return; +return GRUB_ERR_NONE; /* Insert the contents of the variable in the buffer. */ - for (; *val; val++) -*((*bp)++) = *val; + return grub_buffer_append_data (buf, val, grub_strlen (val)); } -static void -terminate_arg (char *buffer, char **bp, int *argc) +static grub_err_t +terminate_arg (grub_buffer_t buffer, int *argc) { - if (*bp != buffer && *((*bp) - 1) != '\0') -{ - *((*bp)++) = '\0'; - (*argc)++; -} + grub_size_t unread = grub_buffer_get_unread_bytes (buffer); + + if (unread == 0) +return GRUB_ERR_NONE; + + if (*(const char *) grub_buffer_peek_data_at (buffer, unread - 1) == '\0') +return GRUB_ERR_NONE; + + if (grub_buffer_append_char (buffer, '\0') != GRUB_ERR_NONE) +return grub_errno; + + (*argc)++; + + return GRUB_ERR_NONE; } static grub_err_t -process_char (char c, char *buffer, char **bp, char *varname, char **vp, +process_char (char c, grub_buffer_t buffer, grub_buffer_t varname, grub_parser_state_t state, int *argc, grub_parser_state_t *newstate) { @@ -153,12 +164,13 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * not describe the variable anymore, write the variable to * the buffer. */ - add_var (varname, bp, vp, state, *newstate); + if (add_var (varname, buffer, state, *newstate) != GRUB_ERR_NONE) +return grub_errno; if (check_varstate (*newstate)) { if (use) - *((*vp)++) = use; +return grub_buffer_append_char (varname, use); } else if (*newstate == GRUB_PARSER_STATE_TEXT && state != GRUB_PARSER_STATE_ESC && grub_isspace (use)) @@ -167,10 +179,10 @@ process_char (char c, char *buffer, char **bp, char *varname, char **vp, * Don't add more than one argument if multiple * spaces are used. */ - terminate_arg (buffer, bp, argc); + return terminate_arg (buffer, argc); } else if (use) -*((*bp)++) = use; +return grub_buffer_append_char (buffer, use); return GRUB_ERR_NONE; } @@ -181,19 +193,22 @@ grub_parser_split_cmdline (const char *cmdline, int *argc, char ***argv) { grub_parser_state_t state = GRUB_PARSER_STATE_TEXT; - /* XXX: Fixed size buffer, perhaps this buffer should be dynamically - allocated. */ - char buffer[1024]; - char *bp = buffer; + grub_buffer_t buffer, varname; char *rd = (char *) cmdline; char *rp = rd; - char varname[200]; - char *vp = varname; - char *args; int i; *argc = 0; *argv = NULL; + + buffer = grub_buffer_new (1024); + if (buffer == NULL) +return grub_errno; + + varname = grub_buffer_new (200); + if (varname == NULL) +goto fail; + do { if (rp == NULL || *rp == '\0') @@ -219,7 +234,7 @@ grub_parser_split_cmdline (const char *cmdline, { grub_parser_state_t newstate; - if (process_char (*rp, buffer, , varname, , state, argc, + if (process_char (*rp, buffer, varname, state, argc, ) != GRUB_ERR_NONE) goto fail; @@ -230,10 +245,12 @@ grub_parser_split_cmdline (const char *cmdline,