View Issue Details

IDProjectCategoryView StatusLast Update
0006196libmicrohttpdperformancepublic2020-04-23 16:10
Reporterfefe Assigned ToChristian Grothoff  
PrioritynormalSeveritytweakReproducibilityN/A
Status closedResolutionwon't fix 
Product Version0.9.71 
Target Version0.9.71Fixed in Version0.9.71 
Summary0006196: format string issue in TALER_MHD_handle_logs
DescriptionThe code for TALER_MHD_handle_logs is in exchange/src/mhd/mhd_config.c.
It takes a format string and a va_list as arguments, and calls vsnprintf on it.
This poses the risk of format string bugs, so we need to make sure none of the callers passes in an attacker-controlled format string.

However, there are no callers. Instead a function pointer to this function is handed out to external modules.

I would recommend against this practice. Instead only give external modules an API where give you a formatted string.
TagsNo tags attached.

Activities

Christian Grothoff

2020-04-22 15:01

manager   ~0015729

There is nothing that CAN be done here. The function exists, because MHD can be given a function with this signature for logging. This function implements this MHD-callback for the GNUnet logger.

Now, to avoid having to re-implement this very same function again and again, we implement it ONCE. That's best practice. Calling any function that takes 'va' and/or a format string with bad inputs is virtually always going to result in undefined behavior.

Now, you can argue against MHD's API here, but there we have many additional constraints: no malloc(), no fixed-size buffers, limited stack space, possibly logging disabled, etc. So I'm also pretty sure we won't change the MHD API.

fefe

2020-04-23 15:38

reporter   ~0015766

I was proposing (without saying it explicitly) to change the MHD callback so that it gives you a string instead of varargs.

Christian Grothoff

2020-04-23 15:49

manager   ~0015769

I understand, but then MHD would have to worry about finding the space for that string (variable-size on the stack, etc.), and would have to build it even if the caller doesn't actually end up using it. Given MHDs focus on ultra-high performance and minimal memory utilization including code size, I don't like that. There are users that compile MHD in a way that avoids it even using vasprintf() in the code base. We have done optimizations on MHD to minimize the functions from libc it depends upon for embedded systems (like replacing sscanf with stroul). And the proposed fix merely avoids va-args crossing modules. So as MHD maintainer, I'm against this change.

Issue History

Date Modified Username Field Change
2020-04-22 12:32 fefe New Issue
2020-04-22 12:32 fefe Status new => assigned
2020-04-22 12:32 fefe Assigned To => Christian Grothoff
2020-04-22 15:01 Christian Grothoff Note Added: 0015729
2020-04-22 15:02 Christian Grothoff Severity minor => tweak
2020-04-22 15:02 Christian Grothoff Status assigned => feedback
2020-04-22 15:46 Christian Grothoff Assigned To Christian Grothoff =>
2020-04-23 15:38 fefe Note Added: 0015766
2020-04-23 15:38 fefe Status feedback => new
2020-04-23 15:49 Christian Grothoff Note Added: 0015769
2020-04-23 15:49 Christian Grothoff Assigned To => Christian Grothoff
2020-04-23 15:49 Christian Grothoff Status new => feedback
2020-04-23 15:50 Christian Grothoff Project Taler => libmicrohttpd
2020-04-23 15:50 Christian Grothoff Category exchange => General
2020-04-23 15:50 Christian Grothoff Category General => other
2020-04-23 15:50 Christian Grothoff Product Version 0.7.0 => 0.9.71
2020-04-23 16:10 Christian Grothoff Status feedback => closed
2020-04-23 16:10 Christian Grothoff Resolution open => won't fix
2020-04-23 16:10 Christian Grothoff Category other => performance
2020-04-23 16:10 Christian Grothoff Fixed in Version => 0.9.71
2020-04-23 16:10 Christian Grothoff Target Version => 0.9.71