From 8eb06fa6975594488aad2dc30d5d7d3011c54ebb Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Fri, 13 Oct 2017 23:37:17 -0400 Subject: [PATCH 1/2] Correct parsing of the process ID returned by interatcive service Signed-off-by: Selva Nair --- openvpn.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openvpn.c b/openvpn.c index ea6726c..f080c67 100644 --- a/openvpn.c +++ b/openvpn.c @@ -1173,7 +1173,8 @@ OnService(connection_t *c, UNUSED char *msg) } p = buf + 11; - if (!err && swscanf (p, L"0x%08x\nProcess ID", &pid) == 1 && pid != 0) + /* next line is the pid if followed by "\nProcess ID" */ + if (!err && wcsstr(p, L"\nProcess ID") && swscanf (p, L"0x%08x", &pid) == 1 && pid != 0) { PrintDebug (L"Process ID of openvpn started by IService: %d", pid); c->hProcess = OpenProcess (PROCESS_TERMINATE|PROCESS_QUERY_INFORMATION, FALSE, pid); From 301a5e56449baa7bff367078f087a32bfff9ec86 Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Sat, 18 Nov 2017 10:14:25 -0500 Subject: [PATCH 2/2] Check for invalid characters in user inputs - Flag password and username input if these contain an invalid character (currently only embedded '\n' is disallowed). Shows a popup when OK is pressed so that the user can correct the input and resubmit. - Add an error message to the log when the management i/f returns ERROR for incorrectly parsed commands. Otherwise such errors go unnoticed. Note: IDS_ERR_INVALID_USERNAME/PASSWORD need translations. Reported and tested by: Florian Beier (H4ndl3 on github) Fixes Trac: #958 Signed-off-by: Selva Nair --- manage.c | 6 ++++ misc.c | 9 ++++++ misc.h | 1 + openvpn-gui-res.h | 4 +++ openvpn.c | 60 +++++++++++++++++++++++++++++++++------ res/openvpn-gui-res-en.rc | 3 +- 6 files changed, 74 insertions(+), 9 deletions(-) diff --git a/manage.c b/manage.c index 2891c78..37cbef4 100644 --- a/manage.c +++ b/manage.c @@ -340,6 +340,12 @@ OnManagement(SOCKET sk, LPARAM lParam) } else if (strncmp(line, "ERROR:", 6) == 0) { + /* Response sent to management is not processed. Log an error in status window */ + char buf[256]; + _snprintf_0(buf, "%lld,N,Previous command sent to management failed: %s", + (long long)time(NULL), line) + rtmsg_handler[log](c, buf); + if (cmd->handler) cmd->handler(c, NULL); UnqueueCommand(c); diff --git a/misc.c b/misc.c index b555aca..fb1e5d9 100644 --- a/misc.c +++ b/misc.c @@ -453,3 +453,12 @@ Widen(const char *utf8) return wstr; } + +/* Return false if input contains any characters in exclude */ +BOOL +validate_input(const WCHAR *input, const WCHAR *exclude) +{ + if (!exclude) + exclude = L"\n"; + return (wcspbrk(input, exclude) == NULL); +} diff --git a/misc.h b/misc.h index 3338368..f7f43cc 100644 --- a/misc.h +++ b/misc.h @@ -39,4 +39,5 @@ BOOL CheckFileAccess (const TCHAR *path, int access); BOOL Base64Encode(const char *input, int input_len, char **output); int Base64Decode(const char *input, char **output); WCHAR *Widen(const char *utf8); +BOOL validate_input(const WCHAR *input, const WCHAR *exclude); #endif diff --git a/openvpn-gui-res.h b/openvpn-gui-res.h index 90410f5..0ffb625 100644 --- a/openvpn-gui-res.h +++ b/openvpn-gui-res.h @@ -310,6 +310,10 @@ #define IDS_NFO_AUTH_PASS_RETRY 2150 #define IDS_NFO_KEY_PASS_RETRY 2151 +/* Invalid input errors */ +#define IDS_ERR_INVALID_PASSWORD_INPUT 2152 +#define IDS_ERR_INVALID_USERNAME_INPUT 2153 + /* Timer IDs */ #define IDT_STOP_TIMER 2500 /* Timer used to trigger force termination */ diff --git a/openvpn.c b/openvpn.c index f080c67..e9d6a0a 100644 --- a/openvpn.c +++ b/openvpn.c @@ -33,6 +33,7 @@ #include #include #include +#include #include "tray.h" #include "main.h" @@ -99,6 +100,22 @@ AppendTextToCaption (HANDLE hwnd, const WCHAR *str) SetWindowText (hwnd, new); } +/* + * Show an error tooltip with msg attached to the specified + * editbox handle. + */ +static void +show_error_tip(HWND editbox, const WCHAR *msg) +{ + EDITBALLOONTIP bt; + bt.cbStruct = sizeof(EDITBALLOONTIP); + bt.pszText = msg; + bt.pszTitle = L"Invalid input"; + bt.ttiIcon = TTI_ERROR_LARGE; + + SendMessage(editbox, EM_SHOWBALLOONTIP, 0, (LPARAM)&bt); +} + /* * Receive banner on connection to management interface * Format: @@ -364,13 +381,25 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) case IDOK: if (GetDlgItemTextW(hwndDlg, ID_EDT_AUTH_USER, username, _countof(username))) { + if (!validate_input(username, L"\n")) + { + show_error_tip(GetDlgItem(hwndDlg, ID_EDT_AUTH_USER), LoadLocalizedString(IDS_ERR_INVALID_USERNAME_INPUT)); + return 0; + } SaveUsername(param->c->config_name, username); } - if ( param->c->flags & FLAG_SAVE_AUTH_PASS && - GetDlgItemTextW(hwndDlg, ID_EDT_AUTH_PASS, password, _countof(password)) && - wcslen(password) ) + if (GetDlgItemTextW(hwndDlg, ID_EDT_AUTH_PASS, password, _countof(password))) { - SaveAuthPass(param->c->config_name, password); + if (!validate_input(password, L"\n")) + { + show_error_tip(GetDlgItem(hwndDlg, ID_EDT_AUTH_PASS), LoadLocalizedString(IDS_ERR_INVALID_PASSWORD_INPUT)); + SecureZeroMemory(password, sizeof(password)); + return 0; + } + if ( param->c->flags & FLAG_SAVE_AUTH_PASS && wcslen(password) ) + { + SaveAuthPass(param->c->config_name, password); + } SecureZeroMemory(password, sizeof(password)); } ManagementCommandFromInput(param->c, "username \"Auth\" \"%s\"", hwndDlg, ID_EDT_AUTH_USER); @@ -418,6 +447,7 @@ INT_PTR CALLBACK GenericPassDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) { auth_param_t *param; + WCHAR password[USER_PASS_LEN]; switch (msg) { @@ -467,6 +497,13 @@ GenericPassDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) switch (LOWORD(wParam)) { case IDOK: + if (GetDlgItemTextW(hwndDlg, ID_EDT_RESPONSE, password, _countof(password)) + && !validate_input(password, L"\n")) + { + show_error_tip(GetDlgItem(hwndDlg, ID_EDT_RESPONSE), LoadLocalizedString(IDS_ERR_INVALID_PASSWORD_INPUT)); + SecureZeroMemory(password, sizeof(password)); + return 0; + } if (param->flags & FLAG_CR_TYPE_CRV1) { /* send username */ @@ -586,11 +623,18 @@ PrivKeyPassDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) break; case IDOK: - if ((c->flags & FLAG_SAVE_KEY_PASS) && - GetDlgItemTextW(hwndDlg, ID_EDT_PASSPHRASE, passphrase, _countof(passphrase)) && - wcslen(passphrase) > 0) + if (GetDlgItemTextW(hwndDlg, ID_EDT_PASSPHRASE, passphrase, _countof(passphrase))) { - SaveKeyPass(c->config_name, passphrase); + if (!validate_input(passphrase, L"\n")) + { + show_error_tip(GetDlgItem(hwndDlg, ID_EDT_PASSPHRASE), LoadLocalizedString(IDS_ERR_INVALID_PASSWORD_INPUT)); + SecureZeroMemory(passphrase, sizeof(passphrase)); + return 0; + } + if ((c->flags & FLAG_SAVE_KEY_PASS) && wcslen(passphrase) > 0) + { + SaveKeyPass(c->config_name, passphrase); + } SecureZeroMemory(passphrase, sizeof(passphrase)); } ManagementCommandFromInput(c, "password \"Private Key\" \"%s\"", hwndDlg, ID_EDT_PASSPHRASE); diff --git a/res/openvpn-gui-res-en.rc b/res/openvpn-gui-res-en.rc index 4ce0060..07a09be 100644 --- a/res/openvpn-gui-res-en.rc +++ b/res/openvpn-gui-res-en.rc @@ -458,5 +458,6 @@ BEGIN IDS_NFO_AUTH_PASS_RETRY "Wrong username or password. Try again..." IDS_NFO_KEY_PASS_RETRY "Wrong password. Try again..." - + IDS_ERR_INVALID_PASSWORD_INPUT "Invalid character in password" + IDS_ERR_INVALID_USERNAME_INPUT "Invalid character in username" END