Remove global allocation of HTTP proxy user/password

Openvpn-devel In review David Sommerseth 3 participants 4 messages Sep 22, 2016 – Sep 23, 2016

Share & tools

David Sommerseth

author

2016-09-22 19:40 UTC #0 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

This patch tries to remove allocating memory which will not being used unless a specific feature is enabled. This is merely to get a patch out to be tested and to see if this is worth the efforts. The code does get extended, but for embedded devices, the benefit might just be big enough to get a reasonable profit.

This is related to a patch where it was considered to increase USER_PASS_LEN from 128 bytes to 4096 by default, but it was discovered that there are several places where the code uses static allocation which then would be wasting memory.

If people could test this patch for HTTP and NTLM proxies, both with and without user/password authentication, I would appreciate that a lot!

If we consider this work to be worth it, similar patches for the management interface and the more generic username/password authentication might come too.

David Sommerseth (1):
Remove static global allocation of HTTP proxy user/passwords

 src/openvpn/ntlm.c  | 16 ++++++++--------
 src/openvpn/proxy.c | 41 +++++++++++++++++++++++++++++------------
 src/openvpn/proxy.h |  2 +-
 3 files changed, 38 insertions(+), 21 deletions(-)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX5DN2AAoJEIbPlEyWcf3ygbUQAIRZ0z5trB+/s39oGXgQzI6V yAPe/Q3vJIUG9KRc3bpmqMPBNhS1ieCYpdpvhVdpgWpKnJbWvfFdnI2yQj4Uth0S G+j7oFxI+c+4LT4o2cyZ1ib7LA204ewkR/etbZvlMJcdq6SW34R3BU4+KHhRGvAp XtVAEzhjGdV9JofeJ880BFKL2J+wrx8wdrX26ysBYmdc4mmlnTpyF4owexh13AyO ptp0MUa4dmTCGyItkKY52PxtfOmj4scnqxwWN/CKwAf8fVo7nUtdQfPAVSUvb7ty LDbh2uSg1F9CVsalYvGV8dSEx3Rz3sfyciNjpPup/Hp7QtHqyjgIds1F/rLwDbvK JDCepPgCW1Y0A+6r8JL1ocB0uXh4akSR7rPeX1iDjC6+l4lZgpozqoLvNmPpDnkh 4UbtnMGUzeaynR47X7FA/TgqdWmoseMkuFindeyW7vHoIdn4Q2a4tQtWwfSW2o8X 2aO/za3hPq1TDa0fOZICWQYJ4ouwXoj1PmU5AcAF0A74v9L8CXPlaI2IZsxajrO/ iescNu/6gvE4VWdLEFBhJWI464icnk8CenEdj08r2UPrMCwdJF/ZiqJvlesU9hFR m59WXXvjGMerBhPaKdiVxqHDoMbzFOGkd3Nedx5k3N8vkxORxf3cz/xLCytZet73 3wdLUKQsTbfb6Cp5Xv/z
=JKpg
-----END PGP SIGNATURE-----

David Sommerseth

author

2016-09-22 19:40 UTC #1 lore raw

PATCH 1/1 Remove static global allocation of HTTP proxy user/passwords

This avoids allocating static memory which is not used unless the a HTTP proxy with authentication is configured.

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/ntlm.c  | 16 ++++++++--------
 src/openvpn/proxy.c | 41 +++++++++++++++++++++++++++++------------
 src/openvpn/proxy.h |  2 +-
 3 files changed, 38 insertions(+), 21 deletions(-)

src/openvpn/ntlm.c +8 -8

index 3390bdd..ee8be6e 100644
@@ -192,7 +192,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
 	 *
 	 */
 	
-  char pwbuf[sizeof (p->up.password) * 2]; /* for unicode password */
+  char pwbuf[USER_PASS_LEN * 2]; /* for unicode password */
   char buf2[128]; /* decoded reply from proxy */
   unsigned char phase3[464];

@@ -218,27 +218,27 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
 
   CLEAR (buf2);
 
