Re: [ros-dev] [ros-diffs] 01/01: [ADVAPI32][SERVICES] Add (dummy) password encryption/decryption functions to CreateServiceA/W and ChangeServiceConfigA/W in order to prepare to pass encrypted password

2018-09-18 Thread Eric Kohl 
Hello Pierre,

thanks for the hint! I'll replace ZeroMemory() in the next patch.

Regards,
Eric


Am 17.09.2018 um 22:15 schrieb Pierre Schweitzer:
> Hi Eric,
> 
> For security reason, when freeing your password buffers (see below), you
> shouldn't be using ZeroMemory() before free, but SecureZeroMemory() to
> zero the buffer. The first one can be optimized (and thus removed) by
> the compiler. The second cannot.
> 
> Cheers,
> Pierre
> 
> Le 17/09/2018 à 16:36, Eric Kohl a écrit :
>> https://git.reactos.org/?p=reactos.git;a=commitdiff;h=5e2c4657ca10dea1154cb43f16ee6962999ac7a4
>>
>> commit 5e2c4657ca10dea1154cb43f16ee6962999ac7a4
>> Author: Eric Kohl 
>> AuthorDate: Mon Sep 17 16:34:48 2018 +0200
>> Commit: Eric Kohl 
>> CommitDate: Mon Sep 17 16:34:48 2018 +0200
>>
>> [ADVAPI32][SERVICES] Add (dummy) password encryption/decryption 
>> functions to CreateServiceA/W and ChangeServiceConfigA/W in order to prepare 
>> to pass encrypted passwords to the service manager
>> ---
>>  base/system/services/config.c|  24 
>>  base/system/services/rpcserver.c |  49 ++--
>>  base/system/services/services.h  |   7 +++
>>  dll/win32/advapi32/service/scm.c | 123 
>> ---
>>  4 files changed, 177 insertions(+), 26 deletions(-)
>>
>> diff --git a/base/system/services/rpcserver.c 
>> b/base/system/services/rpcserver.c
>> index aa64233350..454181bb66 100644
>> --- a/base/system/services/rpcserver.c
>> +++ b/base/system/services/rpcserver.c
>> @@ -2216,12 +2232,23 @@ RChangeServiceConfigW(
>>  dwError = ERROR_SUCCESS;
>>  
>>  if (dwError != ERROR_SUCCESS)
>> +{
>> +DPRINT1("ScmSetServicePassword failed (Error %lu)\n", 
>> dwError);
>>  goto done;
>> +}
>>  }
>>  }
>>  }
>>  
>>  done:
>> +if (lpClearTextPassword != NULL)
>> +{
>> +/* Wipe and release the password buffer */
>> +ZeroMemory(lpClearTextPassword,
>> +   (wcslen(lpClearTextPassword) + 1) * sizeof(WCHAR));
>> +HeapFree(GetProcessHeap(), 0, lpClearTextPassword);
>> +}
>> +
>>  if (hServiceKey != NULL)
>>  RegCloseKey(hServiceKey);
>>  
>> @@ -2612,6 +2645,14 @@ done:
>>  if (hServiceKey != NULL)
>>  RegCloseKey(hServiceKey);
>>  
>> +if (lpClearTextPassword != NULL)
>> +{
>> +/* Wipe and release the password buffer */
>> +ZeroMemory(lpClearTextPassword,
>> +   (wcslen(lpClearTextPassword) + 1) * sizeof(WCHAR));
>> +HeapFree(GetProcessHeap(), 0, lpClearTextPassword);
>> +}
>> +
>>  if (dwError == ERROR_SUCCESS)
>>  {
>>  DPRINT("hService %p\n", hServiceHandle);
> 
> 
> 
> ___
> Ros-dev mailing list
> Ros-dev@reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
> 


___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] 01/01: [ADVAPI32][SERVICES] Add (dummy) password encryption/decryption functions to CreateServiceA/W and ChangeServiceConfigA/W in order to prepare to pass encrypted password

2018-09-17 Thread Pierre Schweitzer 
Hi Eric,

For security reason, when freeing your password buffers (see below), you
shouldn't be using ZeroMemory() before free, but SecureZeroMemory() to
zero the buffer. The first one can be optimized (and thus removed) by
the compiler. The second cannot.

Cheers,
Pierre

Le 17/09/2018 à 16:36, Eric Kohl a écrit :
> https://git.reactos.org/?p=reactos.git;a=commitdiff;h=5e2c4657ca10dea1154cb43f16ee6962999ac7a4
> 
> commit 5e2c4657ca10dea1154cb43f16ee6962999ac7a4
> Author: Eric Kohl 
> AuthorDate: Mon Sep 17 16:34:48 2018 +0200
> Commit: Eric Kohl 
> CommitDate: Mon Sep 17 16:34:48 2018 +0200
> 
> [ADVAPI32][SERVICES] Add (dummy) password encryption/decryption functions 
> to CreateServiceA/W and ChangeServiceConfigA/W in order to prepare to pass 
> encrypted passwords to the service manager
> ---
>  base/system/services/config.c|  24 
>  base/system/services/rpcserver.c |  49 ++--
>  base/system/services/services.h  |   7 +++
>  dll/win32/advapi32/service/scm.c | 123 
> ---
>  4 files changed, 177 insertions(+), 26 deletions(-)
> 
> diff --git a/base/system/services/rpcserver.c 
> b/base/system/services/rpcserver.c
> index aa64233350..454181bb66 100644
> --- a/base/system/services/rpcserver.c
> +++ b/base/system/services/rpcserver.c
> @@ -2216,12 +2232,23 @@ RChangeServiceConfigW(
>  dwError = ERROR_SUCCESS;
>  
>  if (dwError != ERROR_SUCCESS)
> +{
> +DPRINT1("ScmSetServicePassword failed (Error %lu)\n", 
> dwError);
>  goto done;
> +}
>  }
>  }
>  }
>  
>  done:
> +if (lpClearTextPassword != NULL)
> +{
> +/* Wipe and release the password buffer */
> +ZeroMemory(lpClearTextPassword,
> +   (wcslen(lpClearTextPassword) + 1) * sizeof(WCHAR));
> +HeapFree(GetProcessHeap(), 0, lpClearTextPassword);
> +}
> +
>  if (hServiceKey != NULL)
>  RegCloseKey(hServiceKey);
>  
> @@ -2612,6 +2645,14 @@ done:
>  if (hServiceKey != NULL)
>  RegCloseKey(hServiceKey);
>  
> +if (lpClearTextPassword != NULL)
> +{
> +/* Wipe and release the password buffer */
> +ZeroMemory(lpClearTextPassword,
> +   (wcslen(lpClearTextPassword) + 1) * sizeof(WCHAR));
> +HeapFree(GetProcessHeap(), 0, lpClearTextPassword);
> +}
> +
>  if (dwError == ERROR_SUCCESS)
>  {
>  DPRINT("hService %p\n", hServiceHandle);

-- 
Pierre Schweitzer 
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.



signature.asc
Description: OpenPGP digital signature
___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev