View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005365 | JEDI API & WSC Library | Windows Security Code Library (JWSCL) | public | 2010-10-14 10:19 | 2010-10-14 18:16 |
| Reporter | dangi | Assigned To | ChristianWimmer | ||
| Priority | normal | Severity | major | Reproducibility | sometimes |
| Status | resolved | Resolution | fixed | ||
| Summary | 0005365: Buffer overflow in TJwCredentialsPrompt.ShowModal() | ||||
| Description | To reproduce: - change MaxUserNameLength and/or MaxPasswordLength - set UserName and/or Password properties to values which exceed these limits - call TJwCredentialsPrompt.ShowModal() --> access violation Apply the patch to fix this | ||||
| Tags | No tags attached. | ||||
|
2010-10-14 10:19
|
20101014_JwsclCredentials_buffer_overflow_patch.txt (1,103 bytes)
Index: JwsclCredentials.pas
===================================================================
--- JwsclCredentials.pas (revision 1006)
+++ JwsclCredentials.pas (working copy)
@@ -415,17 +415,19 @@
pUser := TJwPChar(LocalAlloc(LMEM_ZEROINIT, fMaxUserNameLength *
sizeof(TJwChar)));
{$IFDEF UNICODE}
- CopyMemory(pUser,TJwPChar(fUserName),Length(fUserName)*sizeof(TJwChar));
+ CopyMemory(pUser, TJwPChar(fUserName),
+ Min(fMaxUserNameLength-1, Length(fUserName)) * sizeof(TJwChar));
{$ELSE}
- StrCopy(pUser, TJwPChar(fUserName + #0));
+ StrLCopy(pUser, TJwPChar(fUserName), fMaxUserNameLength-1);
{$ENDIF}
pPass := TJwPChar(LocalAlloc(LMEM_ZEROINIT, fMaxPasswordLength *
sizeof(TJwChar)));
{$IFDEF UNICODE}
- CopyMemory(pPass,TJwPChar(fPassword),Length(fPassword)*sizeof(TJwChar));
+ CopyMemory(pPass, TJwPChar(fPassword),
+ Min(fMaxPasswordLength-1, Length(fPassword)) * sizeof(TJwChar));
{$ELSE}
- StrCopy(pPass, TJwPChar(fPassword + #0));
+ StrLCopy(pPass, TJwPChar(fPassword), fMaxPasswordLength-1);
{$ENDIF}
|
|
|
Thank you for your report. I wonder if these MaxUserNameLength and MaxPasswordLength are necessary. Do you need them explicitly? To the MSDN doc of CredUICmdLinePromptForCredentials the strings are truncated to the maximum given size. However, on my Win7, the function returns 87 (Invalid Parameter) if the user inputs a username or password that is longer than the given max sizes. |
|
|
Since people want to enter any username and password I would suggest to ignore the max TJwCredentialsPrompt properties. |
|
|
I agree to remove MaxUserNameLength and MaxPasswordLength properties. As the command line option of ShowModal() does not work (see comment in code), I suggest to remove it also, because nobody is using it. |
|
|
I fixed the issue by ignoring max properties and assume always maximum possible values. Furthermore, I fixed the call ShowModal(True) (reading credentials on command line) which works now. For more information see ShowModal doc in code. The code is fixed in trunk (SVN revision >= 1025) and branch 0.9.3 (SVN revision >= 1026) |
|
|
Changes now raises an exception when the user click on Cancel (Windows 7 32 bits EN). I made a patch for branch 0.9.3 SVN rev # 1026 to handle this correctly and always zero and free login and password memory. |
|
2010-10-14 17:30
|
20101014_JwsclCredentials_ERROR_CANCELLED_patch.txt (9,104 bytes)
Index: JwsclCredentials.pas
===================================================================
--- JwsclCredentials.pas (revision 1026)
+++ JwsclCredentials.pas (working copy)
@@ -420,123 +420,134 @@
RsUNCredentialsEmptyServerName, 'ShowModal',
ClassName, RsUNCredentials, 0, False, []);
- GetMem(pUser, CRED_MAX_USERNAME_LENGTH * sizeof(TJwChar));
+ lResult := NO_ERROR;
- hRes := {$IFDEF UNICODE}StringCchCopyW{$ELSE}StringCchCopyA{$ENDIF}
- (pUser, CRED_MAX_USERNAME_LENGTH, TJwPChar(fUserName));
+ pUser := nil;
+ pPass := nil;
+ try
+ GetMem(pUser, CRED_MAX_USERNAME_LENGTH * sizeof(TJwChar));
- if FAILED(hRes) then
- begin
- raise EJwsclInvalidParameterException.CreateFmtEx(
- 'Property %0:s must be between %1:d and %2:d.', 'ShowModal',
- ClassName, RsUNCredentials, 0, hRes, ['UserName', 0, CRED_MAX_USERNAME_LENGTH]);
- end;
+ hRes := {$IFDEF UNICODE}StringCchCopyW{$ELSE}StringCchCopyA{$ENDIF}
+ (pUser, CRED_MAX_USERNAME_LENGTH, TJwPChar(fUserName));
- GetMem(pPass, CREDUI_MAX_PASSWORD_LENGTH * sizeof(TJwChar));
+ if FAILED(hRes) then
+ begin
+ raise EJwsclInvalidParameterException.CreateFmtEx(
+ 'Property %0:s must be between %1:d and %2:d.', 'ShowModal',
+ ClassName, RsUNCredentials, 0, hRes, ['UserName', 0, CRED_MAX_USERNAME_LENGTH]);
+ end;
- hRes := {$IFDEF UNICODE}StringCchCopyW{$ELSE}StringCchCopyA{$ENDIF}
- (pPass, CREDUI_MAX_PASSWORD_LENGTH, TJwPChar(fPassword));
- if FAILED(hRes) then
- begin
- raise EJwsclInvalidParameterException.CreateFmtEx(
- 'Property %0:s must be between %1:d and %2:d.', 'ShowModal',
- ClassName, RsUNCredentials, 0, hRes, ['Password', 0, CREDUI_MAX_PASSWORD_LENGTH]);
- end;
+ GetMem(pPass, CREDUI_MAX_PASSWORD_LENGTH * sizeof(TJwChar));
- FillChar(info, sizeof(info), 0);
- info.cbSize := sizeof(info);
- info.hwndParent := fParentWindow;
- info.pszMessageText := TJwPChar(fMessageText);
- info.pszCaptionText := TJwPChar(fCaption);
+ hRes := {$IFDEF UNICODE}StringCchCopyW{$ELSE}StringCchCopyA{$ENDIF}
+ (pPass, CREDUI_MAX_PASSWORD_LENGTH, TJwPChar(fPassword));
+ if FAILED(hRes) then
+ begin
+ raise EJwsclInvalidParameterException.CreateFmtEx(
+ 'Property %0:s must be between %1:d and %2:d.', 'ShowModal',
+ ClassName, RsUNCredentials, 0, hRes, ['Password', 0, CREDUI_MAX_PASSWORD_LENGTH]);
+ end;
- if Assigned(fBanner) then
- info.hbmBanner := fBanner.Handle;
+ FillChar(info, sizeof(info), 0);
+ info.cbSize := sizeof(info);
+ info.hwndParent := fParentWindow;
+ info.pszMessageText := TJwPChar(fMessageText);
+ info.pszCaptionText := TJwPChar(fCaption);
- aBool := fSaveCheck;
+ if Assigned(fBanner) then
+ info.hbmBanner := fBanner.Handle;
- if (CommandLine) then
- begin
- tempFlag := fFlags;
- //CredUICmdLinePromptForCredentials needs cfFlagsRequireSmartCard or cfFlagsExcludeCertificates
- if not (cfFlagsRequireSmartCard in Flags) then
- Include(tempFlag, cfFlagsExcludeCertificates)
- else
- Exclude(tempFlag, cfFlagsExcludeCertificates);
+ aBool := fSaveCheck;
- lResult :=
+ if (CommandLine) then
+ begin
+ tempFlag := fFlags;
+ //CredUICmdLinePromptForCredentials needs cfFlagsRequireSmartCard or cfFlagsExcludeCertificates
+ if not (cfFlagsRequireSmartCard in Flags) then
+ Include(tempFlag, cfFlagsExcludeCertificates)
+ else
+ Exclude(tempFlag, cfFlagsExcludeCertificates);
+
+ lResult :=
{$IFDEF UNICODE}
- CredUICmdLinePromptForCredentialsW
+ CredUICmdLinePromptForCredentialsW
{$ELSE}
- CredUICmdLinePromptForCredentialsA
+ CredUICmdLinePromptForCredentialsA
{$ENDIF}
- (TJwPChar(fServerName), //PCTSTR pszTargetName,
+ (TJwPChar(fServerName), //PCTSTR pszTargetName,
+ nil, //PCtxtHandle Reserved,
+ fAuthError, //DWORD dwAuthError,
+ pUser, //PCTSTR pszUserName,
+ CRED_MAX_USERNAME_LENGTH, //ULONG ulUserNameMaxChars,
+ pPass, //PCTSTR pszPassword,
+ CREDUI_MAX_PASSWORD_LENGTH, //ULONG ulPasswordMaxChars,
+ @aBool, //PBOOL pfSave,
+ TJwEnumMap.ConvertToCredentialFlag(tempFlag) //DWORD dwFlags
+ );
+ end
+ else
+ begin
+ lResult :=
+{$IFDEF UNICODE}
+ CredUIPromptForCredentialsW
+{$ELSE}
+ CredUIPromptForCredentialsA
+{$ENDIF}
+ (@info, //PCREDUI_INFO pUiInfo,
+ TJwPChar(fServerName), //PCTSTR pszTargetName,
nil, //PCtxtHandle Reserved,
fAuthError, //DWORD dwAuthError,
pUser, //PCTSTR pszUserName,
CRED_MAX_USERNAME_LENGTH, //ULONG ulUserNameMaxChars,
pPass, //PCTSTR pszPassword,
CREDUI_MAX_PASSWORD_LENGTH, //ULONG ulPasswordMaxChars,
- @aBool, //PBOOL pfSave,
- TJwEnumMap.ConvertToCredentialFlag(tempFlag) //DWORD dwFlags
+ aBool, //PBOOL pfSave,
+ TJwEnumMap.ConvertToCredentialFlag(fFlags) //DWORD dwFlags
);
- end
- else
- begin
- lResult :=
-{$IFDEF UNICODE}
- CredUIPromptForCredentialsW
-{$ELSE}
- CredUIPromptForCredentialsA
-{$ENDIF}
- (@info, //PCREDUI_INFO pUiInfo,
- TJwPChar(fServerName), //PCTSTR pszTargetName,
- nil, //PCtxtHandle Reserved,
- fAuthError, //DWORD dwAuthError,
- pUser, //PCTSTR pszUserName,
- CRED_MAX_USERNAME_LENGTH, //ULONG ulUserNameMaxChars,
- pPass, //PCTSTR pszPassword,
- CREDUI_MAX_PASSWORD_LENGTH, //ULONG ulPasswordMaxChars,
- aBool, //PBOOL pfSave,
- TJwEnumMap.ConvertToCredentialFlag(fFlags) //DWORD dwFlags
- );
- end;
+ end;
- if (lResult <> NO_ERROR) then
- begin
- FreeMem(pUser);
- FreeMem(pPass);
- SetLastError(lResult);
+ if (lResult <> NO_ERROR) then
+ begin
+ SetLastError(lResult);
- case lResult of
- ERROR_INVALID_FLAGS:
- raise EJwsclInvalidFlagsException.CreateFmtEx(
- RsUNCredentialsInvalidPropertyFlags, 'ShowModal', ClassName, RsUNCredentials,
- 0, true, []);
- ERROR_INVALID_PARAMETER:
- raise EJwsclInvalidParameterException.CreateFmtEx(
- RsUNCredentialsInvalidParametersCUIPFC, 'ShowModal',
- ClassName, RsUNCredentials, 0, true, []);
- ERROR_NO_SUCH_LOGON_SESSION: raise EJwsclNoSuchLogonSession.CreateFmtEx(
- RsUNCredentialsInvalidLogonSession, 'ShowModal', ClassName, RsUNCredentials,
- 0, true, []);
+ case lResult of
+ ERROR_CANCELLED: ; // User chose Cancel. The user name and password have not changed.
+ ERROR_INVALID_FLAGS:
+ raise EJwsclInvalidFlagsException.CreateFmtEx(
+ RsUNCredentialsInvalidPropertyFlags, 'ShowModal', ClassName, RsUNCredentials,
+ 0, true, []);
+ ERROR_INVALID_PARAMETER:
+ raise EJwsclInvalidParameterException.CreateFmtEx(
+ RsUNCredentialsInvalidParametersCUIPFC, 'ShowModal',
+ ClassName, RsUNCredentials, 0, true, []);
+ ERROR_NO_SUCH_LOGON_SESSION: raise EJwsclNoSuchLogonSession.CreateFmtEx(
+ RsUNCredentialsInvalidLogonSession, 'ShowModal', ClassName, RsUNCredentials,
+ 0, true, []);
+ else
+ raise EJwsclWinCallFailedException.CreateFmtWinCall(
+ RsWinCallFailed, 'ShowModal', ClassName, RsUNCredentials,
+ 0, true, 'CredUIPromptForCredentials',['CredUIPromptForCredentials']);
+ end;
+ end
else
- raise EJwsclWinCallFailedException.CreateFmtWinCall(
- RsWinCallFailed, 'ShowModal', ClassName, RsUNCredentials,
- 0, true, 'CredUIPromptForCredentials',['CredUIPromptForCredentials']);
+ if (lResult = NO_ERROR) then
+ begin
+ fUserName := TJwString(pUser);
+ fPassword := TJwString(pPass);
end;
- end
- else
- if (lResult = NO_ERROR) then
- begin
- fUserName := TJwString(pUser);
- fPassword := TJwString(pPass);
+ finally
+ if Assigned(pUser) then
+ begin
+ ZeroMemory(pUser, CRED_MAX_USERNAME_LENGTH * SizeOf(TJwChar));
+ FreeMem(pUser);
+ end;
- ZeroMemory(pUser, CRED_MAX_USERNAME_LENGTH * SizeOf(TJwChar));
- ZeroMemory(pPass, CREDUI_MAX_PASSWORD_LENGTH * SizeOf(TJwChar));
-
- FreeMem(pUser);
- FreeMem(pPass);
+ if Assigned(pPass) then
+ begin
+ ZeroMemory(pPass, CREDUI_MAX_PASSWORD_LENGTH * SizeOf(TJwChar));
+ FreeMem(pPass);
+ end;
end;
fSaveCheck := aBool;
@@ -550,7 +561,7 @@
if (
{$IFDEF UNICODE}
- CredUIConfirmCredentialsW
+ CredUIConfirmCredentialsW
{$ELSE}
CredUIConfirmCredentialsA
{$ENDIF}
|
|
|
I resolved the issue in 0.9.3 (rev 1027) The username and password are wrapped into JWSCL auto pointers to free and clear them automatically. The problem with cancel was that the command prompt version returns ERROR_CANCELLED if no smartcard was not found. |
|
|
Thank you. It work properly for me now using 0.9.3 (rev 1027). |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2010-10-14 10:19 | dangi | New Issue | |
| 2010-10-14 10:19 | dangi | Status | new => assigned |
| 2010-10-14 10:19 | dangi | Assigned To | => ChristianWimmer |
| 2010-10-14 10:19 | dangi | File Added: 20101014_JwsclCredentials_buffer_overflow_patch.txt | |
| 2010-10-14 13:20 | ChristianWimmer | Note Added: 0017891 | |
| 2010-10-14 13:20 | ChristianWimmer | Status | assigned => acknowledged |
| 2010-10-14 14:16 | ChristianWimmer | Note Added: 0017894 | |
| 2010-10-14 14:23 | dangi | Note Added: 0017897 | |
| 2010-10-14 16:18 | ChristianWimmer | Note Added: 0017899 | |
| 2010-10-14 16:18 | ChristianWimmer | Status | acknowledged => resolved |
| 2010-10-14 16:18 | ChristianWimmer | Resolution | open => fixed |
| 2010-10-14 17:30 | dangi | Note Added: 0017900 | |
| 2010-10-14 17:30 | dangi | Status | resolved => feedback |
| 2010-10-14 17:30 | dangi | Resolution | fixed => reopened |
| 2010-10-14 17:30 | dangi | File Added: 20101014_JwsclCredentials_ERROR_CANCELLED_patch.txt | |
| 2010-10-14 17:30 | dangi | Note Edited: 0017900 | |
| 2010-10-14 17:36 | ChristianWimmer | Note Edited: 0017900 | |
| 2010-10-14 17:42 | ChristianWimmer | Status | feedback => confirmed |
| 2010-10-14 17:56 | ChristianWimmer | Note Added: 0017902 | |
| 2010-10-14 17:56 | ChristianWimmer | Status | confirmed => feedback |
| 2010-10-14 18:09 | dangi | Note Added: 0017903 | |
| 2010-10-14 18:16 | ChristianWimmer | Status | feedback => resolved |
| 2010-10-14 18:16 | ChristianWimmer | Resolution | reopened => fixed |
