From dd2b2af1cb5ee87f637ea1a4a10b52dad4fc330f Mon Sep 17 00:00:00 2001
From: Amos Jeffries <squid3@treenet.co.nz>
Date: Mon, 1 Dec 2008 23:39:25 +1300
Subject: [PATCH] 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 are expected to have any change
in behavior as a result.
---
 src/ACLChecklist.cc | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/ACLChecklist.cc b/src/ACLChecklist.cc
index a59170069b..e16a12745f 100644
--- 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, 0, "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);
-- 
2.47.2