Bug 2526: default ALLOW when no list specified.

The expected behavior of ACL checking should cause an implicit default
deny state to be reached unless a terminating denial causes a state to
flip to allow.

A small logic flaw means that completely explicitly absent access control
list was flipped to ALLOW state.

It is believed that most security controls which have explicitly coded
defaults in ther configuration are not impacted by the bug or its fix.
Only empty delay pools and ICAP re*mods may have any change in behavior
as a result.

diff --git a/src/ACLChecklist.cc b/src/ACLChecklist.cc
index 6ec741400962b7e409af9d8f20e3e6a3dd9625d2..2d0d5808c2cc463d4176c7ad24f7e7a86763fca1 100644 (file)
--- a/src/ACLChecklist.cc
+++ b/src/ACLChecklist.cc
@@ -125,7 +125,7 @@ ACLChecklist::check()
     if (checking())
         return;
 
-    /* deny if no rules present */
+    /** Deny if no rules present. */
     currentAnswer(ACCESS_DENIED);
 
     if (callerGone()) {
@@ -133,11 +133,21 @@ ACLChecklist::check()
         return;
     }
 
+    /** The ACL List should NEVER be NULL when calling this method.
+     * Always caller should check for NULL and handle appropriate to its needs first.
+     * We cannot select a sensible default for all callers here. */
+    if (accessList == NULL) {
+        debugs(28, DBG_CRITICAL, "SECURITY ERROR: ACL " << this << " checked with nothing to match against!!");
+        currentAnswer(ACCESS_DENIED);
+        checkCallback(currentAnswer());
+        return;
+    }
+
     /* NOTE: This holds a cbdata reference to the current access_list
      * entry, not the whole list.
      */
     while (accessList != NULL) {
-        /*
+        /** \par
          * If the _acl_access is no longer valid (i.e. its been
          * freed because of a reconfigure), then bail on this
          * access check.  For now, return ACCESS_DENIED.
@@ -158,9 +168,8 @@ ACLChecklist::check()
         }
 
         if (finished()) {
-            /*
-             * We are done.  Either the request
-             * is allowed, denied, requires authentication.
+            /** \par
+             * Either the request is allowed, denied, requires authentication.
              */
             debugs(28, 3, "ACLChecklist::check: " << this << " match found, calling back with " << currentAnswer());
             cbdataReferenceDone(accessList); /* A */
@@ -181,9 +190,8 @@ ACLChecklist::check()
         cbdataReferenceDone(A);
     }
 
-    /* dropped off the end of the list */
-    debugs(28, 3, "ACLChecklist::check: " << this <<
-           " NO match found, returning " <<
+    /** If dropped off the end of the list return inversion of last line allow/deny action. */
+    debugs(28, 3, HERE << this << " NO match found, returning " <<
            (currentAnswer() != ACCESS_DENIED ? ACCESS_DENIED : ACCESS_ALLOWED));
 
     checkCallback(currentAnswer() != ACCESS_DENIED ? ACCESS_DENIED : ACCESS_ALLOWED);