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 passwords to the service manager

[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=5e2c4657ca10dea1154cb…
>  
>  commit 5e2c4657ca10dea1154cb43f16ee6962999ac7a4
>  Author:     Eric Kohl <eric.kohl(a)reactos.org>
>  AuthorDate: Mon Sep 17 16:34:48 2018 +0200
>  Commit:     Eric Kohl <eric.kohl(a)reactos.org>
>  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 <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.

[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=5e2c4657ca10dea1154cb…
> >
> >  commit 5e2c4657ca10dea1154cb43f16ee6962999ac7a4
> >  Author:     Eric Kohl <eric.kohl(a)reactos.org>
> >  AuthorDate: Mon Sep 17 16:34:48 2018 +0200
> >  Commit:     Eric Kohl <eric.kohl(a)reactos.org>
> >  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(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>