On Fri, Oct 8, 2021 at 12:49 PM <rpl...@apache.org> wrote:
>
> Author: rpluem
> Date: Fri Oct 8 10:49:06 2021
> New Revision: 1894024
>
> URL: http://svn.apache.org/viewvc?rev=1894024&view=rev
> Log:
> * Make aliases more robust against potential traversal attacks, by using
> apr_filepath_merge to merge the real path and the remainder of the fake
> path like we do in the same situation for resources mapped by
> DocumentRoot.
>
[]
>
> --- httpd/httpd/trunk/modules/mappers/mod_alias.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_alias.c Fri Oct 8 10:49:06 2021
[]
> @@ -553,8 +556,41 @@ static char *try_alias_list(request_rec
>
> found = apr_pstrcat(r->pool, alias->real, escurl, NULL);
> }
> - else
> + else if (is_redir) {
This is dead code because the first "if" tests for that already,
should this block be removed then?
> found = apr_pstrcat(r->pool, alias->real, r->uri + l,
> NULL);
> + }
> + else {
> + apr_status_t rv;
> + char *fake = r->uri + l;
> +
> + /*
> + * For the apr_filepath_merge below we need a relative
> path
> + * Hence skip all leading '/'
> + */
> + while (*fake == '/') {
> + fake++;
> + }
> +
> + /* 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))
> + != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> APLOGNO(10297)
> + "Cannot map %s to file",
> r->the_request);
> + return MERGE_ERROR;
> + }
> + canon = 0;
> + }
> + else {
> + /*
> + * r->uri + l might be either pointing to \0 or to a
> + * string full of '/'s. Hence we need to cat.
> + */
> + found = apr_pstrcat(r->pool, alias->real, r->uri +
> l, NULL);
> + }
> + }
> }
> }
>
> @@ -568,7 +604,7 @@ static char *try_alias_list(request_rec
> * canonicalized. After I finish eliminating os canonical.
> * Better fail test for ap_server_root_relative needed here.
> */
> - if (!is_redir) {
> + if (!is_redir && canon) {
> found = ap_server_root_relative(r->pool, found);
I wonder if we shouldn't add APR_FILEPATH_NOTABOVEROOT in
ap_server_root_relative() too.
Not that it helps here after your changes, but since we are on robustness..
> }
Regards;
Yann.
