[PATCH] http module is not checking correctly HTTP headers

[PATCH] http module is not checking correctly HTTP headers

[Javier Moragon]

According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
shall be case insensitive and we are now forced to read headers like
`Content-Length` capitalized.

The problem with that is when a HTTP server responds with a
`content-length` header in lowercase GRUB gets stuck because HTTP
module doesn't know the length of the transmision and the call never
ends. I've been able to reproduce it and after ignoring the text case
it worked perfectly.

Here is it my patch proposal:

diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index b616cf40b..570fa3934 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
char *ptr, grub_size_t len)
       data->first_line_recv = 1;
       return GRUB_ERR_NONE;
     }
-  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
+  if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
("Content-Length: ") )
       == 0 && !data->size_recv)
     {
       ptr += sizeof ("Content-Length: ") - 1;
@@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
char *ptr, grub_size_t len)
       data->size_recv = 1;
       return GRUB_ERR_NONE;
     }
-  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
-    sizeof ("Transfer-Encoding: chunked") - 1) == 0)
+  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
+    grub_strlen ("Transfer-Encoding: chunked") ) == 0)
     {
       data->chunked = 1;
       return GRUB_ERR_NONE;

Re: [PATCH] http module is not checking correctly HTTP headers

[Glenn Washburn]

On Wed, 12 Jan 2022 23:54:58 +0100
Javier Moragon <jamofer@gmail.com> wrote:

> According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> shall be case insensitive and we are now forced to read headers like
> `Content-Length` capitalized.
> 
> The problem with that is when a HTTP server responds with a
> `content-length` header in lowercase GRUB gets stuck because HTTP
> module doesn't know the length of the transmision and the call never
> ends. I've been able to reproduce it and after ignoring the text case
> it worked perfectly.
> 
> Here is it my patch proposal:
> 
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..570fa3934 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> char *ptr, grub_size_t len)
>        data->first_line_recv = 1;
>        return GRUB_ERR_NONE;
>      }
> -  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> +  if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> ("Content-Length: ") )

I don't think there should be a new line here. And why change to
grub_strlen? Now the length is calculated everytime at runtime instead
of once at compile time.

>        == 0 && !data->size_recv)
>      {
>        ptr += sizeof ("Content-Length: ") - 1;
> @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> char *ptr, grub_size_t len)
>        data->size_recv = 1;
>        return GRUB_ERR_NONE;
>      }
> -  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> -    sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> +  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> +    grub_strlen ("Transfer-Encoding: chunked") ) == 0)

Ditto on the grub_strlen. I also don't like the original indentation of
this line and think that it should align with "ptr".

