#3 Memory leak when URL-encoding or adding doublequote

[KSmith]

This is partly because of the declaration of pshk_exec_prog(int option, TCHAR *username, TCHAR *password), which means that the pointers are passed as values, so any changes to the pointers inside the method is not reflected to the caller.
The other problem is that the old memory allocations are not freed when replacing the string pointers. The old string values are not zeroed out either, so a security problem as well!
A third problem is that usernames (samAccountName) in Windows may contain spaces, so username should also be doublequoted to allow for this. Luckily, usernames may NOT contain doublequote, backslash, pipe or other chars with special meaning in the shell...

Part 1 of the fix is to change the method declaration into this (both in cpp and in .h):
int pshk_exec_prog(int option, TCHAR *&username, TCHAR *&password)

Part 2 is to add a few lines to free the old string values, when new are created:
SecureZeroMemory(password, _tcslen(password)); // Added
free(password); // Added
password = encodedStr; // Changed from: password = _tcsdup(encodedStr);
// Must remove the free(encodedStr) line...

Part 3 is to add a few lines after the doublequoting of the password:
encodedStr = (TCHAR *)calloc(_tcslen(username) * 2 + 3, sizeof(TCHAR));
j = 0;
encodedStr[j++] = _T('"');
for (i = 0; i < _tcslen(username); i++) {
encodedStr[j++] = username[i];
}
encodedStr[j] = _T('"');
SecureZeroMemory(username, _tcslen(username));
free(username);
username = encodedStr;

The suggested solution will work, but implies that the pshk_exec_prog knows how to free the string buffers forwared to it. Not really a good thing.
Another possible solution is to use _alloca instead of calloc, to allocate on the stack, but this mught be risky, and also would work with rawurlencode, since the stack space is freed when the method returns.
The best solution would probably be to use std::wstring or the ATL CString-classes and avoid memory pointer handling alltogether.

I hope these suggestions would be of use to you!

Kåre

[KSmith]

Description and suggested solution

[KSmith]

Sorry: Saw a typo after submitting:
"and also would work with rawurlencode" should really be:
"and also would NOT work with rawurlencode"

[Brian Clayton]

My apologies for the long delay, thank you for reporting this. I will have to get in the habit of checking this occasionally as I thought it would email me bug reports automatically. The memory leaks are rather embarrassing, I was responsible for all the orphaned pointers during my initial Unicode implementation. I must admit, at that point it had been a while since I had done much work in C and I got sloppy. I actually noticed this myself during my cleanup of the code to remove the legacy non-Unicode stuff, which was basically pointless now anyway. Unfortunately, I just now saw your comment. Using CString would be a good idea, and I will try to do that when I get a chance. Originally, this was written in C only so CString was not an option, but it would be more logical and easier to manage now that the project has been converted to C++. Right now, it is still done using calloc, but the orphaned pointers have been eliminated (and wiped). Also, I have implemented URLEncoding and double-quoting for the username as well, but it has to be enabled at compile time right now, just to retain the option of the old behavior (although I'm not sure why one would want to). I don't know why I didn't think to add that way back. To enable, simply unset RAW_USERNAME in passwdHk.h.