View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0006196 | libmicrohttpd | performance | public | 2020-04-22 12:32 | 2020-04-23 16:10 |
Reporter | fefe | Assigned To | Christian Grothoff | ||
Priority | normal | Severity | tweak | Reproducibility | N/A |
Status | closed | Resolution | won't fix | ||
Product Version | 0.9.71 | ||||
Target Version | 0.9.71 | Fixed in Version | 0.9.71 | ||
Summary | 0006196: format string issue in TALER_MHD_handle_logs | ||||
Description | The 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. | ||||
Tags | No tags attached. | ||||
|
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. |
|
I was proposing (without saying it explicitly) to change the MHD callback so that it gives you a string instead of varargs. |
|
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. |
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 |