From: Amos Jeffries Date: Tue, 15 Feb 2011 12:55:35 +0000 (+1300) Subject: Bug 3105: malformed Proxy-Authorization leaks memory X-Git-Tag: take03^2~13 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=431aae42efb204149e3ba82e8b314e1756292b01;p=thirdparty%2Fsquid.git Bug 3105: malformed Proxy-Authorization leaks memory This simplifies the header parser for basic auth. Working towards a more generalized model of AuthUser children. Removing two memory allocations, two leaks and several unnecessary functions. --- diff --git a/src/auth/basic/auth_basic.cc b/src/auth/basic/auth_basic.cc index 414518d59c..bd278ce861 100644 --- a/src/auth/basic/auth_basic.cc +++ b/src/auth/basic/auth_basic.cc @@ -160,7 +160,6 @@ AuthBasicConfig::done() BasicUser::~BasicUser() { safe_free(passwd); - safe_free(cleartext); } static void @@ -301,109 +300,46 @@ authBasicAuthUserFindUsername(const char *username) return NULL; } -void -BasicUser::deleteSelf() const -{ - delete this; -} - BasicUser::BasicUser(AuthConfig *aConfig) : AuthUser(aConfig), passwd(NULL), auth_queue(NULL), - cleartext(NULL), - currentRequest(NULL), - httpAuthHeader(NULL) + currentRequest(NULL) {} -bool -BasicUser::decodeCleartext() +char * +AuthBasicConfig::decodeCleartext(const char *httpAuthHeader) { - char *sent_auth = NULL; - - /* username and password */ - sent_auth = xstrdup(httpAuthHeader); - - /* Trim trailing \n before decoding */ - strtok(sent_auth, "\n"); - - cleartext = uudecode(sent_auth); - - safe_free(sent_auth); + const char *proxy_auth = httpAuthHeader; - if (!cleartext) - return false; - - /* - * Don't allow NL or CR in the credentials. - * Oezguer Kesim - */ - debugs(29, 9, HERE << "'" << cleartext << "'"); - - if (strcspn(cleartext, "\r\n") != strlen(cleartext)) { - debugs(29, 1, HERE << "bad characters in authorization header '" << httpAuthHeader << "'"); - safe_free(cleartext); - return false; - } - return true; -} - -void -BasicUser::extractUsername() -{ - char * seperator = strchr(cleartext, ':'); - - if (seperator == NULL) { - username(cleartext); - } else { - /* terminate the username */ - *seperator = '\0'; - - username(cleartext); - - /* replace the colon so we can find the password */ - *seperator = ':'; - } - - if (!static_cast(config)->casesensitive) - Tolower((char *)username()); -} - -void -BasicUser::extractPassword() -{ - passwd = strchr(cleartext, ':'); + /* trim BASIC from string */ + while (xisgraph(*proxy_auth)) + proxy_auth++; - if (passwd == NULL) { - debugs(29, 4, HERE << "no password in proxy authorization header '" << httpAuthHeader << "'"); - passwd = NULL; - currentRequest->setDenyMessage("no password was present in the HTTP [proxy-]authorization header. This is most likely a browser bug"); - } else { - ++passwd; - if (*passwd == '\0') { - debugs(29, 4, HERE << "Disallowing empty password,user is '" << username() << "'"); - passwd = NULL; - currentRequest->setDenyMessage("Request denied because you provided an empty password. Users MUST have a password."); - } else { - passwd = xstrndup(passwd, USER_IDENT_SZ); - } - } -} + /* Trim leading whitespace before decoding */ + while (xisspace(*proxy_auth)) + proxy_auth++; -void -BasicUser::decode(char const *proxy_auth, AuthUserRequest::Pointer auth_user_request) -{ - currentRequest = auth_user_request; - httpAuthHeader = proxy_auth; - if (decodeCleartext()) { - extractUsername(); - extractPassword(); + /* Trim trailing \n before decoding */ + // XXX: really? is the \n actually still there? does the header parse not drop it? + char *eek = xstrdup(proxy_auth); + strtok(eek, "\n"); + char *cleartext = uudecode(eek); + safe_free(eek); + + if (cleartext) { + /* + * Don't allow NL or CR in the credentials. + * Oezguer Kesim + */ + debugs(29, 9, HERE << "'" << cleartext << "'"); + + if (strcspn(cleartext, "\r\n") != strlen(cleartext)) { + debugs(29, DBG_IMPORTANT, "WARNING: Bad characters in authorization header '" << httpAuthHeader << "'"); + safe_free(cleartext); + } } - currentRequest = NULL; // AYJ: why ?? we have only just filled it with data! - // so that we dont have circular UserRequest->User->UseRequest loops persisting outside the auth decode sequence???? - - // okay we dont need the original buffer string any more. - httpAuthHeader = NULL; + return cleartext; } bool @@ -416,26 +352,6 @@ BasicUser::valid() const return true; } -/** - * Generate a duplicate of the bad credentials before clearing the working copy. - */ -void -BasicUser::makeLoggingInstance(AuthUserRequest::Pointer auth_user_request) -{ - if (username()) { - /* log the username */ - debugs(29, 9, HERE << "Creating new user for logging '" << username() << "'"); - /* new scheme data */ - AuthUser::Pointer basic_auth = dynamic_cast(new BasicUser(config)); - auth_user_request->user(basic_auth); - /* save the credentials */ - basic_auth->username(username()); - username(NULL); - /* set the auth_user type */ - basic_auth->auth_type = AUTH_BROKEN; - } -} - void BasicUser::updateCached(BasicUser *from) { @@ -469,43 +385,70 @@ AuthBasicConfig::decode(char const *proxy_auth) { AuthUserRequest::Pointer auth_user_request = dynamic_cast(new AuthBasicUserRequest); /* decode the username */ - /* trim BASIC from string */ - while (xisgraph(*proxy_auth)) - proxy_auth++; + // retrieve the cleartext (in a dynamically allocated char*) + char *cleartext = decodeCleartext(proxy_auth); - /* decoder copy. maybe temporary. maybe added to hash if none already existing. */ - BasicUser *local_basic = new BasicUser(this); + // empty header? no auth details produced... + if (!cleartext) + return auth_user_request; - /* Trim leading whitespace before decoding */ - while (xisspace(*proxy_auth)) - proxy_auth++; + AuthUser::Pointer lb; + /* permitted because local_basic is purely local function scope. */ + BasicUser *local_basic = NULL; + + char *seperator = strchr(cleartext, ':'); + + lb = local_basic = new BasicUser(this); + if (seperator == NULL) { + local_basic->username(cleartext); + } else { + /* terminate the username */ + *seperator = '\0'; + local_basic->username(cleartext); + local_basic->passwd = xstrdup(seperator+1); + } + + if (!casesensitive) + Tolower((char *)local_basic->username()); + + if (local_basic->passwd == NULL) { + debugs(29, 4, HERE << "no password in proxy authorization header '" << proxy_auth << "'"); + auth_user_request->setDenyMessage("no password was present in the HTTP [proxy-]authorization header. This is most likely a browser bug"); + } else { + if (local_basic->passwd[0] == '\0') { + debugs(29, 4, HERE << "Disallowing empty password. User is '" << local_basic->username() << "'"); + safe_free(local_basic->passwd); + auth_user_request->setDenyMessage("Request denied because you provided an empty password. Users MUST have a password."); + } + } - local_basic->decode(proxy_auth, auth_user_request); + xfree(cleartext); if (!local_basic->valid()) { - local_basic->makeLoggingInstance(auth_user_request); + lb->auth_type = AUTH_BROKEN; + auth_user_request->user(lb); return auth_user_request; } /* now lookup and see if we have a matching auth_user structure in memory. */ AuthUser::Pointer auth_user; - if ((auth_user = authBasicAuthUserFindUsername(local_basic->username())) == NULL) { + if ((auth_user = authBasicAuthUserFindUsername(lb->username())) == NULL) { /* the user doesn't exist in the username cache yet */ /* save the credentials */ - debugs(29, 9, HERE << "Creating new user '" << local_basic->username() << "'"); + debugs(29, 9, HERE << "Creating new user '" << lb->username() << "'"); /* set the auth_user type */ - local_basic->auth_type = AUTH_BASIC; + lb->auth_type = AUTH_BASIC; /* current time for timeouts */ - local_basic->expiretime = current_time.tv_sec; + lb->expiretime = current_time.tv_sec; /* this basic_user struct is the 'lucky one' to get added to the username cache */ /* the requests after this link to the basic_user */ /* store user in hash */ - local_basic->addToNameCache(); + lb->addToNameCache(); - auth_user = dynamic_cast(local_basic); + auth_user = lb; assert(auth_user != NULL); } else { /* replace the current cached password with the new one */ diff --git a/src/auth/basic/auth_basic.h b/src/auth/basic/auth_basic.h index fef4b9ffa3..b97e62e2b8 100644 --- a/src/auth/basic/auth_basic.h +++ b/src/auth/basic/auth_basic.h @@ -31,17 +31,13 @@ class BasicUser : public AuthUser public: MEMPROXY_CLASS(BasicUser); - virtual void deleteSelf() const; BasicUser(AuthConfig *); ~BasicUser(); bool authenticated() const; void queueRequest(AuthUserRequest::Pointer auth_user_request, RH * handler, void *data); void submitRequest(AuthUserRequest::Pointer auth_user_request, RH * handler, void *data); - void decode(char const *credentials, AuthUserRequest::Pointer); - char *getCleartext() {return cleartext;} bool valid() const; - void makeLoggingInstance(AuthUserRequest::Pointer auth_user_request); /** Update the cached password for a username. */ void updateCached(BasicUser *from); @@ -52,12 +48,7 @@ public: BasicAuthQueueNode *auth_queue; private: - bool decodeCleartext(); - void extractUsername(); - void extractPassword(); - char *cleartext; AuthUserRequest::Pointer currentRequest; - char const *httpAuthHeader; }; MEMPROXY_CLASS_INLINE(BasicUser); @@ -79,12 +70,16 @@ public: virtual void fixHeader(AuthUserRequest::Pointer, HttpReply *, http_hdr_type, HttpRequest *); virtual void init(AuthConfig *); virtual void parse(AuthConfig *, int, char *); + void decode(char const *httpAuthHeader, AuthUserRequest::Pointer); virtual void registerWithCacheManager(void); virtual const char * type() const; char *basicAuthRealm; time_t credentialsTTL; int casesensitive; int utf8; + +private: + char * decodeCleartext(const char *httpAuthHeader); }; #endif /* __AUTH_BASIC_H__ */ diff --git a/src/auth/negotiate/auth_negotiate.cc b/src/auth/negotiate/auth_negotiate.cc index a2c954ec8a..bd2efe90e3 100644 --- a/src/auth/negotiate/auth_negotiate.cc +++ b/src/auth/negotiate/auth_negotiate.cc @@ -320,12 +320,6 @@ AuthNegotiateConfig::decode(char const *proxy_auth) return auth_user_request; } -void -NegotiateUser::deleteSelf() const -{ - delete this; -} - NegotiateUser::NegotiateUser(AuthConfig *aConfig) : AuthUser (aConfig) { proxy_auth_list.head = proxy_auth_list.tail = NULL; diff --git a/src/auth/negotiate/auth_negotiate.h b/src/auth/negotiate/auth_negotiate.h index f5d414b0f3..25c220f4be 100644 --- a/src/auth/negotiate/auth_negotiate.h +++ b/src/auth/negotiate/auth_negotiate.h @@ -29,7 +29,6 @@ public: MEMPROXY_CLASS(NegotiateUser); NegotiateUser(AuthConfig *); ~NegotiateUser(); - virtual void deleteSelf() const; virtual int32_t ttl() const; dlink_list proxy_auth_list; diff --git a/src/auth/ntlm/auth_ntlm.cc b/src/auth/ntlm/auth_ntlm.cc index b1d2328c6f..406045aab7 100644 --- a/src/auth/ntlm/auth_ntlm.cc +++ b/src/auth/ntlm/auth_ntlm.cc @@ -294,12 +294,6 @@ AuthNTLMConfig::decode(char const *proxy_auth) return auth_user_request; } -void -NTLMUser::deleteSelf() const -{ - delete this; -} - NTLMUser::NTLMUser (AuthConfig *aConfig) : AuthUser (aConfig) { proxy_auth_list.head = proxy_auth_list.tail = NULL; diff --git a/src/auth/ntlm/auth_ntlm.h b/src/auth/ntlm/auth_ntlm.h index 2a2514e5e1..ec137fb6da 100644 --- a/src/auth/ntlm/auth_ntlm.h +++ b/src/auth/ntlm/auth_ntlm.h @@ -21,7 +21,6 @@ public: NTLMUser(AuthConfig *); ~NTLMUser(); - virtual void deleteSelf() const; virtual int32_t ttl() const; dlink_list proxy_auth_list;