[PATCH 0/2] Fix a couple issues in moddep parsing

[PATCH 0/2] Fix a couple issues in moddep parsing

[Glenn Washburn]

The first patch fixes an OOB read bug and the second outputs a less confusing
error to the user when the moddep.lst line is too long. Really it would be
better to support lines of unlimited length, but I'm not motivated to add that.
The condition under which these issues are triggered should never really happen
because no module (currently) has enough dependencies to generate such long
lines in moddep.lst. I was triggering this under some odd conditions where
the all_video module dependency line contained all grub modules. So I think
having a max length for moddep.lst lines is reasonable at this point.

Glenn

Glenn Washburn (2):
  util/resolve.c: Do not read past the end of the array in read_dep_list
  util/resolve.c: Bail with error if moddep lst file line is too long

 util/resolve.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH 1/2] util/resolve.c: Do not read past the end of the array in read_dep_list

[Glenn Washburn]

If the last non-NULL byte of 'buf' is not a white-space character (such as
when a read line is longer than the size of 'buf'), then 'p' will eventually
point to the byte after the last byte in 'buf'. After which 'p' will be
dereferenced in the while conditional leading to an out of bounds read. Make
sure that 'p' is inside 'buf' before dereferencing it.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 util/resolve.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/resolve.c b/util/resolve.c
index 3e887d2ff..5e9afa10c 100644
--- a/util/resolve.c
+++ b/util/resolve.c
@@ -102,7 +102,7 @@ read_dep_list (FILE *fp)
       dep_list = dep;
 
       /* Add dependencies.  */
-      while (*p)
+      while (p < (buf + sizeof (buf)) && *p)
 	{
 	  struct mod_list *mod;
 	  char *name;
-- 
2.27.0

[PATCH 2/2] util/resolve.c: Bail with error if moddep lst file line is too long

[Glenn Washburn]

The code reads each line into a buffer of size 1024 and does not check if
the line is longer. So a line longer than 1024 will be read as a valid line
followed by an invalid line. Then an error confusing to the user is sent
with the test "invalid line format". But the line format is prefectly fine,
the problem is in GRUB's parser. Check if we've hit a line longer than the
size of the buffer, and if so send a more correct and reasonable error.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 util/resolve.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/resolve.c b/util/resolve.c
index 5e9afa10c..b0f2661f7 100644
--- a/util/resolve.c
+++ b/util/resolve.c
@@ -127,6 +127,9 @@ read_dep_list (FILE *fp)
 	  mod->next = dep->list;
 	  dep->list = mod;
 	}
+
+	if ((p - buf) == sizeof (buf))
+	  grub_util_error (_("line too long, length greater than %lu: module %s"), sizeof (buf), dep->name);
     }
 
   return dep_list;
-- 
2.27.0

Re: [PATCH 2/2] util/resolve.c: Bail with error if moddep lst file line is too long

[Daniel Kiper]

On Wed, Jan 12, 2022 at 08:55:01PM -0600, Glenn Washburn wrote:
> The code reads each line into a buffer of size 1024 and does not check if
> the line is longer. So a line longer than 1024 will be read as a valid line
> followed by an invalid line. Then an error confusing to the user is sent
> with the test "invalid line format". But the line format is prefectly fine,
> the problem is in GRUB's parser. Check if we've hit a line longer than the
> size of the buffer, and if so send a more correct and reasonable error.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  util/resolve.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/util/resolve.c b/util/resolve.c
> index 5e9afa10c..b0f2661f7 100644
> --- a/util/resolve.c
> +++ b/util/resolve.c
> @@ -127,6 +127,9 @@ read_dep_list (FILE *fp)
>  	  mod->next = dep->list;
>  	  dep->list = mod;
>  	}
> +
> +	if ((p - buf) == sizeof (buf))
> +	  grub_util_error (_("line too long, length greater than %lu: module %s"), sizeof (buf), dep->name);

I had to replace "%lu" with "%zu". Otherwise Windows builds were broken.

Daniel

Re: [PATCH 0/2] Fix a couple issues in moddep parsing

[Daniel Kiper]

On Wed, Jan 12, 2022 at 08:54:59PM -0600, Glenn Washburn wrote:
> The first patch fixes an OOB read bug and the second outputs a less confusing
> error to the user when the moddep.lst line is too long. Really it would be
> better to support lines of unlimited length, but I'm not motivated to add that.
> The condition under which these issues are triggered should never really happen
> because no module (currently) has enough dependencies to generate such long
> lines in moddep.lst. I was triggering this under some odd conditions where
> the all_video module dependency line contained all grub modules. So I think
> having a max length for moddep.lst lines is reasonable at this point.

For both Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel