Re: [PATCH] Make "Symbolic link not allowed" log message more verbose

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

We actually made a provision in APR that I really wish HTTPD would adopt
for their apr_status_t codes.

The range beginning APR_OS_START_USERERR are reserved exclusively
for anything the application wants to do (see srclib/apr/include/apr_errno.h.)

It sort of points out a problem however; we (APR) really should have made
some 'registration' provision, such that multiple libraries and the base app
could all reserve a 'chunk' of error codes for their use.  With that reg should
either come a string array of error message texts, or a callback to provide
those messages.

Anyway, I love the spirit of your patch, but think we probably should have
passed enough context into resolve_symlink for it to log the error message
itself.  That would make directory_walk much easier to read, if only the
dir_walk would quit doing so much :-)

If no one else feels like massaging it that way, I'll do so a few weeks out
after 2.0.45 has flown the coup.

Bill

At 04:04 AM 2/17/2003, Andrew Eland wrote:

>Hi,
>
>After spending some time wondering why a particular symbolic link wasn't
>allowed, I made a patch (against 2.0.44, server/request.c) that adds one
>of the following reasons to the "Symbolic link not allowed" message:
>* FollowSymLinks option not enabled
>* owner doesn't match
>* couldn't stat file
>
>I hope somebody thinks it's useful.
>
>  -- Andrew Eland (http://www.andreweland.org)
>
>--- request-old.c       Sat Feb  8 16:55:49 2003
>+++ request.c   Sat Feb  8 18:17:54 2003
>@@ -378,24 +378,34 @@
>  * we start off with an lstat().  Every lstat() must be dereferenced in case
>  * it points at a 'nasty' - we must always rerun check_safe_file (or similar.)
>  */
>-static int resolve_symlink(char *d, apr_finfo_t *lfi, int opts, apr_pool_t *p)
>+typedef enum {
>+    SYMLINK_OK,
>+    SYMLINK_NOT_ENABLED,
>+    SYMLINK_STAT_FAILED,
>+    SYMLINK_OWNER_DOES_NOT_MATCH,
>+} resolve_symlink_result;
>+
>+static resolve_symlink_result resolve_symlink(char *d,
>+                                              apr_finfo_t *lfi,
>+                                              int opts,
>+                                              apr_pool_t *p)
> {
>     apr_finfo_t fi;
>     int res;
>     const char *savename;
>
>     if (!(opts & (OPT_SYM_OWNER | OPT_SYM_LINKS))) {
>-        return HTTP_FORBIDDEN;
>+        return SYMLINK_NOT_ENABLED;
>     }
>
>     /* Save the name from the valid bits. */
>     savename = (lfi->valid & APR_FINFO_NAME) ? lfi->name : NULL;
>
>     if (opts & OPT_SYM_LINKS) {
>-        if ((res = apr_stat(&fi, d, lfi->valid & ~(APR_FINFO_NAME
>-                                                 | APR_FINFO_LINK), p))
>-                 != APR_SUCCESS) {
>-            return HTTP_FORBIDDEN;
>+        if ((res = apr_stat(&fi, d, lfi->valid & ~(APR_FINFO_NAME
>+                                                   | APR_FINFO_LINK), p))
>+            != APR_SUCCESS) {
>+            return SYMLINK_STAT_FAILED;
>         }
>
>         /* Give back the target */
>@@ -405,7 +415,7 @@
>             lfi->valid |= APR_FINFO_NAME;
>         }
>
>-        return OK;
>+        return SYMLINK_OK;
>     }
>
>     /* OPT_SYM_OWNER only works if we can get the owner of
>@@ -415,17 +425,17 @@
>     if (!(lfi->valid & APR_FINFO_OWNER)) {
>         if ((res = apr_lstat(&fi, d, lfi->valid | APR_FINFO_OWNER, p))
>             != APR_SUCCESS) {
>-            return HTTP_FORBIDDEN;
>+        return SYMLINK_STAT_FAILED;
>         }
>     }
>
>     if ((res = apr_stat(&fi, d, lfi->valid & ~(APR_FINFO_NAME), p))
>         != APR_SUCCESS) {
>-        return HTTP_FORBIDDEN;
>+        return SYMLINK_STAT_FAILED;
>     }
>
>     if (apr_compare_users(fi.user, lfi->user) != APR_SUCCESS) {
>-        return HTTP_FORBIDDEN;
>+        return SYMLINK_OWNER_DOES_NOT_MATCH;
>     }
>
>     /* Give back the target */
>@@ -435,9 +445,28 @@
>         lfi->valid |= APR_FINFO_NAME;
>     }
>
>-    return OK;
>+    return SYMLINK_OK;
> }
>
>+static void log_resolve_symlink_result(resolve_symlink_result result,
>+                                       const request_rec *r)
>+{
>+    const char *reason="";
>+    switch(result) {
>+    case SYMLINK_NOT_ENABLED:
>+        reason=" (FollowSymLinks option not enabled)";
>+        break;
>+    case SYMLINK_STAT_FAILED:
>+        reason=" (couldn't stat file)";
>+        break;
>+    case SYMLINK_OWNER_DOES_NOT_MATCH:
>+        reason=" (owner doesn't match)";
>+        break;
>+    }
>+    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>+                  "Symbolic link not allowed: %s%s",
>+                  r->filename, reason);
>+}
>
> /*
>  * As we walk the directory configuration, the merged config won't
>@@ -1017,12 +1046,11 @@
>             if (thisinfo.filetype == APR_LNK) {
>                 /* Is this a possibly acceptable symlink?
>                  */
>+                resolve_symlink_result res;
>                 if ((res = resolve_symlink(r->filename, &thisinfo,
>                                            opts.opts, r->pool)) != OK) {
>-                    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>-                                  "Symbolic link not allowed: %s",
>-                                  r->filename);
>-                    return r->status = res;
>+                    log_resolve_symlink_result(res, r);
>+                    return r->status = HTTP_FORBIDDEN;
>                 }
>             }
>
>@@ -1731,10 +1759,11 @@
>         /*
>          * Resolve this symlink.  We should tie this back to dir_walk's cache
>          */
>+        resolve_symlink_result res;
>         if ((res = resolve_symlink(rnew->filename, &rnew->finfo,
>                                    ap_allow_options(rnew), rnew->pool))
>             != OK) {
>-            rnew->status = res;
>+            rnew->status = (res == SYMLINK_OK ? OK : HTTP_FORBIDDEN);
>             return rnew;
>         }
>     }