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

[Evgeny Grin]

Hi Alexander,

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.

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.

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.

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.

--
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

OpenPGP_signature
Description: OpenPGP digital signature