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; > } > }
