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

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.
+        /* ### 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 :-)
          */
-         if (!ap_os_is_filename_valid(path)) {
-             errno = ENOENT;
-             rv = -1;
-         }
-         else {
-             errno = 0;
         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 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/)

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 */

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 {           

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

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... */

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 */

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;

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,

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