From: Christos Tsantilas Date: Thu, 16 Apr 2015 13:18:44 +0000 (+0300) Subject: Negotiate Kerberos authentication request size exceeds output buffer size. X-Git-Tag: merge-candidate-3-v1~173 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d0873e0c66fc9d874e8247d699348654cb069723;p=thirdparty%2Fsquid.git 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 39701e5dd0..d2a412748d 100644 --- 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 c95f8bc155..5e134bd27d 100644 --- 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 9dbba52619..76b76f2f34 100644 --- 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);