On 10/8/21 5:21 PM, Yann Ylavic wrote:
> 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?
Good catch. r1894034.
>
>> 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 sure. I guess ap_server_root_relative() origins from a different use case
processing more trusted data like in configuration
files where /../ going above the root might be fine. Furthermore it does not
help if the path given to ap_server_root_relative is
absolute and the path itself does not go beyond root. e.g
/www/docs/../../etc/passwd would not create any failure.
Regards
Rüdiger
