From: William A. Rowe Jr Date: Fri, 10 Nov 2000 15:29:07 +0000 (+0000) Subject: Killing ap_os_is_filename_valid. Left actual win32 code, since it is X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=29d125fae4abad1e6a4a44e13aa860af25057d32;p=apache Killing ap_os_is_filename_valid. Left actual win32 code, since it is moving into apr and the check_safe_file call. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@86906 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/modules/http/http_request.c b/modules/http/http_request.c index 8c4adffad6..7c58335e1c 100644 --- a/modules/http/http_request.c +++ b/modules/http/http_request.c @@ -261,24 +261,17 @@ static int get_path_info(request_rec *r) while (cp > path) { /* See if the pathname ending here exists... */ - *cp = '\0'; - /* We must not stat() filenames that may cause os-specific system - * problems, such as "/file/aux" on DOS-abused filesystems. - * So pretend that they do not exist by returning an ENOENT error. - * This will force us to drop that part of the path and keep - * looking back for a "real" file that exists, while still allowing - * the "invalid" path parts within the PATH_INFO. - */ - if (!ap_os_is_filename_valid(path)) { - errno = ENOENT; - rv = -1; - } - else { - errno = 0; - rv = apr_stat(&r->finfo, path, r->pool); - } + /* ### We no longer need the test ap_os_is_filename_valid() here + * since apr_stat isn't a posix thing - it's apr_stat's responsibility + * to handle whatever path string arrives at it's door - by platform + * and volume restrictions as applicable... + * TODO: This code becomes even simpler if apr_stat grows + * an APR_PATHINCOMPLETE result to indicate that we are staring at + * an partial virtual root. Only OS2/Win32/Netware need apply it :-) + */ + rv = apr_stat(&r->finfo, path, r->pool); if (cp != end) *cp = '/'; @@ -299,12 +292,7 @@ static int get_path_info(request_rec *r) *cp = '\0'; return OK; } - /* must set this to zero, some stat()s may have corrupted it - * even if they returned an error. - */ - r->finfo.protection = 0; -#if defined(APR_ENOENT) && defined(APR_ENOTDIR) if (APR_STATUS_IS_ENOENT(rv) || APR_STATUS_IS_ENOTDIR(rv)) { last_cp = cp; @@ -315,40 +303,15 @@ static int get_path_info(request_rec *r) --cp; } else { -#if defined(APR_EACCES) if (APR_STATUS_IS_EACCES(rv)) -#endif ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, - "access to %s failed", r->uri); + "access to %s denied", r->uri); + else + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, + "access to %s failed", r->uri); return HTTP_FORBIDDEN; } -#else - /* XXX: WARNING - APR broke this security exception! - */ - -#error APR_ENOENT || APR_ENOTDIR not defined; please see the -#error comments at this line in the source for a workaround. - /* - * If ENOENT || ENOTDIR is not defined in one of the your OS's - * include files, Apache does not know how to check to see why the - * stat() of the index file failed; there are cases where it can fail - * even though the file exists. This means that it is possible for - * someone to get a directory listing of a directory even though - * there is an index (eg. index.html) file in it. If you do not have - * a problem with this, delete the above #error lines and start the - * compile again. If you need to do this, please submit a bug report - * from http://www.apache.org/bug_report.html letting us know that - * you needed to do this. Please be sure to include the operating - * system you are using. - */ last_cp = cp; - - while (--cp > path && *cp != '/') - continue; - - while (cp > path && cp[-1] == '/') - --cp; -#endif /* APR_ENOENT && APR_ENOTDIR */ } return OK; } @@ -433,26 +396,33 @@ static int directory_walk(request_rec *r) return OK; } + /* XXX This needs to be rolled into APR, the APR function will not + * be allowed to fold the case of any non-existant segment of the path: + */ r->filename = ap_os_case_canonical_filename(r->pool, r->filename); + /* TODO This is rather silly right here, we should simply be setting + * filename and path_info at the end of our directory_walk + */ res = get_path_info(r); if (res != OK) { return res; } + /* XXX This becomes mute, and will already happen above for elements + * that actually exist: + */ r->filename = ap_os_canonical_filename(r->pool, r->filename); test_filename = apr_pstrdup(r->pool, r->filename); + /* XXX This becomes mute, since the APR canonical parsing will handle + * 2slash and dot directory issues: + */ ap_no2slash(test_filename); num_dirs = ap_count_dirs(test_filename); - if (!ap_os_is_filename_valid(r->filename)) { - ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r, - "Filename is not valid: %s", r->filename); - return HTTP_FORBIDDEN; - } - + /* XXX This needs to be rolled into APR: */ if ((res = check_safe_file(r))) { return res; } @@ -473,6 +443,11 @@ static int directory_walk(request_rec *r) */ test_dirname = apr_palloc(r->pool, test_filename_len + 2); + /* XXX These exception cases go away if apr_stat() returns the + * APR_PATHINCOMPLETE status, so we skip hard filesystem testing + * of the initial 'pseudo' elements: + */ + #if defined(HAVE_UNC_PATHS) /* If the name is a UNC name, then do not perform any true file test * against the machine name (start at //machine/share/) diff --git a/os/beos/os.h b/os/beos/os.h index 618ff52f92..a42ac10ebe 100644 --- a/os/beos/os.h +++ b/os/beos/os.h @@ -75,6 +75,5 @@ extern int ap_os_is_path_absolute(const char *file); #define ap_os_canonical_filename(p,f) (f) #define ap_os_case_canonical_filename(p,f) (f) #define ap_os_systemcase_filename(p,f) (f) -#define ap_os_is_filename_valid(f) (1) #endif /* !APACHE_OS_H */ diff --git a/os/bs2000/os.h b/os/bs2000/os.h index 4ed873fcaa..69feb130e6 100644 --- a/os/bs2000/os.h +++ b/os/bs2000/os.h @@ -87,7 +87,6 @@ extern int ap_os_is_path_absolute(const char *file); /* Other ap_os_ routines not used by this platform */ -#define ap_os_is_filename_valid(f) (1) #if !defined(_POSIX_SOURCE) && !defined(_XOPEN_SOURCE) typedef struct { diff --git a/os/os2/os.h b/os/os2/os.h index 9cb02be796..d0b49d7345 100644 --- a/os/os2/os.h +++ b/os/os2/os.h @@ -93,8 +93,6 @@ extern int ap_os_is_path_absolute(const char *file); char *ap_os_canonical_filename(apr_pool_t *p, const char *file); char *ap_os_case_canonical_filename(apr_pool_t *p, const char *szFile); char *ap_os_systemcase_filename(apr_pool_t *p, const char *szFile); -/* FIXME: the following should be implemented on this platform */ -#define ap_os_is_filename_valid(f) (1) /* OS/2 doesn't have symlinks so S_ISLNK is always false */ #define S_ISLNK(m) 0 diff --git a/os/tpf/os.h b/os/tpf/os.h index d3b66405c4..70fcb5ef9b 100644 --- a/os/tpf/os.h +++ b/os/tpf/os.h @@ -88,9 +88,6 @@ extern int ap_os_is_path_absolute(const char *f); #endif -/* Other ap_os_ routines not used by this platform */ - -#define ap_os_is_filename_valid(f) (1) /* Sorry if this is ugly, but the include order doesn't allow me * to use request_rec here... */ diff --git a/os/unix/os.h b/os/unix/os.h index e6094b9612..75098277b6 100644 --- a/os/unix/os.h +++ b/os/unix/os.h @@ -132,13 +132,4 @@ extern int ap_os_is_path_absolute(const char *file); */ #define ap_os_systemcase_filename(p,f) (f) -/** - * Tests for validity of a filename on the current platform. The tests for - * validity are operating system specific - * @param f The file to test - * @return 1 If the filename is valid, 0 otherwise - * @deffunc int ap_os_is_filename_valid(const char *file) - */ -#define ap_os_is_filename_valid(f) (1) - #endif /* !APACHE_OS_H */ diff --git a/os/win32/os.h b/os/win32/os.h index c362590a63..da3a167c9b 100644 --- a/os/win32/os.h +++ b/os/win32/os.h @@ -128,7 +128,6 @@ __inline int ap_os_is_path_absolute(const char *file) AP_DECLARE(char *) ap_os_canonical_filename(apr_pool_t *p, const char *file); AP_DECLARE(char *) ap_os_case_canonical_filename(apr_pool_t *pPool, const char *szFile); AP_DECLARE(char *) ap_os_systemcase_filename(apr_pool_t *pPool, const char *szFile); -AP_DECLARE(int) ap_os_is_filename_valid(const char *file); typedef void thread; typedef void event; diff --git a/os/win32/util_win32.c b/os/win32/util_win32.c index c958d27770..27cae11155 100644 --- a/os/win32/util_win32.c +++ b/os/win32/util_win32.c @@ -361,7 +361,12 @@ AP_DECLARE(char *) ap_os_canonical_filename(apr_pool_t *pPool, const char *szFil return pNewName; } -/* +#ifdef NEVER_SINCE_THESE_TESTS_ARE_MOVING +/* + * XXX we will no longer use this redunant parsing function, it's + * logic moves off into the canonical filename processing and the + * apr file handling functions. Left for today till it's finished. + * * ap_os_is_filename_valid is given a filename, and returns 0 if the filename * is not valid for use on this system. On Windows, this means it fails any * of the tests below. Otherwise returns 1. @@ -387,9 +392,8 @@ AP_DECLARE(char *) ap_os_canonical_filename(apr_pool_t *pPool, const char *szFil * * 4) any segment in which the basename (before first period) matches * one of the DOS device names - * (the list comes from KB article Q100108 although some people - * reports that additional names such as "COM5" are also special - * devices). + * (the list comes from KB article Q100108 although additional + * names such as "COM5" are also special devices). * * If the path fails ANY of these tests, the result must be to deny access. */ @@ -501,6 +505,7 @@ AP_DECLARE(int) ap_os_is_filename_valid(const char *file) return 1; } +#endif AP_DECLARE(apr_status_t) ap_os_create_privileged_process(const request_rec *r, apr_proc_t *newproc, const char *progname, diff --git a/server/util.c b/server/util.c index 1bd2e355ee..986c9dde71 100644 --- a/server/util.c +++ b/server/util.c @@ -894,12 +894,12 @@ AP_DECLARE(apr_status_t) ap_pcfg_openfile(configfile_t **ret_cfg, apr_pool_t *p, return APR_EBADF; } - if (!ap_os_is_filename_valid(name)) { - ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_NOERRNO, 0, NULL, - "Access to config file %s denied: not a valid filename", - name); - return APR_EACCES; - } + /* ### We no longer need the test ap_os_is_filename_valid() here + * The directory was already walked on a segment by segment basis, + * so we should never be called with a bad path element, and device + * names as access file names never posed as security threats, since + * it was the admin's choice to assign the .htaccess file's name. + */ status = apr_open(&file, name, APR_READ | APR_BUFFERED, APR_OS_DEFAULT, p); #ifdef DEBUG