]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Bug 3105: malformed Proxy-Authorization leaks memory
authorAmos Jeffries <squid3@treenet.co.nz>
Tue, 15 Feb 2011 12:55:35 +0000 (01:55 +1300)
committerAmos Jeffries <squid3@treenet.co.nz>
Tue, 15 Feb 2011 12:55:35 +0000 (01:55 +1300)
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.

src/auth/basic/auth_basic.cc
src/auth/basic/auth_basic.h
src/auth/negotiate/auth_negotiate.cc
src/auth/negotiate/auth_negotiate.h
src/auth/ntlm/auth_ntlm.cc
src/auth/ntlm/auth_ntlm.h

index 414518d59c16afb87832b9e9caf729de08279c0b..bd278ce861ccf26e9eef9f38785a97b7e82b82d6 100644 (file)
@@ -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 <oec@codeblau.de>
-     */
-    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<AuthBasicConfig*>(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 <oec@codeblau.de>
+         */
+        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<AuthUser*>(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<AuthUserRequest*>(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<AuthUser*>(local_basic);
+        auth_user = lb;
         assert(auth_user != NULL);
     } else {
         /* replace the current cached password with the new one */
index fef4b9ffa31ce8ffd649dfb52795607dd60b7379..b97e62e2b8eaf53b491347eeeab344c0610f2d7e 100644 (file)
@@ -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__ */
index a2c954ec8a7a89ea5c7183afcc0ca4642fcb9dc5..bd2efe90e3efe3ac53c6107827097d042740253e 100644 (file)
@@ -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;
index f5d414b0f31b5ef177b9ab65eb42f6fca31c71a0..25c220f4be266ec0ae755a520a67aa8a1f4a233e 100644 (file)
@@ -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;
index b1d2328c6f8392ba1abcb64b6b6c936e80c6c121..406045aab73eded7264b353577b9d4506522bb98 100644 (file)
@@ -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;
index 2a2514e5e1ddb00ea957c946e7c276c82c76f700..ec137fb6da73225ee9be92327b15a27d2614b08f 100644 (file)
@@ -21,7 +21,6 @@ public:
     NTLMUser(AuthConfig *);
     ~NTLMUser();
 
-    virtual void deleteSelf() const;
     virtual int32_t ttl() const;
 
     dlink_list proxy_auth_list;