Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support static-challenge "concat" option #701

Merged
merged 2 commits into from
Sep 10, 2024
+17 −5
Merged

Support static-challenge "concat" option #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ed9eba
Support concatenating response with password
selvanair Jun 10, 2024
d1dd944
Parse new static-challenge format option from management interface
selvanair Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions openvpn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam)
if (RecallAuthPass(param->c->config_name, password))
{
SetDlgItemTextW(hwndDlg, ID_EDT_AUTH_PASS, password);
if (username[0] != L'\0' && !(param->flags & FLAG_CR_TYPE_SCRV1)
if (username[0] != L'\0' && !(param->flags & (FLAG_CR_TYPE_SCRV1|FLAG_CR_TYPE_CONCAT))
&& password[0] != L'\0' && param->c->failed_auth_attempts == 0)
{
/* user/pass available and no challenge response needed: skip dialog
Expand All @@ -605,7 +605,7 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam)
{
SendMessage(GetDlgItem(hwndDlg, ID_EDT_AUTH_PASS), EM_SETSEL, 0, MAKELONG(0, -1));
}
else if (param->flags & FLAG_CR_TYPE_SCRV1)
else if (param->flags & (FLAG_CR_TYPE_SCRV1|FLAG_CR_TYPE_CONCAT))
{
SetFocus(GetDlgItem(hwndDlg, ID_EDT_AUTH_CHALLENGE));
}
Expand Down Expand Up @@ -662,7 +662,7 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam)
/* enable OK button only if username and either password or response are filled */
BOOL enableOK = GetWindowTextLength(GetDlgItem(hwndDlg, ID_EDT_AUTH_USER))
&& (GetWindowTextLength(GetDlgItem(hwndDlg, ID_EDT_AUTH_PASS))
|| ((param->flags & FLAG_CR_TYPE_SCRV1)
|| ((param->flags & (FLAG_CR_TYPE_SCRV1|FLAG_CR_TYPE_CONCAT))
&& GetWindowTextLength(GetDlgItem(hwndDlg, ID_EDT_AUTH_CHALLENGE)))
);
EnableWindow(GetDlgItem(hwndDlg, IDOK), enableOK);
Expand Down Expand Up @@ -706,9 +706,19 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam)
{
SaveAuthPass(param->c->config_name, password);
}
if (param->flags & FLAG_CR_TYPE_CONCAT)
{
GetDlgItemTextW(hwndDlg, ID_EDT_AUTH_CHALLENGE, password + wcslen(password), _countof(password)-wcslen(password));
SetDlgItemTextW(hwndDlg, ID_EDT_AUTH_PASS, password);
/* erase potentially secret contents in the response text box */
memset(password, L'x', wcslen(password));
SetDlgItemTextW(hwndDlg, ID_EDT_AUTH_CHALLENGE, password);
Copy link
Contributor

@cron2 cron2 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand this code here - the GetDlgItemTextW() seems clear to me ("if we have CONCAT, read the text field from the window and put it after the existing content in password = concat).

What I'm unclear on is the SetDlgItemTextW() part - so we put the concatenated password into the
ID_EDT_AUTH_PASS field of the window, and a full set of x (of length password+challenge) into the ID_EDT_AUTH_CHALLENGE field?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's because later in ManagementCommandFromInput() we get the value of password+auth_challenge from ID_EDT_AUTH_PASS field - so we need that field to contain those concatenated values. And we do erase the content in ID_EDT_AUTH_CHALLENGE because ManagementCommandFromInput() does it only for ID_EDT_AUTH_PASS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as lev said. This allows one to reuse the password submission code by re-writing the password filed with password+OTP. A less "hacky" way would be to add a new function "ManagementComamndFromTwoInputs()" duplicating most of "ManagementCommandFromInput()"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks. I only half-understood this - I saw "ok, it's appending the CR to the password, to be used later down" and then totally overlooked that password[] is actually x-ed out right after :-) - makes sense.

LGTM.

}

SecureZeroMemory(password, sizeof(password));
}
ManagementCommandFromInput(param->c, "username \"Auth\" \"%s\"", hwndDlg, ID_EDT_AUTH_USER);

if (param->flags & FLAG_CR_TYPE_SCRV1)
{
ManagementCommandFromTwoInputsBase64(param->c, "password \"Auth\" \"SCRV1:%s:%s\"", hwndDlg, ID_EDT_AUTH_PASS, ID_EDT_AUTH_CHALLENGE);
Expand Down Expand Up @@ -1414,8 +1424,9 @@ OnPassword(connection_t *c, char *msg)
}
else if ( (chstr = strstr(msg, "SC:")) && strlen(chstr) > 5)
{
param->flags |= FLAG_CR_TYPE_SCRV1;
param->flags |= (*(chstr + 3) != '0') ? FLAG_CR_ECHO : 0;
ULONG flags = strtoul(chstr+3, NULL, 10);
param->flags |= (flags & 0x2) ? FLAG_CR_TYPE_CONCAT : FLAG_CR_TYPE_SCRV1;
param->flags |= (flags & 0x1) ? FLAG_CR_ECHO : 0;
param->str = strdup(chstr + 5);
LocalizedDialogBoxParamEx(ID_DLG_AUTH_CHALLENGE, c->hwndStatus, UserAuthDialogFunc, (LPARAM) param);
}
Expand Down
1 change: 1 addition & 0 deletions openvpn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ void WriteStatusLog(connection_t *c, const WCHAR *prefix, const WCHAR *line, BOO
#define FLAG_STRING_PKCS11 0x20 /* PKCS11 id needed */
#define FLAG_PASS_PKEY 0x40 /* Private key password needed */
#define FLAG_CR_TYPE_CRTEXT 0x80 /* crtext */
#define FLAG_CR_TYPE_CONCAT 0x100 /* concatenate otp with password */

typedef struct {
connection_t *c;
Expand Down
Loading