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 39701e5dd0c08db8e240c5e274fa26ac61396299..d2a412748d690b5996b5f496d07385150e63e99d 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 c95f8bc155548ac41538ae63d88809f9e0dc5752..5e134bd27d8bc07aeba4a6368b454761d2917bcf 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -69,11 +69,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;
 }
 
@@ -126,16 +135,26 @@ Auth::Negotiate::UserRequest::startHelperLookup(HttpRequest *, AccessLogEntry::P
     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 9dbba526196fbeb07af69854e4fd35460133086c..76b76f2f3440508253b715e6e2baf670b94347e5 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -68,11 +68,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;
 }
 
@@ -122,19 +131,29 @@ Auth::Ntlm::UserRequest::startHelperLookup(HttpRequest *, AccessLogEntry::Pointe
     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);