Use after free in Util : GetServerName (called by CreateUser)

[bevanweiss]

Hi all,
I wouldn't say that C/C++ is my strong suite, and perhaps there's tricks around the Microsoft memory allocations that I'm just not aware of

In the code chunk below, it appears there's two situations with a use after free on the server name.

1. It's returning pDomainControllerInfo->DomainControllerName (or DomainControllerName +2) as allocated memory, but it's already Free'd the memory allocated for them with the NetApiBufferFree usage.
2. If the DsGetDcName calls fail, we just repoint the servername back to the input domain string. But the caller now has two pointers to the same string.. and wouldn't know whether it should free one or both.

static void GetServerName(LPWSTR pwzDomain, LPWSTR* ppwzServerName)
{
    DWORD er = ERROR_SUCCESS;
    PDOMAIN_CONTROLLER_INFOW pDomainControllerInfo = NULL;

    if (pwzDomain && *pwzDomain)
    {
        er = ::DsGetDcNameW(NULL, (LPCWSTR)pwzDomain, NULL, NULL, NULL, &pDomainControllerInfo);
        if (RPC_S_SERVER_UNAVAILABLE == er)
        {
            // MSDN says, if we get the above error code, try again with the "DS_FORCE_REDISCOVERY" flag
            er = ::DsGetDcNameW(NULL, (LPCWSTR)pwzDomain, NULL, NULL, DS_FORCE_REDISCOVERY, &pDomainControllerInfo);
        }

        if (ERROR_SUCCESS == er && pDomainControllerInfo->DomainControllerName)
        {
            // Skip the \\ prefix if present.
            if ('\\' == *pDomainControllerInfo->DomainControllerName && '\\' == *pDomainControllerInfo->DomainControllerName + 1)
            {
                *ppwzServerName = pDomainControllerInfo->DomainControllerName + 2;
            }
            else
            {
                *ppwzServerName = pDomainControllerInfo->DomainControllerName;
            }
        }
        else
        {
            *ppwzServerName = pwzDomain;
        }
    }

    if (pDomainControllerInfo)
    {
        ::NetApiBufferFree((LPVOID)pDomainControllerInfo);
    }
}

https://github.com/wixtoolset/wix/blob/64fc1bc29460db8b525d2817078e1a40a197b4c9/src/ext/Util/ca/scaexec.cpp#L718-L754

Can someone correct me on my error here..?
It seems the safest thing to do would have been to declare pwzDomain as LPCWSTR, and then in all three exit cases do a _wcsdup instead of the assignment.
In this way when the NetApiBuffer is returned, we don't touch it afterwards, and there's no confusion in the caller around whether it should free ppwzServerName's string, or pwzDomain.

[BMurri]

@bevanweiss For not having C/C++ as your strong suite, you do have eagle eyes.

As of right now, there's only one caller to that method, and the caller does not explicitly free the returned string. In the case where ppwzServerName is pointed to pwzDomain, cleanup occurs when pwzDomain is cleaned up, but in the other two cases, yes, a use after free is absolutely happening. This code is probably working simply because the returned string is used before that freed space on the heap is reused (but that's a happy coincidence, not something we should ever rely upon). NetApiBufferFree does not clear the value of DomainControllerName before releasing the memory (likely because it's not considered particularly sensitive), but that's an implementation detail we should never rely upon.

You're right that the fix needs to be to dup the returned string and have the caller always explicitly free it. My vote would be to fix it. I'd be willing to submit a PR if the maintainers assign an issue to me.

[bevanweiss]

I was looking at sprinkling this function around a little more with my Create Group stuff (at least once when adding the group, but likely twice more for adding/removing membership of groups to other groups... just because it's relying on NetLocalGroupAdd(Del)Members which wants to talk directly to a server).

I believe there's a triage tomorrow, I'll raise an issue in Github for this now, so it should be available for triage then.