View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0006196libmicrohttpdperformancepublic2020-04-22 10:322020-04-23 14:10
Reporterfefe Assigned ToChristian Grothoff Due Date
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 13: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 13: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 13: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 ModifiedUsernameFieldChange
2020-04-22 10:32fefeNew Issue
2020-04-22 10:32fefeStatusnew => assigned
2020-04-22 10:32fefeAssigned To => Christian Grothoff
2020-04-22 13:01Christian GrothoffNote Added: 0015729
2020-04-22 13:02Christian GrothoffSeverityminor => tweak
2020-04-22 13:02Christian GrothoffStatusassigned => feedback
2020-04-22 13:46Christian GrothoffAssigned ToChristian Grothoff =>
2020-04-23 13:38fefeNote Added: 0015766
2020-04-23 13:38fefeStatusfeedback => new
2020-04-23 13:49Christian GrothoffNote Added: 0015769
2020-04-23 13:49Christian GrothoffAssigned To => Christian Grothoff
2020-04-23 13:49Christian GrothoffStatusnew => feedback
2020-04-23 13:50Christian GrothoffProjectTaler => libmicrohttpd
2020-04-23 13:50Christian GrothoffCategoryexchange => General
2020-04-23 13:50Christian GrothoffCategoryGeneral => other
2020-04-23 13:50Christian GrothoffProduct Version0.7.0 => 0.9.71
2020-04-23 14:10Christian GrothoffStatusfeedback => closed
2020-04-23 14:10Christian GrothoffResolutionopen => won't fix
2020-04-23 14:10Christian GrothoffCategoryother => performance
2020-04-23 14:10Christian GrothoffFixed in Version => 0.9.71
2020-04-23 14:10Christian GrothoffTarget Version => 0.9.71