On Sat, Jun 24, 2017 at 12:49 AM, <gsm...@apache.org> wrote:
> Author: gsmith
> Date: Sat Jun 24 05:49:45 2017
> New Revision: 1799731
>
> URL: http://svn.apache.org/viewvc?rev=1799731&view=rev
> Log:
> Send a 404 response like other OSs do instead of 403 on Windows when
> a path segment or file requested uses a reserved word so Windows
> cannot be fingerprinted. PR55887
How does this solve fingerprinting?
This patch was ill-conceived... -1 as explained below;
> +#ifdef _WIN32
> + /* Windows has a number of reserved words that cannot be used
> + * as a file or directory name so thisinfo.filetype will
> + * always be != APR_DIR. Don't allow us be fingerprinted with
> + * a 403 and instead send a 404 like other OSs would. PR55887
> + */
> + preg = ap_pregcomp(r->pool,
> +
> "/(aux|con|com[1-9]|lpt[1-9]|nul|prn)"
> + "($|/|.)",
> AP_REG_EXTENDED | AP_REG_ICASE);
> + if (ap_regexec(preg, r->uri, 0, NULL, 0) == 0)
> + return r->status = HTTP_NOT_FOUND;
> +#endif
You should be aware that this code path can be hit a number of times
per request, often hundreds for an autoindex listing, etc. You should
see a notable rise in cpu.
As suggested this could be a compile-once and recycle pattern.
But why a regex against the URI?!? That's horrible, you are reparsing
all those leading bytes we already parsed. Part of the URI may have
been alias-mapped away from one resource name to another (or in
the htaccess case, mapped into a badly named path.) r->filename
is the cumulative name of the *FILE* we are inspecting, not *URI*.
Worse still, because seg_name is the actual path component we are
now looking at, e.g. from /path/to/lpt1/foo - if we are at the third elt
that string becomes the normalized form of lpt1. There was no reason
to be reinspecting the earlier path elts throughout the segment walk!
But this could all be identified by the APR_CHR ->filetype, no? Sure
beats an unnecessarily long string pattern match.
While we are at it, why even forking WIN32? If you want to prevent
APR_CHR files on Windows (or netware or os2 or...) from being
identified, why wouldn't you simply change the behavior across all
architectures with a new test case ahead of the != APR_DIR check...
else if (thisinfo.filetype == APR_CHR) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00038)
"Forbidden: %s points to a char stream path",
r->filename);
return r->status = HTTP_NOT_FOUND;
}
else if (thisinfo.filetype != APR_DIR) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00038)
"Forbidden: %s doesn't point to "
"a file or directory",
r->filename);
return r->status = HTTP_FORBIDDEN;
}
As I asked upfront, why is it believed that this solves the win32
issue? It's trivial to check for case folding and that folding will
occur in a manner unique to the Windows implementation of the
NTFS filesystem. You would need to modify the following code
to force case mismatches to a 404 result to try to convince anyone
that the server is running on unix, non-samba;
if ((thisinfo.valid & APR_FINFO_NAME)
&& strcmp(seg_name, thisinfo.name)) {
/* TODO: provide users an option that an internal/external
* redirect is required here? We need to walk the URI and
* filename in tandem to properly correlate these.
*/
strcpy(seg_name, thisinfo.name);
filename_len = strlen(r->filename);
}
More to the point, the actual TCP traffic itself is subject to all sorts
of inferences if it can be observed (and that this isn't a proxied box
behind the firewall.)
Let's please back out that patch entirely and start again by asking
the question, do we want to treat CHR file paths as not found rather
than forbidden, and does anyone see an opportunity for httpd's
core or third party modules to proceed to try to work around that
file/path leading to potential security blunders? E.g. mod_speling?
If we agree that APR_CHR can be 'anonymized' then a generic
and fast fix is in order.
p.s. Next time we try to deoptimize the code this badly, let's add a
config option so the admin chooses to spend such extra cycles?
