------------------------------------------------------------ revno: 13802 revision-id: squid3@treenet.co.nz-20150420024643-2dhcw6n89igt0g1w parent: squid3@treenet.co.nz-20150420024144-srf1a3635zushydu author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Sun 2015-04-19 19:46:43 -0700 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20150420024643-2dhcw6n89igt0g1w # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 4cbb3292ccdd2943e9e4a3d3743ec2bc29c6a785 # timestamp: 2015-04-20 02:47:44 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20150420024144-\ # srf1a3635zushydu # # Begin patch === modified file 'src/auth/UserRequest.h' --- src/auth/UserRequest.h 2015-01-13 09:13:49 +0000 +++ src/auth/UserRequest.h 2015-04-20 02:46:43 +0000 @@ -27,8 +27,8 @@ /** * 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 === modified file 'src/auth/negotiate/UserRequest.cc' --- src/auth/negotiate/UserRequest.cc 2015-02-01 09:14:12 +0000 +++ src/auth/negotiate/UserRequest.cc 2015-04-20 02:46:43 +0000 @@ -68,11 +68,20 @@ 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 @@ 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); - else - snprintf(buf, sizeof(buf), "KK %s\n", client_blob); + printResult = snprintf(buf, sizeof(buf), "KK %s %s\n", client_blob, keyExtras); + else + 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; === modified file 'src/auth/ntlm/UserRequest.cc' --- src/auth/ntlm/UserRequest.cc 2015-01-19 11:50:22 +0000 +++ src/auth/ntlm/UserRequest.cc 2015-04-20 02:46:43 +0000 @@ -67,11 +67,20 @@ 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 @@ 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);