On 10/8/21 5:21 PM, Yann Ylavic wrote:
> On Fri, Oct 8, 2021 at 12:49 PM 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
