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?
Thanks,
Evgeny Kotkov
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1918820)
+++ include/httpd.h (working copy)
@@ -2875,10 +2875,6 @@
apr_int32_t flags,
apr_pool_t *p);
-#ifdef WIN32
-#define apr_filepath_merge ap_filepath_merge
-#endif
-
/* Win32/NetWare/OS2 need to check for both forward and back slashes
* in ap_normalize_path() and ap_escape_url().
*/
Index: modules/arch/win32/mod_isapi.c
===================================================================
--- modules/arch/win32/mod_isapi.c (revision 1918820)
+++ modules/arch/win32/mod_isapi.c (working copy)
@@ -990,7 +990,7 @@
#ifdef WIN32
/* We need to make this a real Windows path name */
- apr_filepath_merge(&file, "", file, APR_FILEPATH_NATIVE, r->pool);
+ ap_filepath_merge(&file, "", file, APR_FILEPATH_NATIVE, r->pool);
#endif
*buf_size = apr_cpystrn(buf_data, file, *buf_size) - buf_data;
Index: modules/dav/fs/quota.c
===================================================================
--- modules/dav/fs/quota.c (revision 1918820)
+++ modules/dav/fs/quota.c (working copy)
@@ -98,10 +98,10 @@
break;
case APR_DIR:
- rv = apr_filepath_merge(&newpath, path, finfo.name, 0, r->pool);
+ rv = ap_filepath_merge(&newpath, path, finfo.name, 0, r->pool);
if (rv != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
- "apr_filepath_merge \"%s\" \"%s\" failed",
+ "ap_filepath_merge \"%s\" \"%s\" failed",
path, finfo.name);
goto out;
}
Index: modules/filters/mod_include.c
===================================================================
--- modules/filters/mod_include.c (revision 1918820)
+++ modules/filters/mod_include.c (working copy)
@@ -1697,9 +1697,9 @@
char *newpath;
/* be safe; only files in this directory or below allowed */
- rv = apr_filepath_merge(&newpath, NULL, tag_val,
- APR_FILEPATH_SECUREROOTTEST |
- APR_FILEPATH_NOTABSOLUTE, r->pool);
+ rv = ap_filepath_merge(&newpath, NULL, tag_val,
+ APR_FILEPATH_SECUREROOTTEST |
+ APR_FILEPATH_NOTABSOLUTE, r->pool);
if (rv != APR_SUCCESS) {
error_fmt = APLOGNO(02668) "unable to access file \"%s\" "
@@ -1834,9 +1834,9 @@
char *newpath;
/* be safe; only files in this directory or below allowed */
- rv = apr_filepath_merge(&newpath, NULL, parsed_string,
- APR_FILEPATH_SECUREROOTTEST |
- APR_FILEPATH_NOTABSOLUTE, ctx->dpool);
+ rv = ap_filepath_merge(&newpath, NULL, parsed_string,
+ APR_FILEPATH_SECUREROOTTEST |
+ APR_FILEPATH_NOTABSOLUTE, ctx->dpool);
if (rv != APR_SUCCESS) {
error_fmt = "unable to include file \"%s\" in parsed file %s";
Index: modules/lua/mod_lua.c
===================================================================
--- modules/lua/mod_lua.c (revision 1918820)
+++ modules/lua/mod_lua.c (working copy)
@@ -196,8 +196,8 @@
if (filename) {
char *file;
- apr_filepath_merge(&file, server_cfg->root_path,
- filename, APR_FILEPATH_NOTRELATIVE, r->pool);
+ ap_filepath_merge(&file, server_cfg->root_path,
+ filename, APR_FILEPATH_NOTRELATIVE, r->pool);
spec->file = file;
}
else {
@@ -1541,11 +1541,11 @@
ap_get_module_config(cmd->server->module_config, &lua_module);
char *fixed_filename;
- rv = apr_filepath_merge(&fixed_filename,
- server_cfg->root_path,
- arg,
- APR_FILEPATH_NOTRELATIVE,
- cmd->pool);
+ rv = ap_filepath_merge(&fixed_filename,
+ server_cfg->root_path,
+ arg,
+ APR_FILEPATH_NOTRELATIVE,
+ cmd->pool);
if (rv != APR_SUCCESS) {
return apr_psprintf(cmd->pool,
Index: modules/mappers/mod_alias.c
===================================================================
--- modules/mappers/mod_alias.c (revision 1918820)
+++ modules/mappers/mod_alias.c (working copy)
@@ -585,7 +585,7 @@
char *fake = r->uri + l;
/*
- * For the apr_filepath_merge below we need a relative path
+ * For the ap_filepath_merge below we need a relative path
* Hence skip all leading '/'
*/
while (*fake == '/') {
@@ -594,9 +594,9 @@
/* Merge if there is something left to merge */
if (*fake) {
- if ((rv = apr_filepath_merge(&found, alias->real, fake,
- APR_FILEPATH_TRUENAME
- | APR_FILEPATH_SECUREROOT,
r->pool))
+ if ((rv = ap_filepath_merge(&found, alias->real, fake,
+ APR_FILEPATH_TRUENAME
+ | APR_FILEPATH_SECUREROOT,
r->pool))
!= APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
APLOGNO(10297)
"Cannot map %s to file",
r->the_request);
Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c (revision 1918820)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -1004,16 +1004,16 @@
* enough.
*/
if ((slash = ap_strchr_c(curpath, '/')) != NULL) {
- rv = apr_filepath_merge(&statpath, root,
- apr_pstrndup(pool, curpath,
- (apr_size_t)(slash - curpath)),
- APR_FILEPATH_NOTABOVEROOT |
- APR_FILEPATH_NOTRELATIVE, pool);
+ rv = ap_filepath_merge(&statpath, root,
+ apr_pstrndup(pool, curpath,
+ (apr_size_t)(slash - curpath)),
+ APR_FILEPATH_NOTABOVEROOT |
+ APR_FILEPATH_NOTRELATIVE, pool);
}
else {
- rv = apr_filepath_merge(&statpath, root, curpath,
- APR_FILEPATH_NOTABOVEROOT |
- APR_FILEPATH_NOTRELATIVE, pool);
+ rv = ap_filepath_merge(&statpath, root, curpath,
+ APR_FILEPATH_NOTABOVEROOT |
+ APR_FILEPATH_NOTRELATIVE, pool);
}
if (rv == APR_SUCCESS) {
Index: modules/md/md_util.c
===================================================================
--- modules/md/md_util.c (revision 1918820)
+++ modules/md/md_util.c (working copy)
@@ -422,7 +422,7 @@
va_start(ap, p);
path = va_arg(ap, char *);
while (path && APR_SUCCESS == rv && (segment = va_arg(ap, char *))) {
- rv = apr_filepath_merge((char **)&path, path, segment,
APR_FILEPATH_SECUREROOT , p);
+ rv = ap_filepath_merge((char **)&path, path, segment,
APR_FILEPATH_SECUREROOT , p);
}
va_end(ap);
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1918820)
+++ modules/proxy/proxy_util.c (working copy)
@@ -4895,9 +4895,9 @@
"search for temporary directory failed");
return HTTP_INTERNAL_SERVER_ERROR;
}
- apr_filepath_merge(&template, temp_dir,
- "modproxy.tmp.XXXXXX",
- APR_FILEPATH_NATIVE, p);
+ ap_filepath_merge(&template, temp_dir,
+ "modproxy.tmp.XXXXXX",
+ APR_FILEPATH_NATIVE, p);
status = apr_file_mktemp(&tmpfile, template, 0, p);
if (status != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r,
APLOGNO(01090)
Index: modules/ssl/ssl_ct_util.c
===================================================================
--- modules/ssl/ssl_ct_util.c (revision 1918820)
+++ modules/ssl/ssl_ct_util.c (working copy)
@@ -30,7 +30,7 @@
{
apr_status_t rv;
- rv = apr_filepath_merge(out, dirname, basename, 0, p);
+ rv = ap_filepath_merge(out, dirname, basename, 0, p);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rv, s,
APLOGNO(02776) "can't build filename from %s and %s",
Index: server/config.c
===================================================================
--- server/config.c (revision 1918820)
+++ server/config.c (working copy)
@@ -1597,8 +1597,8 @@
{
char *newpath = NULL;
apr_status_t rv;
- rv = apr_filepath_merge(&newpath, ap_server_root, file,
- APR_FILEPATH_TRUENAME, p);
+ rv = ap_filepath_merge(&newpath, ap_server_root, file,
+ APR_FILEPATH_TRUENAME, p);
if (newpath && (rv == APR_SUCCESS || APR_STATUS_IS_EPATHWILD(rv)
|| APR_STATUS_IS_ENOENT(rv)
|| APR_STATUS_IS_ENOTDIR(rv))) {
@@ -1615,8 +1615,8 @@
apr_status_t rv;
const char *runtime_dir = ap_runtime_dir ? ap_runtime_dir :
ap_server_root_relative(p, DEFAULT_REL_RUNTIMEDIR);
- rv = apr_filepath_merge(&newpath, runtime_dir, file,
- APR_FILEPATH_TRUENAME, p);
+ rv = ap_filepath_merge(&newpath, runtime_dir, file,
+ APR_FILEPATH_TRUENAME, p);
if (newpath && (rv == APR_SUCCESS || APR_STATUS_IS_EPATHWILD(rv)
|| APR_STATUS_IS_ENOENT(rv)
|| APR_STATUS_IS_ENOTDIR(rv))) {
Index: server/core.c
===================================================================
--- server/core.c (revision 1918820)
+++ server/core.c (working copy)
@@ -1694,8 +1694,8 @@
return "DocumentRoot must be a directory";
}
- if (apr_filepath_merge((char**)&conf->ap_document_root, NULL, arg,
- APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS
+ if (ap_filepath_merge((char**)&conf->ap_document_root, NULL, arg,
+ APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS
|| !ap_is_directory(cmd->temp_pool, arg)) {
if (cmd->server->is_virtual) {
ap_log_perror(APLOG_MARK, APLOG_STARTUP, 0,
@@ -2570,8 +2570,8 @@
/*
* Ensure that the pathname is canonical, and append the trailing /
*/
- rv = apr_filepath_merge(&newpath, NULL, cmd->path,
- APR_FILEPATH_TRUENAME, cmd->pool);
+ rv = ap_filepath_merge(&newpath, NULL, cmd->path,
+ APR_FILEPATH_TRUENAME, cmd->pool);
if (rv != APR_SUCCESS && rv != APR_EPATHWILD) {
return apr_pstrcat(cmd->pool, "<Directory \"", cmd->path,
"\"> path is invalid.", NULL);
@@ -2759,8 +2759,8 @@
/* Ensure that the pathname is canonical, but we
* can't test the case/aliases without a fixed path */
- if (apr_filepath_merge(&newpath, "", cmd->path,
- 0, cmd->pool) != APR_SUCCESS)
+ if (ap_filepath_merge(&newpath, "", cmd->path,
+ 0, cmd->pool) != APR_SUCCESS)
return apr_pstrcat(cmd->pool, "<Files \"", cmd->path,
"\"> is invalid.", NULL);
cmd->path = newpath;
@@ -3303,8 +3303,8 @@
return err;
}
- if ((apr_filepath_merge((char**)&ap_server_root, NULL, arg,
- APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
+ if ((ap_filepath_merge((char**)&ap_server_root, NULL, arg,
+ APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
|| !ap_is_directory(cmd->temp_pool, ap_server_root)) {
return "ServerRoot must be a valid directory";
}
@@ -3320,9 +3320,9 @@
return err;
}
- if ((apr_filepath_merge((char**)&ap_runtime_dir, NULL,
- ap_server_root_relative(cmd->temp_pool, arg),
- APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
+ if ((ap_filepath_merge((char**)&ap_runtime_dir, NULL,
+ ap_server_root_relative(cmd->temp_pool, arg),
+ APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
|| !ap_is_directory(cmd->temp_pool, ap_runtime_dir)) {
return "DefaultRuntimeDir must be a valid directory, absolute or
relative to ServerRoot";
}
@@ -3338,9 +3338,9 @@
return err;
}
- if ((apr_filepath_merge((char**)&core_state_dir, NULL,
- ap_server_root_relative(cmd->temp_pool, arg),
- APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
+ if ((ap_filepath_merge((char**)&core_state_dir, NULL,
+ ap_server_root_relative(cmd->temp_pool, arg),
+ APR_FILEPATH_TRUENAME, cmd->pool) != APR_SUCCESS)
|| !ap_is_directory(cmd->temp_pool, core_state_dir)) {
return "DefaultStateDir must be a valid directory, absolute or
relative to ServerRoot";
}
@@ -5055,9 +5055,9 @@
while (*path == '/') {
++path;
}
- if ((rv = apr_filepath_merge(&r->filename, ap_document_root(r), path,
- APR_FILEPATH_TRUENAME
- | APR_FILEPATH_SECUREROOT, r->pool))
+ if ((rv = ap_filepath_merge(&r->filename, ap_document_root(r), path,
+ APR_FILEPATH_TRUENAME
+ | APR_FILEPATH_SECUREROOT, r->pool))
!= APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00127)
"Cannot map %s to file", r->the_request);
@@ -5741,7 +5741,7 @@
? core_state_dir
: ap_server_root_relative(p, DEFAULT_REL_STATEDIR);
- rv = apr_filepath_merge(&newpath, state_dir, file, APR_FILEPATH_TRUENAME,
p);
+ rv = ap_filepath_merge(&newpath, state_dir, file, APR_FILEPATH_TRUENAME,
p);
if (newpath && (rv == APR_SUCCESS || APR_STATUS_IS_EPATHWILD(rv)
|| APR_STATUS_IS_ENOENT(rv)
|| APR_STATUS_IS_ENOTDIR(rv))) {
@@ -6093,12 +6093,8 @@
if (APR_SUCCESS != (rv = check_unc(addpath, p))) {
return rv;
}
-#undef apr_filepath_merge
#endif
return apr_filepath_merge(newpath, rootpath, addpath, flags, p);
-#ifdef WIN32
-#define apr_filepath_merge ap_filepath_merge
-#endif
}
Index: server/mpm/netware/mpm_netware.c
===================================================================
--- server/mpm/netware/mpm_netware.c (revision 1918820)
+++ server/mpm/netware/mpm_netware.c (working copy)
@@ -1134,7 +1134,7 @@
for (i=len; i; i--) {
if (s[i] == '\\' || s[i] == '/') {
s[i] = '\0';
- apr_filepath_merge(&def_server_root, NULL, s,
+ ap_filepath_merge(&def_server_root, NULL, s,
APR_FILEPATH_TRUENAME, process->pool);
break;
}
Index: server/mpm/winnt/mpm_winnt.c
===================================================================
--- server/mpm/winnt/mpm_winnt.c (revision 1918820)
+++ server/mpm/winnt/mpm_winnt.c (working copy)
@@ -1140,8 +1140,8 @@
if (!strcasecmp(def_server_root, "bin"))
*(def_server_root - 1) = '\0';
}
- apr_filepath_merge(&def_server_root, NULL, binpath,
- APR_FILEPATH_TRUENAME, process->pool);
+ ap_filepath_merge(&def_server_root, NULL, binpath,
+ APR_FILEPATH_TRUENAME, process->pool);
/* Use process->pool so that the rewritten argv
* lasts for the lifetime of the server process,
Index: server/request.c
===================================================================
--- server/request.c (revision 1918820)
+++ server/request.c (working copy)
@@ -708,8 +708,8 @@
* so we can begin by checking the cache for a recent directory walk.
* This call will ensure we have an absolute path in the same pass.
*/
- if ((rv = apr_filepath_merge(&entry_dir, NULL, r->filename,
- APR_FILEPATH_NOTRELATIVE, r->pool))
+ if ((rv = ap_filepath_merge(&entry_dir, NULL, r->filename,
+ APR_FILEPATH_NOTRELATIVE, r->pool))
!= APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00030)
"Module bug? Request filename path %s is invalid or "
@@ -898,9 +898,9 @@
*/
if ((r->finfo.filetype == APR_DIR) && r->path_info && *r->path_info)
{
- if ((rv = apr_filepath_merge(&r->path_info, r->filename,
- r->path_info,
- APR_FILEPATH_NOTABOVEROOT, r->pool))
+ if ((rv = ap_filepath_merge(&r->path_info, r->filename,
+ r->path_info,
+ APR_FILEPATH_NOTABOVEROOT, r->pool))
!= APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(00033)
"dir_walk error, path_info %s is not relative "
@@ -2483,8 +2483,8 @@
rnew->canonical_filename = (char*)(1);
}
- if (apr_filepath_merge(&rnew->filename, fdir, new_file,
- APR_FILEPATH_TRUENAME, rnew->pool) != APR_SUCCESS) {
+ if (ap_filepath_merge(&rnew->filename, fdir, new_file,
+ APR_FILEPATH_TRUENAME, rnew->pool) != APR_SUCCESS) {
rnew->status = HTTP_FORBIDDEN;
return rnew;
}
Index: server/util.c
===================================================================
--- server/util.c (revision 1918820)
+++ server/util.c (working copy)
@@ -226,7 +226,7 @@
/* We actually compare the canonical root to this root, (but we don't
* waste time checking the case), since every use of this function in
* httpd-2.1 tests if the path is 'proper', meaning we've already passed
- * it through apr_filepath_merge, or we haven't.
+ * it through ap_filepath_merge, or we haven't.
*/
AP_DECLARE(int) ap_os_is_path_absolute(apr_pool_t *p, const char *dir)
{
Index: server/util_script.c
===================================================================
--- server/util_script.c (revision 1918820)
+++ server/util_script.c (working copy)
@@ -441,7 +441,7 @@
NULL);
#ifdef WIN32
/* We need to make this a real Windows path name */
- apr_filepath_merge(&pt, "", pt, APR_FILEPATH_NATIVE, r->pool);
+ ap_filepath_merge(&pt, "", pt, APR_FILEPATH_NATIVE, r->pool);
#endif
apr_table_setn(e, "PATH_TRANSLATED", pt);
}
