Message ID | 1580488320-10224-1-git-send-email-selva.nair@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [Openvpn-devel,v2] Swap the order of checks for validating interactive service user | expand |
Built and tested, works as specified.
Acked-by: Lev Stipakov <lstipakov@gmail.com>
<div dir="ltr"><div dir="ltr">Built and tested, works as specified.<div><br></div><div>Acked-by: Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>></div></div></div>
I am sorry, I have to retract my ACK. When ValidateOptions is called first and config is non located in global directory (Program Files), service replies to gui via pipe with error message: 0x20000001 You have specified a config file location (client.xxx.ovpn relative to C:\Users\lev\OpenVPN\config\client.xxx) that requires admin approval. This error may be avoided by adding your account to the "OpenVPN Administrators" group (null) This message is incorrect, since account is in admin group, we just haven't checked that yet. When following check verifies that user indeed belongs to the admin group, service runs openvpn and connection got established. By some reasons, above error message is not read on the gui side. However, by adding small delay to service code or a breakpoint, message reaches gui, which displays an error. While under normal circumstances it works, I believe it just works by accident (error message is not read on gui side without delay on service side). We need to move sending error message out of ValidateOptions. ma 3. helmik. 2020 klo 10.23 Lev Stipakov (lstipakov@gmail.com) kirjoitti: > Built and tested, works as specified. > > Acked-by: Lev Stipakov <lstipakov@gmail.com> >
Hi, On Mon, Feb 3, 2020 at 3:49 AM Lev Stipakov <lstipakov@gmail.com> wrote: > > I am sorry, I have to retract my ACK. > > When ValidateOptions is called first and config is non located in global directory (Program Files), > service replies to gui via pipe with error message: > > 0x20000001 > You have specified a config file location (client.xxx.ovpn relative to C:\Users\lev\OpenVPN\config\client.xxx) that requires admin approval. This error may be avoided by adding your account to the "OpenVPN Administrators" group > (null) > Nothing is as simple as it looks, is it.. After the first round of stupidity, I had run-tested version 2, but still did not catch this. > This message is incorrect, since account is in admin group, we just haven't checked that yet. When following check verifies that user indeed belongs to the admin group, service runs openvpn and connection got established. By some reasons, above error message is not read on the gui side. However, by adding small delay to service code or a breakpoint, message reaches gui, which displays an error. > Its a timing bug in the GUI. The I/O completion routine for reading from the service is initialized from the thread that handles the connection. But this thread is created in a suspended state and resumed only after sending the start request to the service from the main thread. That could be the reason why this message is missed by the GUI. This needs to be fixed as it could potentially affect normal operation of the GUI. > While under normal circumstances it works, I elieve it just works by accident (error message is not read on gui side without delay on service side). We need to move sending error message out of ValidateOptions. This error message was originally added so that the GUI can notify the user with a helpful message. But the GUI now checks config location and user's group memberships and offers to spawn an elevated process to add the user to ovpn_admin_group before calling the service. But still useful to return this message in case some other client decides to use the service. I'll move it out of ValidateOptions and add code to return it only when appropriate. Selva
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 6e72a14..ff5b08b 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1580,9 +1580,15 @@ RunOpenvpn(LPVOID p) goto out; } - /* Check user is authorized or options are white-listed */ - if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group) - && !ValidateOptions(pipe, sud.directory, sud.options)) + /* + * Only authorized users are allowed to use any command line options or + * have the config file in locations other than the global config directory. + * + * Check options are white-listed and config is in the global directory + * OR user is authorized to run any config. + */ + if (!ValidateOptions(pipe, sud.directory, sud.options) + && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group)) { goto out; }