Re: GetTokenInformation() and FreeSid() at port/exec.c

Andrew Chernow wrote:
> TAKATSUKA Haruka wrote:
>> Hi.
>>
>> We found the unbalance of xxAlloc and xxFree at AddUserToDacl() in
>> src/port/exec.c (of current HEAD code).
>>
>> psidUser is a pointer of the element of a TOKEN_USER structure
>> allocated by  HeapAlloc().  The FreeSid() frees a SID allocated by
>> AllocateAndInitializeSid().  I think that it is correct to use
>> HeapFree(GetProcessHeap(), 0, pTokenUser).
>>
>> At present, a specific error, crash or trouble seems not to have
>> happened.
>>
>>
>> src/port/exec.c:748     AddUserToDacl()
>> src/port/exec.c:841      GetUserSid()
>>         pTokenUser = (PTOKEN_USER) HeapAlloc(GetProcessHeap(),
>> HEAP_ZERO_MEMORY, dwLength);
>>
>> src/port/exec.c:807      AddUserToDacl()
>>     FreeSid(psidUser);
>
> I quickly poked around and found what I believe to be two memory issues.
>
> 1. GetUserSid() uses HeapAlloc() to allocate a TOKEN_USER, but never
> calls HeapFree() if the function succeeds.  Instead, it pulls out the
> token's SID and returns it.  This is a memory leak.
>
> 2. The SID returned by GetUserSid() is incorrectly being passed to
> FreeSid() within AddUserToDacl()'s cleanup section.  This memory belongs
> to the TOKEN_USER allocated by HeapAlloc() in GetUserSid(), it cannot be
> passed to FreeSid.
>
> Quick question, Why HeapAlloc and LocalAlloc.  Why not use malloc?
>
> One solution would be to return a copy of the SID from GetUserSid and
> HeapFree the TOKEN_USER.
>
> Replace GetUserSid() line 869
>
> *ppSidUser = pTokenUser->User.Sid;
> return TRUE;
>
> With the below (error checking excluded)
>
> DWORD len = GetLengthSid(pTokenUser->User.Sid)
> *ppSidUser = (PSID) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len);
> CopySid(len, *ppSidUser, pTokenUser->User.Sid);
>
> // SID is copied, free TOKEN_USER
> HeapFree(GetProcessHeap(), 0, pTokenUser);
> return TRUE;
>
> Also, AddUserToDacl() line 807
>
> FreeSid(psidUser) should be HeapFree(GetProcessHeap(), 0, psidUser)
>
> in order to work with my suggested changes in GetUserSid().

How about something like this? I switched to using LocalAlloc() in all
places to be consistent, instead of mixing heap and local. (Though per
doc, LocalAlloc is actually a wrapper for HeapAlloc in win32).

--
 Magnus Hagander
 Self: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/src/port/exec.c
--- b/src/port/exec.c
***************
*** 804,810 **** AddUserToDacl(HANDLE hProcess)

  cleanup:
      if (psidUser)
!         FreeSid(psidUser);

      if (pacl)
          LocalFree((HLOCAL) pacl);
--- 804,810 ----

  cleanup:
      if (psidUser)
!         LocalFree((HLOCAL) psidUser);

      if (pacl)
          LocalFree((HLOCAL) pacl);
***************
*** 822,827 **** cleanup:
--- 822,830 ----
   * GetUserSid*PSID *ppSidUser, HANDLE hToken)
   *
   * Get the SID for the current user
+  *
+  * The caller of this function is responsible for calling LocalFree() on the
+  * returned SID area.
   */
  static BOOL
  GetUserSid(PSID *ppSidUser, HANDLE hToken)
***************
*** 838,844 **** GetUserSid(PSID *ppSidUser, HANDLE hToken)
      {
          if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
          {
!             pTokenUser = (PTOKEN_USER) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);

              if (pTokenUser == NULL)
              {
--- 841,847 ----
      {
          if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
          {
!             pTokenUser = (PTOKEN_USER) LocalAlloc(LPTR, dwLength);

              if (pTokenUser == NULL)
              {
***************
*** 859,872 **** GetUserSid(PSID *ppSidUser, HANDLE hToken)
                               dwLength,
                               &dwLength))
      {
!         HeapFree(GetProcessHeap(), 0, pTokenUser);
          pTokenUser = NULL;

          log_error("could not get token information: %lu", GetLastError());
          return FALSE;
      }

!     *ppSidUser = pTokenUser->User.Sid;
      return TRUE;
  }

--- 862,895 ----
                               dwLength,
                               &dwLength))
      {
!         LocalFree(pTokenUser);
          pTokenUser = NULL;

          log_error("could not get token information: %lu", GetLastError());
          return FALSE;
      }

!     /*
!      * Need to copy the data to a separate buffer, so we can free our buffer
!      * and let the caller free only the sid part.
!      */
!     dwLength = GetLengthSid(pTokenUser->User.Sid);
!     *ppSidUser = (PSID) LocalAlloc(LPTR, dwLength);
!     if (!*ppSidUser)
!     {
!         LocalFree(pTokenUser);
!         log_error("could not allocate %lu bytes of memory", dwLength);
!         return FALSE;
!     }
!     if (!CopySid(dwLength, *ppSidUser, pTokenUser->User.Sid))
!     {
!         LocalFree(*ppSidUser);
!         LocalFree(pTokenUser);
!         log_error("could not copy SID: %lu", GetLastError());
!         return FALSE;
!     }
!
!     LocalFree(pTokenUser);
      return TRUE;
  }