-  ASSERT (strlen (p->up.username) > 0);
-  ASSERT (strlen (p->up.password) > 0);
+  ASSERT (strlen (p->up->username) > 0);
+  ASSERT (strlen (p->up->password) > 0);
 	
 	/* username parsing */
-	separator = strchr(p->up.username, '\\');
+	separator = strchr(p->up->username, '\\');
 	if (separator == NULL) {
-		strncpy(username, p->up.username, sizeof(username)-1);
+		strncpy(username, p->up->username, sizeof(username)-1);
 		username[sizeof(username)-1]=0;
 		domain[0]=0;
 	} else {
 		strncpy(username, separator+1, sizeof(username)-1);
 		username[sizeof(username)-1]=0;
-		len = separator - p->up.username;
+		len = separator - p->up->username;
 		if (len > sizeof(domain) - 1) len = sizeof(domain) - 1;
-		strncpy(domain, p->up.username,  len);
+		strncpy(domain, p->up->username,  len);
 		domain[len]=0;
 	}
 
 
   /* fill 1st 16 bytes with md4 hash, disregard terminating null */
-  gen_md4_hash (pwbuf, unicodize (pwbuf, p->up.password) - 2, md4_hash);
+  gen_md4_hash (pwbuf, unicodize (pwbuf, p->up->password) - 2, md4_hash);
 
   /* pad to 21 bytes */
   memset(md4_hash + MD4_DIGEST_LENGTH, 0, 5);

src/openvpn/proxy.c +29 ~~-12~~

85 lines not on this page · raw message

src/openvpn/proxy.h +1 -1

index 7d2581c..600e2fe 100644
@@ -69,7 +69,7 @@ struct http_proxy_info {
   bool defined;
   int auth_method;
   struct http_proxy_options options;
-  struct user_pass up;
+  struct user_pass *up;
   char *proxy_authenticate;
   bool queried_creds;
 };

Signature · 1 line

1.8.3.1

Selva Nair

2016-09-22 20:51 UTC #2 lore raw

Re: [Openvpn-devel] [PATCH] Remove static global allocation of HTTP proxy user/passwords

Hi,

Quoted 2 lines · David Sommerseth

This avoids allocating static memory which is not used unless the a HTTP proxy with authentication is configured.

.....

The only place the original code referred to the global static_proxy_user_pass appears to be in this function and from its usage it appears redundant.

static void

Quoted 18 lines

