| Message ID | 20220313193154.9350-3-openvpn@sf.lists.topphemmelig.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Disable multiple deferred authentication | expand |
> David Sommerseth <openvpn@sf.lists.topphemmelig.net> hat am 13.03.2022 20:31 geschrieben: > diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c > index e3a89293..8236e29e 100644 > --- a/src/openvpn/plugin.c > +++ b/src/openvpn/plugin.c > @@ -802,7 +802,7 @@ plugin_call_ssl(const struct plugin_list *pl, > const char **envp; > const int n = plugin_n(pl); > bool error = false; > - bool deferred = false; > + bool deferred_auth_done = false; > > setenv_del(es, "script_type"); > envp = make_env_array(es, false, &gc); > @@ -824,7 +824,34 @@ plugin_call_ssl(const struct plugin_list *pl, > break; > > case OPENVPN_PLUGIN_FUNC_DEFERRED: > - deferred = true; > + if ((type == OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY) > + && deferred_auth_done) > + { > + /* > + * Do not allow deferred auth if a deferred auth has > + * already been started. This should allow a single > + * deferred auth call to happen, with one or more > + * auth calls with an instant authentication result. > + * > + * The plug-in API is not designed for multiple > + * deferred authentications to happen, as the > + * auth_control_file file will be shared across all > + * the plug-ins. > + * > + * Since this is considered a critical configuration > + * error, we bail out and exit the OpenVPN process. > + */ > + error = true; > + msg(M_FATAL, > + "Exiting due to multiple authentication plug-ins " > + "performing deferred authentication. Only one " Super nitpick, but there is a second space missing here to be consistent with the other full stop. > + "authentication plug-in doing deferred auth is " > + "allowed. Ignoring the result and stopping now, " > + "the current authentication result is not to be " > + "trusted."); > + break; > + } > + deferred_auth_done = true; > break; > > default: Regards, -- Frank Lichtenheld
Hi, On 13/03/2022 20:31, David Sommerseth wrote: > From: David Sommerseth <davids@openvpn.net> > > The plug-in API in OpenVPN 2.x is not designed for running multiple > deferred authentication processes in parallel. The authentication > results of such configurations are not to be trusted. For now we bail > out when this discovered with an error in the log. > > CVE: 2022-0547 > Signed-off-by: David Sommerseth <davids@openvpn.net> Same as the patch for master. Acked-by: Antonio Quartulli <a@unstable.cc> > + error = true; > + msg(M_FATAL, > + "Exiting due to multiple authentication plug-ins " > + "performing deferred authentication. Only one " > + "authentication plug-in doing deferred auth is " > + "allowed. Ignoring the result and stopping now, " > + "the current authentication result is not to be " > + "trusted."); Frank reported that we should use double space after the full-stop. Honestly I'd prefer just single-space everywhere as it is more "traditional". This said, Gert can make the final decision and modify the patch on the fly. Cheers,
This is the same patch that Antonio tested when it was still only
discussed on the security@ list (except for the "two spaces" comment
fix). Also, fixed a "do" -> "does" in plugin-options.rst.
As discussed before, I still think that "aborting the server process"
is excessive and we should just return OPENVPN_PLUGIN_FUNC_ERROR in
this case (with a proper log message), but we'll have to agree to
disagree on this one. I have a patch to bring proper functionality
instead... :-)
I have not tested this particular scenario (Antonio has), just made
sure nothing else got broken.
I have not updated Changes.rst in the context of this patch - will
do that when preparing 2.5.6 release.
Your patch has been applied to the master and release/2.5 branch.
commit 282ddbac54f8d4923844f69983b38dd2b813a00a (master)
commit af3e382649d96ae77cc5e42be8270f355e5cfec5 (release/2.5)
Author: David Sommerseth
Date: Sun Mar 13 20:31:53 2022 +0100
plug-ins: Disallow multiple deferred authentication plug-ins
Signed-off-by: David Sommerseth <davids@openvpn.net>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20220313193154.9350-3-openvpn@sf.lists.topphemmelig.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23931.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
> Antonio Quartulli <a@unstable.cc> hat am 15.03.2022 16:11 geschrieben: > Frank reported that we should use double space after the full-stop. > Honestly I'd prefer just single-space everywhere as it is more > "traditional". Yeah, I also prefer one space but I actually grepped the code and while it is somewhat inconsistent, double-space seems to be winning decisively right now. And we definitely should not use both in the same message ;) Regards, -- Frank Lichtenheld
diff --git a/doc/man-sections/plugin-options.rst b/doc/man-sections/plugin-options.rst index 51c574fe..6cbbc2f3 100644 --- a/doc/man-sections/plugin-options.rst +++ b/doc/man-sections/plugin-options.rst @@ -55,3 +55,12 @@ plug-ins must be prebuilt and adhere to the OpenVPN Plug-In API. (such as tls-verify, auth-user-pass-verify, or client-connect), then every module and script must return success (:code:`0`) in order for the connection to be authenticated. + + **WARNING**: + Plug-ins may do deferred execution, meaning the plug-in will + return the control back to the main OpenVPN process and provide + the plug-in result later on via a different thread or process. + OpenVPN does **NOT** support multiple authentication plug-ins + **where more than one of them** do deferred authentication. + If this behaviour is detected, OpenVPN will shut down upon first + authentication. diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c index e3a89293..8236e29e 100644 --- a/src/openvpn/plugin.c +++ b/src/openvpn/plugin.c @@ -802,7 +802,7 @@ plugin_call_ssl(const struct plugin_list *pl, const char **envp; const int n = plugin_n(pl); bool error = false; - bool deferred = false; + bool deferred_auth_done = false; setenv_del(es, "script_type"); envp = make_env_array(es, false, &gc); @@ -824,7 +824,34 @@ plugin_call_ssl(const struct plugin_list *pl, break; case OPENVPN_PLUGIN_FUNC_DEFERRED: - deferred = true; + if ((type == OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY) + && deferred_auth_done) + { + /* + * Do not allow deferred auth if a deferred auth has + * already been started. This should allow a single + * deferred auth call to happen, with one or more + * auth calls with an instant authentication result. + * + * The plug-in API is not designed for multiple + * deferred authentications to happen, as the + * auth_control_file file will be shared across all + * the plug-ins. + * + * Since this is considered a critical configuration + * error, we bail out and exit the OpenVPN process. + */ + error = true; + msg(M_FATAL, + "Exiting due to multiple authentication plug-ins " + "performing deferred authentication. Only one " + "authentication plug-in doing deferred auth is " + "allowed. Ignoring the result and stopping now, " + "the current authentication result is not to be " + "trusted."); + break; + } + deferred_auth_done = true; break; default: @@ -844,7 +871,7 @@ plugin_call_ssl(const struct plugin_list *pl, { return OPENVPN_PLUGIN_FUNC_ERROR; } - else if (deferred) + else if (deferred_auth_done) { return OPENVPN_PLUGIN_FUNC_DEFERRED; }