Re: [libmicrohttpd] [PATCH] MHD_add_response_header: Check on passed nullptr

[Alexander Dahl]

Hello Evgeny,

Am Thu, Jan 27, 2022 at 08:15:37PM +0300 schrieb Evgeny Grin:
> Thanks for the patch.
> Breaking API is always a bad idea, unless API itself is broken badly by
> design.
> However this is not a such case. MHD never have guaranteed tolerating of
> NULL in this function. The data must be a valid C string. The NULL pointer
> is definitely not a valid C string.

Well, I could not tell based on the API documentation, what's intended
behaviour here.

> MHD would (silently) ignore bad parameters with your patch. This may mask
> some bug in code that use MHD_add_response_header() function so while this
> patch may look useful for production code, actually it makes situation even
> worse as it may give false feeling of correct functioning instead of
> revealing bug as soon as possible.

Well, my patch just restores the behaviour of libmicrohttpd from
before 185f740e0684, where MHD_add_response_header() returned what
add_response_entry() returns.  That function add_response_entry()
checks for (response == NULL) and silently returns MHD_NO in that
case.

Intended or not, changeset 185f740e0684 broke that behaviour, and it
did not mention in commit message nor in code itself.

> Hypothetical situation:
> ===============================
> MHD_add_response_header (r, "System-Command", "sudo rm -rf /
> --no-preserve-root");
> char *behaviour = strdup ("no-execute, check-syntax-only");
> MHD_add_response_header (r, "Modify-Behavior", behaviour);
> free (behaviour);
> MHD_queue_response (c, MHD_HTTP_OK, r);
> ==============================
> Very typical mistake: no checks for returned values. The code may work fine,
> but at some moment, due to some leak in another place, strdup() fail. With
> your patch second header would not be added, but response would be sent. I
> think it's better to crash earlier in such situation than trying to fix
> automatically wrong parameters.

You're absolutely right. Users of your library must check return
values, no doubt about that.

> From the description of the issue in your library, it's clear than you was
> able to find another error (return 200 instead of 404) with wrong filename
> only when MHD stopped tolerating NULL. If MHD would not tolerate NULL
> initially (which was not intentional behaviour), you would be able to find
> and fix this bug earlier.

It's not my library, I'm just a user.  And I'm sorry, I'm confused
now.  What is the intentional behaviour of libmicrohttpd? 

I mean, libmicrohttpd detecting NULL and returning MHD_NO tells me
something went wrong at runtime, if I check for return value, which I
should do.

Checking for return value is pointless however, if libmicrohttpd
crashes before returning anything.

Greets
Alex

> 
> -- 
> Evgeny
> 
> 
> On 27.01.2022 14:10, Alexander Dahl wrote:
> > The response argument is passed to `add_response_entry()` eventually
> > which does a check on NULL.  This was done without accessing struct
> > members of *response* in the past, however since 185f740e0684 ("allow
> > clients to override sanity check for content-length header") an access
> > to response->flags leads to a segfault.
> > 
> > This was spotted when building an app with libhttpserver which currently
> > might pass a nullptr to `MHD_add_response_header()`, see the bug report
> > over there for details.
> > 
> > Link: https://github.com/etr/libhttpserver/issues/255
> > Fixes: 185f740e0684 ("allow clients to override sanity check for 
> > content-length header")
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> > 
> > Notes:
> >      Hello everyone,
> >      I discovered this when working with libhttpserver [1] which currently
> >      does not check some return codes and thus ends up passing a null
> >      pointer.  This was no problem against version 0.9.62-1 from the debian
> >      package, but is against recent 0.9.75.  I'm working on fixing that
> >      potentially harmful behaviour of the other lib, but I think the check
> >      here is valuable in itself, because it prevents libmicrohttpd to
> >      segfault.
> >      Greets
> >      Alex
> >      [1] https://github.com/etr/libhttpserver
> > 
> >   src/microhttpd/response.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/microhttpd/response.c b/src/microhttpd/response.c
> > index ca3639f4..2a8b3cbe 100644
> > --- a/src/microhttpd/response.c
> > +++ b/src/microhttpd/response.c
> > @@ -494,6 +494,9 @@ MHD_add_response_header (struct MHD_Response *response,
> >                            const char *header,
> >                            const char *content)
> >   {
> > +  if (response == NULL)
> > +    return MHD_NO;
> > +
> >     if (MHD_str_equal_caseless_ (header, MHD_HTTP_HEADER_CONNECTION))
> >       return add_response_header_connection (response, content);
> > 
> > base-commit: 1b1361e4c6e07a74e1a70f96fc570510aaa36815