You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2021/10/08 15:21:27 UTC
Re: svn commit: r1894024 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
modules/mappers/mod_alias.c
On Fri, Oct 8, 2021 at 12:49 PM <rp...@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.
Re: svn commit: r1894024 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number modules/mappers/mod_alias.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/8/21 5:21 PM, Yann Ylavic wrote:
> On Fri, Oct 8, 2021 at 12:49 PM <rp...@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