From: Amos Jeffries <squid3@treenet.co.nz>
Date: Sun, 18 Jan 2015 11:02:13 +0000 (-0800)
Subject: Bug 3997: Excessive NTLM or Negotiate auth helper annotations
X-Git-Tag: SQUID_3_4_12~8
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=01b28d5d43c2b0d2afe57dcc10e84c79beb91167;p=thirdparty%2Fsquid.git

Bug 3997: Excessive NTLM or Negotiate auth helper annotations

With the transaction annotations feature added in Squid-3.4 auth
helper response values get recorded as annotatiions. In the case
of NTLM and Negotiate authentication the helper response contains
a large credentials token which changes frequently.

Also, user credentials state is cached. In the case of NTLM and
Negotiate the active credentials are cached in the TCP connection
state data, but also for the cache mgr helper reports make use of
caching in a global username cache.

When these two features are combined, the global username cache
for mgr reporting accumulates all TCP connection specific
token= values presented by the client on all its connections, and
any changes to the token over its lifetime.

The result is that for users performing either many transactions,
or staying connected for long periods the memory consumption from
unnecesarily stored tokens is excessive. When clients do both the
machine memory can be consumed, and the CPU can reach 100%
consumption just walking the annotations lists during regular
operations.

To fix this we drop the security credentials tokens from cached
annotations list in NTLM and Negotiate. Digest is also included
though its HA1 token value is static it has similar privacy issues
related to storage.
---

diff --git a/src/Notes.cc b/src/Notes.cc
index 00039565f6..13d530e2ee 100644
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -188,6 +188,21 @@ NotePairs::add(const char *key, const char *note)
     entries.push_back(new NotePairs::Entry(key, note));
 }
 
+void
+NotePairs::remove(const char *key)
+{
+    Vector<NotePairs::Entry *>::iterator i = entries.begin();
+    while (i != entries.end()) {
+        if ((*i)->name.cmp(key) == 0) {
+            NotePairs::Entry *e = (*i);
+            entries.prune(e);
+            delete e;
+            i = entries.begin(); // vector changed underneath us
+        } else
+            ++i;
+    }
+}
+
 void
 NotePairs::addStrList(const char *key, const char *values)
 {
diff --git a/src/Notes.h b/src/Notes.h
index 47950d4c88..401c8b9162 100644
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -154,6 +154,11 @@ public:
      */
     void add(const char *key, const char *value);
 
+    /**
+     * Remove all notes with a given key.
+     */
+    void remove(const char *key);
+
     /**
      * Adds a note key and values strList to the notes list.
      * If the key name already exists in list, add the new values to its set
diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index f625bd8648..9107d739f6 100644
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -298,6 +298,8 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
     auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    // remove any private credentials detail which got added.
+    auth_user_request->user()->notes.remove("ha1");
 
     static bool oldHelperWarningDone = false;
     switch (reply.result) {
diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 086da9f7b9..64cffc98c6 100644
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -229,6 +229,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
     auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    // remove any private credentials detail which got added.
+    auth_user_request->user()->notes.remove("token");
 
     Auth::Negotiate::UserRequest *lm_request = dynamic_cast<Auth::Negotiate::UserRequest *>(auth_user_request.getRaw());
     assert(lm_request != NULL);
diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 4ce04ebf41..ebfe8952ac 100644
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -223,6 +223,8 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
     // add new helper kv-pair notes to the credentials object
     // so that any transaction using those credentials can access them
     auth_user_request->user()->notes.appendNewOnly(&reply.notes);
+    // remove any private credentials detail which got added.
+    auth_user_request->user()->notes.remove("token");
 
     Auth::Ntlm::UserRequest *lm_request = dynamic_cast<Auth::Ntlm::UserRequest *>(auth_user_request.getRaw());
     assert(lm_request != NULL);