Re: [RFC/PATCH] Avoid redefining apr_filepath_merge() for UNC path checks
On Fri, Jul 12, 2024 at 7:15 AM Ruediger Pluem wrote: > On 7/12/24 1:05 PM, Eric Covener wrote: > > Any concerns on this? @Ruediger Pluem ? > > I agree with all the arguments, but how do we protect 3rd party modules? > They would need to switch to ap_filepath_merge as well. Leaving the awkward redefine in would help there, while still updating all the callers we can?
Re: [RFC/PATCH] Avoid redefining apr_filepath_merge() for UNC path checks
On 7/12/24 1:05 PM, Eric Covener wrote: > Any concerns on this? @Ruediger Pluem ? I agree with all the arguments, but how do we protect 3rd party modules? They would need to switch to ap_filepath_merge as well. Regards Rüdiger
Re: [RFC/PATCH] Avoid redefining apr_filepath_merge() for UNC path checks
Any concerns on this? @Ruediger Pluem ? On Wed, Jul 3, 2024 at 9:29 PM Eric Covener wrote: > > On Tue, Jul 2, 2024 at 7:34 AM Evgeny Kotkov via dev > 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 : > > > > - 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 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 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. -- Eric Covener [email protected]
Re: [RFC/PATCH] Avoid redefining apr_filepath_merge() for UNC path checks
On Tue, Jul 2, 2024 at 7:34 AM Evgeny Kotkov via dev 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 : > > - 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 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 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.
