| Message ID | 20171203131651.622-1-a@unstable.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2] reload HTTP proxy credentials when moving to the next connection profile | expand |
Hi, Feature-ACK, but one implementation concern: On 03-12-17 14:16, Antonio Quartulli wrote: > The HTTP proxy credentials are stored in a static variable that is > possibly initialized before each connection attempt. > > However, the variable is never "released" therefore get_user_pass() > refuses to overwrite its content and leaves it as it is. > Consequently, if the user config contains multiple connection profiles > with different http-proxy, each having its own credentials, only the > first user/pass couple is loaded and the others are all ignored. > This leads to connection failures because the proper credentials are > not associated with the right proxy server. > > The root of the misbehaviour seems to be located in the fact that, > despite the argument force passed to get_user_pass_http() being true, > no action is taken to release the static object containing the > credentials. > > Fix the misbehaviour by releasing the http-proxy credential object > when the reload is "forced". > > Trac: #836 > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > > v2: > - rebased on current master > > src/openvpn/proxy.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c > index fdc73b4a..cfc1d11c 100644 > --- a/src/openvpn/proxy.c > +++ b/src/openvpn/proxy.c > @@ -255,7 +255,16 @@ username_password_as_base64(const struct http_proxy_info *p, > static void > get_user_pass_http(struct http_proxy_info *p, const bool force) > { > - if (!static_proxy_user_pass.defined || force) > + /* > + * in case of forced (re)load, make sure the static storage is set as > + * undefined, otherwise get_user_pass() won't try to load any credential > + */ > + if (force) > + { > + static_proxy_user_pass.defined = false; > + } I'm wondering, shouldn't we clear the username/password in the struct if we set defined=false as a hardening measure? And shouldn't we then just replace this if(force) block with a call to purge_user_pass(up, force)? Or should we be careful to not clear the other members of static_proxy_user_pass? -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 03/12/17 23:02, Steffan Karger wrote: > Hi, > > Feature-ACK, but one implementation concern: > > On 03-12-17 14:16, Antonio Quartulli wrote: >> The HTTP proxy credentials are stored in a static variable that is >> possibly initialized before each connection attempt. >> >> However, the variable is never "released" therefore get_user_pass() >> refuses to overwrite its content and leaves it as it is. >> Consequently, if the user config contains multiple connection profiles >> with different http-proxy, each having its own credentials, only the >> first user/pass couple is loaded and the others are all ignored. >> This leads to connection failures because the proper credentials are >> not associated with the right proxy server. >> >> The root of the misbehaviour seems to be located in the fact that, >> despite the argument force passed to get_user_pass_http() being true, >> no action is taken to release the static object containing the >> credentials. >> >> Fix the misbehaviour by releasing the http-proxy credential object >> when the reload is "forced". >> >> Trac: #836 >> Signed-off-by: Antonio Quartulli <a@unstable.cc> >> --- >> >> v2: >> - rebased on current master >> >> src/openvpn/proxy.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c >> index fdc73b4a..cfc1d11c 100644 >> --- a/src/openvpn/proxy.c >> +++ b/src/openvpn/proxy.c >> @@ -255,7 +255,16 @@ username_password_as_base64(const struct http_proxy_info *p, >> static void >> get_user_pass_http(struct http_proxy_info *p, const bool force) >> { >> - if (!static_proxy_user_pass.defined || force) >> + /* >> + * in case of forced (re)load, make sure the static storage is set as >> + * undefined, otherwise get_user_pass() won't try to load any credential >> + */ >> + if (force) >> + { >> + static_proxy_user_pass.defined = false; >> + } > > I'm wondering, shouldn't we clear the username/password in the struct if > we set defined=false as a hardening measure? And shouldn't we then just > replace this if(force) block with a call to purge_user_pass(up, force)? > Or should we be careful to not clear the other members of > static_proxy_user_pass? There should be no problem in clearing up the struct, because the idea is that it has to be fully reinitialized with the "next proxy" information (so nothing to save). Actually we could just re-use clear_user_pass_http() that is there for this purpose. I'll send v3 with this change. Thanks a lot for your review! Cheers, > > -Steffan > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel >
diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c index fdc73b4a..cfc1d11c 100644 --- a/src/openvpn/proxy.c +++ b/src/openvpn/proxy.c @@ -255,7 +255,16 @@ username_password_as_base64(const struct http_proxy_info *p, static void get_user_pass_http(struct http_proxy_info *p, const bool force) { - if (!static_proxy_user_pass.defined || force) + /* + * in case of forced (re)load, make sure the static storage is set as + * undefined, otherwise get_user_pass() won't try to load any credential + */ + if (force) + { + static_proxy_user_pass.defined = false; + } + + if (!static_proxy_user_pass.defined) { unsigned int flags = GET_USER_PASS_MANAGEMENT; if (p->queried_creds)
The HTTP proxy credentials are stored in a static variable that is possibly initialized before each connection attempt. However, the variable is never "released" therefore get_user_pass() refuses to overwrite its content and leaves it as it is. Consequently, if the user config contains multiple connection profiles with different http-proxy, each having its own credentials, only the first user/pass couple is loaded and the others are all ignored. This leads to connection failures because the proper credentials are not associated with the right proxy server. The root of the misbehaviour seems to be located in the fact that, despite the argument force passed to get_user_pass_http() being true, no action is taken to release the static object containing the credentials. Fix the misbehaviour by releasing the http-proxy credential object when the reload is "forced". Trac: #836 Signed-off-by: Antonio Quartulli <a@unstable.cc> --- v2: - rebased on current master src/openvpn/proxy.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)