[Openvpn-devel,2/4] Extend FindSystemInfo custom action to detect OpenVPNService state

Message ID 20181219202611.2144-2-simon@rozman.si
State Accepted
Headers show
Series [Openvpn-devel,1/4] Make DriverCertification MSI property public | expand

Commit Message

Simon Rozman Dec. 19, 2018, 9:26 a.m. UTC
---
 src/openvpnmsica/openvpnmsica.c | 168 ++++++++++++++++++++++++++++----
 1 file changed, 151 insertions(+), 17 deletions(-)

Comments

Gert Doering Jan. 18, 2019, 8:44 a.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

This is code I can even understand (to some extent) ;-) - overall it looks
good, but a few style warts sneaked in...

+finish_QueryServiceStatusEx:;
+        
+    // Service is not started. Is it set to auto-start?
+    // MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB.
+    // This is small enough to fit on stack.

.. there shouldn't be a ";" after a label, and no C++ comments... but
I assume the uncrustify patch coming next will fix that...

Something else might not be obvious to the reader:

+    if (pQsc->dwStartType <= SERVICE_AUTO_START)
+    {

so what does "lesser than AUTO_START" mean?  manual start?  no start at
all?  Comparing for "lesser or equal" with something enum-like might
warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
or so...  (yes, I can google this, but still).


Compile tested on Ubuntu 16.04 / mingw.


Your patch has been applied to the master branch.

commit 8148ee9d01de75d0302f224ef499eddfabc6fee2
Author: Simon Rozman
Date:   Wed Dec 19 21:26:09 2018 +0100

     Extend FindSystemInfo custom action to detect OpenVPNService state

     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20181219202611.2144-2-simon@rozman.si>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18039.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Simon Rozman Jan. 20, 2019, 2:19 a.m. UTC | #2
Hi,

> +finish_QueryServiceStatusEx:;
> +
> +    // Service is not started. Is it set to auto-start?
> +    // MSDN describes the maximum buffer size for QueryServiceConfig()
> to be 8kB.
> +    // This is small enough to fit on stack.
> 
> .. there shouldn't be a ";" after a label, and no C++ comments... but I
> assume the uncrustify patch coming next will fix that...

The sentence following the finish_QueryServiceStatusEx label is a local
variable declaration. While MSVC and MinGW are fine with or without
semicolon, the Visual Studio 2017 IDE's IntelliSense is complaining with
"E1072: a declaration cannot have a label".

Now, I could leave that semicolon in place to keep IntelliSense false
warning away, but looking at the code I asked myself a question: "If my code
is so convoluted it produces a false warning to a bot, how convoluted it
must be for a human to understand?" False warnings are usually sign of
overcomplex or atypical code design. I have refactored the code.

> 
> Something else might not be obvious to the reader:
> 
> +    if (pQsc->dwStartType <= SERVICE_AUTO_START)
> +    {
> 
> so what does "lesser than AUTO_START" mean?  manual start?  no start at
> all?  Comparing for "lesser or equal" with something enum-like might
> warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
> or so...  (yes, I can google this, but still).

This is a very generic test "Is driver or service set to start automatically
in any phase of the system launch?" on the Windows. I've added a comment
with a brief explanation.

A patch will follow.

Regards,
Simon
Gert Doering Jan. 20, 2019, 2:30 a.m. UTC | #3
Hi,

On Sun, Jan 20, 2019 at 01:19:50PM +0000, Simon Rozman wrote:
> Now, I could leave that semicolon in place to keep IntelliSense false
> warning away, but looking at the code I asked myself a question: "If my code
> is so convoluted it produces a false warning to a bot, how convoluted it
> must be for a human to understand?" False warnings are usually sign of
> overcomplex or atypical code design. I have refactored the code.

Thanks for the explanation, I wasn't aware that there is a special case
where a semicolon is required after a label.  Even if only MSVC :-)

But second half is even better ;-)

> > Something else might not be obvious to the reader:
> > 
> > +    if (pQsc->dwStartType <= SERVICE_AUTO_START)
> > +    {
> > 
> > so what does "lesser than AUTO_START" mean?  manual start?  no start at
> > all?  Comparing for "lesser or equal" with something enum-like might
> > warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
> > or so...  (yes, I can google this, but still).
> 
> This is a very generic test "Is driver or service set to start automatically
> in any phase of the system launch?" on the Windows. 

Ah.  So "anyone who has any clue about windows services would know and
understand" - good enough.  We need more clueful windows people to review
windows patches :-)

