Negotiate Kerberos authentication request size exceeds output buffer size.

Despite the "must match" comment, MAX_AUTHTOKEN_LEN in
auth/UserRequest.h got out of sync with similar constants in Negotiate helpers.
A 32KB buffer cannot fit some helper requests (e.g., those carrying Privilege
Account Certificate information in the client's Kerberos ticket). Each truncated
request blocks the negotiate helper channel, eventually causing helper queue
overflow and possibly killing Squid.

This patch increases MAX_AUTHTOKEN_LEN in UserRequest.h to 65535 which
is also the maximum used by the negotiate helpers. The patch also adds checks
to avoid sending truncated requests, treating them as helper errors instead.

This is a Measurement Factory project.

diff --git a/src/auth/UserRequest.h b/src/auth/UserRequest.h
index 88460206be8cb0c43d47994cd21b813fcb3d7c67..b81d907c392e03156375267b917b7b0059208cd6 100644 (file)
--- a/src/auth/UserRequest.h
+++ b/src/auth/UserRequest.h
@@ -27,8 +27,8 @@ class HttpRequest;
 /**
  * Maximum length (buffer size) for token strings.
  */
-// AYJ: must match re-definition in helpers/negotiate_auth/kerberos/negotiate_kerb_auth.cc
-#define MAX_AUTHTOKEN_LEN   32768
+// XXX: Keep in sync with all others: bzr grep 'define MAX_AUTHTOKEN_LEN'
+#define MAX_AUTHTOKEN_LEN   65535
 
 /**
  * Node used to link an IP address to some user credentials

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 4fa282786f9dad8c9fbfa867c085e5f333694142..d24fa5acc2938e46d5390fbfcf7a224b89a35173 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -68,11 +68,20 @@ const char *
 Auth::Negotiate::UserRequest::credentialsStr()
 {
     static char buf[MAX_AUTHTOKEN_LEN];
+    int printResult = 0;
     if (user()->credentials() == Auth::Pending) {
-        snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
+        printResult = snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
     } else {
-        snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
+        printResult = snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
     }
+
+    // truncation is OK because we are used only for logging
+    if (printResult < 0) {
+        debugs(29, 2, "Can not build negotiate authentication credentials.");
+        buf[0] = '\0';
+    } else if (printResult >= (int)sizeof(buf))
+        debugs(29, 2, "Negotiate authentication credentials truncated.");
+
     return buf;
 }
 
@@ -125,16 +134,26 @@ Auth::Negotiate::UserRequest::startHelperLookup(HttpRequest *req, AccessLogEntry
     debugs(29, 8, HERE << "credentials state is '" << user()->credentials() << "'");
 
     const char *keyExtras = helperRequestKeyExtras(request, al);
+    int printResult = 0;
     if (user()->credentials() == Auth::Pending) {
         if (keyExtras)
-            snprintf(buf, sizeof(buf), "YR %s %s\n", client_blob, keyExtras);
+            printResult = snprintf(buf, sizeof(buf), "YR %s %s\n", client_blob, keyExtras);
         else
-            snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
+            printResult = snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
     } else {
         if (keyExtras)
-            snprintf(buf, sizeof(buf), "KK %s %s\n", client_blob, keyExtras);
+            printResult = snprintf(buf, sizeof(buf), "KK %s %s\n", client_blob, keyExtras);
         else
-            snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
+            printResult = snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
+    }
+
+    if (printResult < 0 || printResult >= (int)sizeof(buf)) {
+        if (printResult < 0)
+            debugs(29, DBG_CRITICAL, "ERROR: Can not build negotiate authentication helper request");
+        else
+            debugs(29, DBG_CRITICAL, "ERROR: Negotiate authentication helper request too big for the " << sizeof(buf) << "-byte buffer");
+        handler(data);
+        return;
     }
 
     waiting = 1;

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 847d5e2d675cf3c73b03b0ef4c20487af8252641..6a589807e6ba618678347da0a94ba2cb0974c186 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -67,11 +67,20 @@ const char *
 Auth::Ntlm::UserRequest::credentialsStr()
 {
     static char buf[MAX_AUTHTOKEN_LEN];
+    int printResult;
     if (user()->credentials() == Auth::Pending) {
-        snprintf(buf, sizeof(buf), "YR %s\n", client_blob);
+        printResult = snprintf(buf, sizeof(buf), "YR %s\n", client_blob);
     } else {
-        snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
+        printResult = snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
     }
+
+    // truncation is OK because we are used only for logging
+    if (printResult < 0) {
+        debugs(29, 2, "Can not build ntlm authentication credentials.");
+        buf[0] = '\0';
+    } else if (printResult >= (int)sizeof(buf))
+        debugs(29, 2, "Ntlm authentication credentials truncated.");
+
     return buf;
 }
 
@@ -121,19 +130,29 @@ Auth::Ntlm::UserRequest::startHelperLookup(HttpRequest *req, AccessLogEntry::Poi
     debugs(29, 8, HERE << "credentials state is '" << user()->credentials() << "'");
 
     const char *keyExtras = helperRequestKeyExtras(request, al);
+    int printResult = 0;
     if (user()->credentials() == Auth::Pending) {
         if (keyExtras)
-            snprintf(buf, sizeof(buf), "YR %s %s\n", client_blob, keyExtras);
+            printResult = snprintf(buf, sizeof(buf), "YR %s %s\n", client_blob, keyExtras);
         else
-            snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
+            printResult = snprintf(buf, sizeof(buf), "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
     } else {
         if (keyExtras)
-            snprintf(buf, sizeof(buf), "KK %s %s\n", client_blob, keyExtras);
+            printResult = snprintf(buf, sizeof(buf), "KK %s %s\n", client_blob, keyExtras);
         else
-            snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
+            printResult = snprintf(buf, sizeof(buf), "KK %s\n", client_blob);
     }
     waiting = 1;
 
+    if (printResult < 0 || printResult >= (int)sizeof(buf)) {
+        if (printResult < 0)
+            debugs(29, DBG_CRITICAL, "ERROR: Can not build ntlm authentication helper request");
+        else
+            debugs(29, DBG_CRITICAL, "ERROR: Ntlm authentication helper request too big for the " << sizeof(buf) << "-byte buffer.");
+        handler(data);
+        return;
+    }
+
     safe_free(client_blob);
     helperStatefulSubmit(ntlmauthenticators, buf, Auth::Ntlm::UserRequest::HandleReply,
                          new Auth::StateData(this, handler, data), authserver);