>      {
>        data->chunked = 1;
>        return GRUB_ERR_NONE;

Otherwise, it seems like good patch.

Glenn

Re: [PATCH] http module is not checking correctly HTTP headers

[Javier Moragon]

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

I'm sorry, it's my first time using a mailing list for publishing
patches so I sent you the message directly instead of to grub-devel
and my mail client wrapped the patch lines. I attached the patch
Instead of pasting the diff, I hope this time the lines don't get
wrapped.

El jue, 13 ene 2022 a las 5:08, Glenn Washburn
(<development@efficientek.com>) escribió:
>
> On Wed, 12 Jan 2022 23:54:58 +0100
> Javier Moragon <jamofer@gmail.com> wrote:
>
> > According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> > shall be case insensitive and we are now forced to read headers like
> > `Content-Length` capitalized.
> >
> > The problem with that is when a HTTP server responds with a
> > `content-length` header in lowercase GRUB gets stuck because HTTP
> > module doesn't know the length of the transmision and the call never
> > ends. I've been able to reproduce it and after ignoring the text case
> > it worked perfectly.
> >
> > Here is it my patch proposal:
> >
> > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > index b616cf40b..570fa3934 100644
> > --- a/grub-core/net/http.c
> > +++ b/grub-core/net/http.c
> > @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> > char *ptr, grub_size_t len)
> >        data->first_line_recv = 1;
> >        return GRUB_ERR_NONE;
> >      }
> > -  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> > +  if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> > ("Content-Length: ") )
>
> I don't think there should be a new line here. And why change to
> grub_strlen? Now the length is calculated everytime at runtime instead
> of once at compile time.
>
> >        == 0 && !data->size_recv)
> >      {
> >        ptr += sizeof ("Content-Length: ") - 1;
> > @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> > char *ptr, grub_size_t len)
> >        data->size_recv = 1;
> >        return GRUB_ERR_NONE;
> >      }
> > -  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> > -    sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> > +  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> > +    grub_strlen ("Transfer-Encoding: chunked") ) == 0)
>
> Ditto on the grub_strlen. I also don't like the original indentation of
> this line and think that it should align with "ptr".
>
> >      {
> >        data->chunked = 1;
> >        return GRUB_ERR_NONE;
>
> Otherwise, it seems like good patch.
>
> Glenn
>

[-- Attachment #2: patch.patch --]
[-- Type: application/x-patch, Size: 1007 bytes --]

[PATCH] http module is not checking correctly HTTP headers

[Jamo]

From: Javier Moragon <jamofer@gmail.com>

I applied the last suggestion in this patch and I've finally realized 
how to use git send-email.

I'm sorry for the unnecesary mails and I hope for next contributions
I won't make the same mistakes, Thank you!

---
 grub-core/net/http.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index b616cf40b..7a56ec7ef 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
       data->first_line_recv = 1;
       return GRUB_ERR_NONE;
     }
-  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
+  if (grub_strncasecmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
       == 0 && !data->size_recv)
     {
       ptr += sizeof ("Content-Length: ") - 1;
@@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
       data->size_recv = 1;
       return GRUB_ERR_NONE;
     }
-  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
-		   sizeof ("Transfer-Encoding: chunked") - 1) == 0)
+  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
+                        sizeof ("Transfer-Encoding: chunked") - 1) == 0)
     {
       data->chunked = 1;
       return GRUB_ERR_NONE;
-- 
2.32.0

Re: [PATCH] http module is not checking correctly HTTP headers

[Glenn Washburn]

On Thu, 13 Jan 2022 23:14:54 +0100
Javier Moragon <jamofer@gmail.com> wrote:

> I'm sorry, it's my first time using a mailing list for publishing
> patches so I sent you the message directly instead of to grub-devel
> and my mail client wrapped the patch lines. I attached the patch
> Instead of pasting the diff, I hope this time the lines don't get
> wrapped.

Its okay, I had a few hiccups my first several times sending patches to
the list. Sending the patch as an attachment does fix the line wrapping
issue, but it creates another one. Its makes it more difficult for
reviewers to review it. This list is accustomed to having the patch
sent inline (in the text of the email). The advantage of this is that
reviewers get to comment inline on specific lines of the patch (like I
did for your original patch). Most people use git-format-patch and
git-send for creating and sending patches to the list.

Also, I've noticed that you changed the indentation of the second change
of this latest patch. Could you please create a new patch with the
indention exactly as it was? Your editor is probably changing the
indentation with out you realizing it. To be clear, the "sizeof" should
be preceeded by a string of 2 tabs followed by 2 spaces.

Glenn

> 
> El jue, 13 ene 2022 a las 5:08, Glenn Washburn
> (<development@efficientek.com>) escribió:
> >
> > On Wed, 12 Jan 2022 23:54:58 +0100
> > Javier Moragon <jamofer@gmail.com> wrote:
> >
> > > According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> > > shall be case insensitive and we are now forced to read headers like
> > > `Content-Length` capitalized.
> > >
> > > The problem with that is when a HTTP server responds with a
> > > `content-length` header in lowercase GRUB gets stuck because HTTP
> > > module doesn't know the length of the transmision and the call never
> > > ends. I've been able to reproduce it and after ignoring the text case
> > > it worked perfectly.
> > >
> > > Here is it my patch proposal:
> > >
> > > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > > index b616cf40b..570fa3934 100644
> > > --- a/grub-core/net/http.c
> > > +++ b/grub-core/net/http.c
> > > @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >        data->first_line_recv = 1;
> > >        return GRUB_ERR_NONE;
> > >      }
> > > -  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> > > +  if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> > > ("Content-Length: ") )
> >
> > I don't think there should be a new line here. And why change to
> > grub_strlen? Now the length is calculated everytime at runtime instead
> > of once at compile time.
> >
> > >        == 0 && !data->size_recv)
> > >      {
> > >        ptr += sizeof ("Content-Length: ") - 1;
> > > @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >        data->size_recv = 1;
> > >        return GRUB_ERR_NONE;
> > >      }
> > > -  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> > > -    sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> > > +  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> > > +    grub_strlen ("Transfer-Encoding: chunked") ) == 0)
> >
> > Ditto on the grub_strlen. I also don't like the original indentation of
> > this line and think that it should align with "ptr".
> >
> > >      {
> > >        data->chunked = 1;
> > >        return GRUB_ERR_NONE;
> >
> > Otherwise, it seems like good patch.
> >
> > Glenn
> >

Re: [PATCH] http module is not checking correctly HTTP headers

[Glenn Washburn]

On Fri, 14 Jan 2022 00:05:59 +0100
Jamo <jamofer@gmail.com> wrote:

> From: Javier Moragon <jamofer@gmail.com>
> 
> I applied the last suggestion in this patch and I've finally realized 
> how to use git send-email.
> 
> I'm sorry for the unnecesary mails and I hope for next contributions
> I won't make the same mistakes, Thank you!

No worries, we all had to learn at some point. There are still a couple
issues. One is that this space is for the commit message. For instance,
based on your first email it might be:

  According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
  shall be case insensitive and we are now forced to read headers like
  `Content-Length` capitalized.

  The problem with that is when a HTTP server responds with a
  `content-length` header in lowercase GRUB gets stuck because HTTP
  module doesn't know the length of the transmision and the call never
  ends.

Another issue is that when you update a patch, you should update its
version number. See the "-v" option in git-format-patch. Also, the list
convention is to start a new thread when submitting an updated patch.
So no need to do have an "In-Reply-To" header on the next update.

> 
> ---

If you want to have some text that will not get recorded in the commit,
like the text after your email above, you can put that here.

>  grub-core/net/http.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..7a56ec7ef 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
>        data->first_line_recv = 1;
>        return GRUB_ERR_NONE;
>      }
> -  if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> +  if (grub_strncasecmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
>        == 0 && !data->size_recv)
>      {
>        ptr += sizeof ("Content-Length: ") - 1;
> @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
>        data->size_recv = 1;
>        return GRUB_ERR_NONE;
>      }
> -  if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> -		   sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> +  if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> +                        sizeof ("Transfer-Encoding: chunked") - 1) == 0)

Almost there. The code style guidelines for this project use a TAB
character for every 8 spaces. It looks like you used all spaces to do
the indent.

>      {
>        data->chunked = 1;
>        return GRUB_ERR_NONE;

Glenn