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 00039565f6ad47befcf128585bb043c554db25bf..13d530e2ee64a6ed7e4a206af0dd71dfb09feac8 100644 (file)
--- 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 47950d4c88cb261a0983490e8589d8b50e798251..401c8b9162bdb0e8f91e07ba5e2ea164dee6932e 100644 (file)
--- 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 f625bd864842b770be1ffc1c6c3d0edc451f6283..9107d739f6f0e4fb042e6a00a131d5571d50dbc8 100644 (file)
--- 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 086da9f7b9251ce777837443094f10333130f930..64cffc98c6dfdd29d6f5947399f3b80021a4ae34 100644 (file)
--- 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 4ce04ebf41f3d7a091ac32c6a1c3a9023e4a4986..ebfe8952ac7bc4316f64fe2fa8cc67d0d954244e 100644 (file)
--- 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);