(Selva is hiding somewhere)

> I've added a comment with a brief explanation.
> 
> A patch will follow.

Thanks!

gert

Patch

diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c
index 721bd4f8..810c2858 100644
--- a/src/openvpnmsica/openvpnmsica.c
+++ b/src/openvpnmsica/openvpnmsica.c
@@ -44,6 +44,7 @@ 
 #include <tchar.h>
 
 #ifdef _MSC_VER
+#pragma comment(lib, "advapi32.lib")
 #pragma comment(lib, "iphlpapi.lib")
 #pragma comment(lib, "shell32.lib")
 #pragma comment(lib, "shlwapi.lib")
@@ -193,29 +194,34 @@  _openvpnmsica_debug_popup(_In_z_ LPCTSTR szFunctionName)
 #endif
 
 
-UINT __stdcall
-FindSystemInfo(_In_ MSIHANDLE hInstall)
+/**
+ * Detects Windows version and sets DRIVERCERTIFICATION property to "", "whql", or "attsgn"
+ * accordingly.
+ *
+ * @param hInstall      Handle to the installation provided to the DLL custom action
+ *
+ * @return ERROR_SUCCESS on success; An error code otherwise
+ *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
+ */
+static UINT
+openvpnmsica_set_driver_certification(_In_ MSIHANDLE hInstall)
 {
-#ifdef _MSC_VER
-#pragma comment(linker, DLLEXP_EXPORT)
-#endif
-
-    openvpnmsica_debug_popup(TEXT(__FUNCTION__));
-
     UINT uiResult;
-    BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
-
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
 
     /* Get Windows version. */
+#ifdef _MSC_VER
+#pragma warning(push)
+#pragma warning(disable: 4996) /* 'GetVersionExW': was declared deprecated. */
+#endif
     OSVERSIONINFOEX ver_info = { .dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX) };
     if (!GetVersionEx((LPOSVERSIONINFO)&ver_info)) {
         uiResult = GetLastError();
         msg(M_NONFATAL | M_ERRNO, "%s: GetVersionEx() failed", __FUNCTION__);
-        goto cleanup_CoInitialize;
+        return uiResult;
     }
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
 
     /* The Windows version is usually spoofed, check using RtlGetVersion(). */
     TCHAR szDllPath[0x1000];
@@ -294,17 +300,145 @@  FindSystemInfo(_In_ MSIHANDLE hInstall)
     {
         SetLastError(uiResult); /* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */
         msg(M_NONFATAL | M_ERRNO, "%s: MsiSetProperty(\"DRIVERCERTIFICATION\") failed", __FUNCTION__);
-        goto cleanup_CoInitialize;
+        return uiResult;
+    }
+
+    return ERROR_SUCCESS;
+}
+
+
+/**
+ * Detects if the OpenVPNService service is in use (running or paused) and sets
+ * OPENVPNSERVICE to the service process PID, or its path if it is set to
+ * auto-start, but not running.
+ *
+ * @param hInstall      Handle to the installation provided to the DLL custom action
+ *
+ * @return ERROR_SUCCESS on success; An error code otherwise
+ *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
+ */
+static UINT
+openvpnmsica_set_openvpnserv_state(_In_ MSIHANDLE hInstall)
+{
+    UINT uiResult;
+
+    /* Get Service Control Manager handle. */
+    SC_HANDLE hSCManager = OpenSCManager(NULL, SERVICES_ACTIVE_DATABASE, SC_MANAGER_CONNECT);
+    if (hSCManager == NULL)
+    {
+        uiResult = GetLastError();
+        msg(M_NONFATAL | M_ERRNO, "%s: OpenSCManager() failed", __FUNCTION__);
+        return uiResult;
+    }
+
+    /* Get OpenVPNService service handle. */
+    SC_HANDLE hService = OpenService(hSCManager, TEXT("OpenVPNService"), SERVICE_QUERY_STATUS | SERVICE_QUERY_CONFIG);
+    if (hService == NULL)
+    {
+        uiResult = GetLastError();
+        if (uiResult == ERROR_SERVICE_DOES_NOT_EXIST) {
+            /* This is not actually an error. */
+            goto cleanup_OpenSCManager;
+        }
+        msg(M_NONFATAL | M_ERRNO, "%s: OpenService(\"OpenVPNService\") failed", __FUNCTION__);
+        goto cleanup_OpenSCManager;
+    }
+
+    /* Query service status. */
+    SERVICE_STATUS_PROCESS ssp;
+    DWORD dwBufSize;
+    if (!QueryServiceStatusEx(hService, SC_STATUS_PROCESS_INFO, (LPBYTE)&ssp, sizeof(ssp), &dwBufSize))
+    {
+        uiResult = GetLastError();
+        msg(M_NONFATAL | M_ERRNO, "%s: QueryServiceStatusEx(\"OpenVPNService\") failed", __FUNCTION__);
+        goto finish_QueryServiceStatusEx;
+    }
+
+    switch (ssp.dwCurrentState)
+    {
+    case SERVICE_START_PENDING:
+    case SERVICE_RUNNING:
+    case SERVICE_STOP_PENDING:
+    case SERVICE_PAUSE_PENDING:
+    case SERVICE_PAUSED:
+    case SERVICE_CONTINUE_PENDING:
+    {
+        /* Set OPENVPNSERVICE property to service PID. */
+        TCHAR szPID[10/*MAXDWORD in decimal*/ + 1/*terminator*/];
+        _stprintf_s(
+            szPID, _countof(szPID),
+            TEXT("%u"),
+            ssp.dwProcessId);
+
+        uiResult = MsiSetProperty(hInstall, TEXT("OPENVPNSERVICE"), szPID);
+        if (uiResult != ERROR_SUCCESS)
+        {
+            SetLastError(uiResult); /* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */
+            msg(M_NONFATAL | M_ERRNO, "%s: MsiSetProperty(\"OPENVPNSERVICE\") failed", __FUNCTION__);
+        }
+        goto cleanup_OpenService;
+    }
+    break;
+    }
+finish_QueryServiceStatusEx:;
+
+    // Service is not started. Is it set to auto-start?
+    // MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB.
+    // This is small enough to fit on stack.
+    BYTE _buffer_8k[8192];
+    LPQUERY_SERVICE_CONFIG pQsc = (LPQUERY_SERVICE_CONFIG)_buffer_8k;
+    dwBufSize = sizeof(_buffer_8k);
+    if (!QueryServiceConfig(hService, pQsc, dwBufSize, &dwBufSize))
+    {
+        uiResult = GetLastError();
+        msg(M_NONFATAL | M_ERRNO, "%s: QueryServiceStatusEx(\"QueryServiceConfig\") failed", __FUNCTION__);
+        goto cleanup_OpenService;
+    }
+
+    if (pQsc->dwStartType <= SERVICE_AUTO_START)
+    {
+        uiResult = MsiSetProperty(hInstall, TEXT("OPENVPNSERVICE"), pQsc->lpBinaryPathName);
+        if (uiResult != ERROR_SUCCESS)
+        {
+            SetLastError(uiResult); /* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */
+            msg(M_NONFATAL | M_ERRNO, "%s: MsiSetProperty(\"OPENVPNSERVICE\") failed", __FUNCTION__);
+            goto cleanup_OpenService;
+        }
     }
 
     uiResult = ERROR_SUCCESS;
 
-cleanup_CoInitialize:
-    if (bIsCoInitialized) CoUninitialize();
+cleanup_OpenService:
+    CloseServiceHandle(hService);
+cleanup_OpenSCManager:
+    CloseServiceHandle(hSCManager);
     return uiResult;
 }
 
 
+UINT __stdcall
+FindSystemInfo(_In_ MSIHANDLE hInstall)
+{
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
+    openvpnmsica_debug_popup(TEXT(__FUNCTION__));
+
+    BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
+
+    /* Set MSI session handle in TLS. */
+    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
+    s->hInstall = hInstall;
+
+    openvpnmsica_set_driver_certification(hInstall);
+    openvpnmsica_set_openvpnserv_state(hInstall);
+
+    if (bIsCoInitialized) CoUninitialize();
+    return ERROR_SUCCESS;
+}
+
+
 UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall)
 {