On Tue, Jul 2, 2024 at 7:34 AM Evgeny Kotkov via dev <dev@httpd.apache.org> wrote: > > Hi all, > > Currently, `httpd.h` redefines the `apr_filepath_merge()` function on Win32 > as follows: > ``` > #ifdef WIN32 > #define apr_filepath_merge ap_filepath_merge > #endif > ``` > > The intent behind this redefinition seems to be to retroactively apply > the recently introduced UNC path checks to any existing calls of > apr_filepath_merge(), including those in third-party module code. > > However, this approach has several downsides, because the security-related > behavior of the code is now tied to the inclusion of <httpd.h>: > > - Utility layers that do not include httpd.h but use apr_filepath_merge() > will need to include this header to ensure appropriate security behavior. > > - Refactorings that involve adding or removing #include <httpd.h> can > inadvertently change the security properties of the code. > > - It becomes challenging to understand the code behavior by inspection alone. > > Personally, I find these issues quite significant. > > An alternative would be to explicitly use ap_filepath_merge() everywhere and > remove the platform-specific define. This alternative is demonstrated in the > attached patch: > [[[ > * include/httpd.h > (apr_filepath_merge): Remove this Win32-specific #define, and … > > * modules/*, server/*: > …replace all its usages with ap_filepath_merge(). This ensures explicit and > consistent security-related behavior that doesn't depend on the inclusion > of the <httpd.h> header. > ]]] > > What do you think?
It certainly has a weird look/smell as-is but security@ was on the fence. I think there was probably a bias towards staying where we started, but I think it's reasonable to swap it out especially if anyone has strong feelings about it as above.
