You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rp...@apache.org on 2021/10/08 10:49:07 UTC
svn commit: r1894024 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number modules/mappers/mod_alias.c
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.
Modified:
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/modules/mappers/mod_alias.c
Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1894024&r1=1894023&r2=1894024&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Oct 8 10:49:06 2021
@@ -1 +1 @@
-10297
+10298
Modified: httpd/httpd/trunk/modules/mappers/mod_alias.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_alias.c?rev=1894024&r1=1894023&r2=1894024&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/mappers/mod_alias.c (original)
+++ httpd/httpd/trunk/modules/mappers/mod_alias.c Fri Oct 8 10:49:06 2021
@@ -67,8 +67,10 @@ typedef struct {
module AP_MODULE_DECLARE_DATA alias_module;
-static char magic_error_value;
-#define PREGSUB_ERROR (&magic_error_value)
+static char magic_pregsub_error_value;
+#define PREGSUB_ERROR (&magic_pregsub_error_value)
+static char magic_merge_error_value;
+#define MERGE_ERROR (&magic_merge_error_value)
static void *create_alias_config(apr_pool_t *p, server_rec *s)
{
@@ -500,6 +502,7 @@ static char *try_alias_list(request_rec
alias_entry *entries = (alias_entry *) aliases->elts;
ap_regmatch_t regm[AP_MAX_REG_MATCH];
char *found = NULL;
+ int canon = 1;
int i;
for (i = 0; i < aliases->nelts; ++i) {
@@ -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) {
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);
}
if (found) {
@@ -596,7 +632,7 @@ static int translate_alias_redir(request
if ((ret = try_redirect(r, &status)) != NULL
|| (ret = try_alias_list(r, serverconf->redirects, 1, &status))
!= NULL) {
- if (ret == PREGSUB_ERROR)
+ if ((ret == PREGSUB_ERROR) || (ret == MERGE_ERROR))
return HTTP_INTERNAL_SERVER_ERROR;
if (ap_is_HTTP_REDIRECT(status)) {
alias_dir_conf *dirconf = (alias_dir_conf *)
@@ -653,7 +689,7 @@ static int fixup_redir(request_rec *r)
if ((ret = try_redirect(r, &status)) != NULL
|| (ret = try_alias_list(r, dirconf->redirects, 1, &status))
!= NULL) {
- if (ret == PREGSUB_ERROR)
+ if ((ret == PREGSUB_ERROR) || (ret == MERGE_ERROR))
return HTTP_INTERNAL_SERVER_ERROR;
if (ap_is_HTTP_REDIRECT(status)) {
if (dirconf->allow_relative != ALIAS_FLAG_ON || ret[0] != '/') {
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
Re: svn commit: r1894024 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
modules/mappers/mod_alias.c
Posted by Yann Ylavic <yl...@gmail.com>.
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.