get_user_pass_http (struct http_proxy_info *p, const bool force) {

src/openvpn/proxy.c +11 -1

-  if (!static_proxy_user_pass.defined || force)
+  if (!proxy_user_pass_cache || !proxy_user_pass_cache->defined|| force)
     {
       unsigned int flags = GET_USER_PASS_MANAGEMENT;
+
+      if (!proxy_user_pass_cache)
+        {
+          ALLOC_OBJ_CLEAR (proxy_user_pass_cache, struct user_pass);
+        }
+      else
+        {
+          CLEAR (*proxy_user_pass_cache);
+        }
+
       if (p->queried_creds)

src/openvpn/proxy.c static const char * · line 251

Quoted 10 lines

src/openvpn/proxy.c +1 -2

        flags |= GET_USER_PASS_PREVIOUS_CREDS_FAILED;
       if (p->options.inline_creds)
        flags |= GET_USER_PASS_INLINE_CREDS;
-      get_user_pass (&static_proxy_user_pass,
+      get_user_pass (proxy_user_pass_cache,
                     p->options.auth_file,
                     UP_TYPE_PROXY,
                     flags);
       p->queried_creds = true;
-      p->up = static_proxy_user_pass;

A copy of the struct is made only when a new password is obtained using get_user_pass. Then, what was the purpose of the static? Or was it a bug that the copy of the cached password was not made every time this function was called? If not, we can just delete the static and use p->up directly.

Quoted 3 lines

+ p->up = proxy_user_pass_cache;
}

Selva

David Sommerseth

2016-09-23 08:42 UTC #3 lore raw

Re: [Openvpn-devel] [PATCH] Remove static global allocation of HTTP proxy user/passwords

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 30 lines · Selva Nair

Hi,
On Thu, Sep 22, 2016 at 3:40 PM, David Sommerseth <[email protected]... <mailto:[email protected]...>> wrote:
This avoids allocating static memory which is not used unless the a HTTP proxy with authentication is configured.
.....
The only place the original code referred to the global static_proxy_user_pass appears to be in this function and from its usage it appears redundant.
static void get_user_pass_http (struct http_proxy_info *p, const bool force) { - if (!static_proxy_user_pass.defined || force) + if (!proxy_user_pass_cache || !proxy_user_pass_cache->defined||
force) { unsigned int flags = GET_USER_PASS_MANAGEMENT; + + if
(!proxy_user_pass_cache) + { + ALLOC_OBJ_CLEAR
(proxy_user_pass_cache, struct user_pass); + } + else +
{ + CLEAR (*proxy_user_pass_cache); + } + if
(p->queried_creds) flags |= GET_USER_PASS_PREVIOUS_CREDS_FAILED; if (p->options.inline_creds) flags |= GET_USER_PASS_INLINE_CREDS; -
get_user_pass (&static_proxy_user_pass, + get_user_pass
(proxy_user_pass_cache, p->options.auth_file, UP_TYPE_PROXY,
flags); p->queried_creds = true; - p->up =
static_proxy_user_pass;

Quoted 7 lines

A copy of the struct is made only when a new password is obtained using get_user_pass. Then, what was the purpose of the static? Or was it a bug that the copy of the cached password was not made every time this function was called? If not, we can just delete the static and use p->up directly.
} + p->up = proxy_user_pass_cache; }

I completely agree! I did consider this too, but figured I needed to understand better what happens if a client uses connection blocks and if the caching of username/password goes across these connection blocks.

If you have --http-proxy configured which requires authentication and
two connection blocks. By removing the caching, I worry that OpenVPN
would ask for a new proxy username/password each time it tries a different connection block.

I haven't had enough time to dig into this scenario, but if that is not the issue then I don't see any point in keeping this cache. But it is hard to get a complete overview where each of these configuration structs are unique and singular and where they are re-used and all points at the same "object".

But I'm happy to see more spotting the same issue. And I'm always thankful for your thorough reviews, Selva!

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX5OrxAAoJEIbPlEyWcf3yULsP/RuL/+qm3svPCyatylCIupC5 cAXb480V+gyw+acE0IJMYGwQ7tQ7mjNKVDhV+BP5nSROX+Kgc5VPy/sHULkuNDLM NKw4pa0X53PGPIiGSGMryphheFb22hiBVi0RPzGRC453IRSQSasKXuRMur+3Jlkf J9T6i/WlrOD2c1VROGPY7PXjHXZct7+P42rERxWaJTTQ6KA+yS6Nzx+EZw/v9EO4 dnZhORPkq6IBu+cZ7JshhA6S2wY0CUDVZ0wSIFG2KcrJi/JjP5GkeTS+74RVbL3V +OPcGBdlHC08wDCQpRdKsAAv7M/GmdR2sQJKoPlhT7JTkLr87LC1vV+e5tx3FonN n45jB1bVV0VZ0Bo+lVG+4j3eyNaN/hhdMLHsQDdxQ9n1v7ajGFYf29K+nxsymb4V GFMw45LHitHkCQfmptOjRMkmoRQE2tOjniiUyLWj1a9uEJwesPfHBpa1qYP9LprQ rqUbscDiH5VPATUzAlv7O1UMo7mJVQdkYd9y9X9ayc+8uY5uARqf/sPoga8QGRcD 0v7/CvrXCKhVDYpIV2UWgstJ2aw2Rmv3voKC8rNlchIK5ivHly7PO0Hh4KjW2qXO ZnW1kvYhThw0nz/W63v6QHZa/JY+p4BiybTS3mANAwvy4uzYlZugG7oq/DpFj+5/ eYH5gDWPMtJxACZzlfM5
=Ndg9
-----END PGP SIGNATURE-----