address codeQL warnings (#598)

* codeql fixes

* fix type mismatches

* fix pointers in w32_time methods

* fixes for codeQL warnings

* modify checks for codeql warnings

* add comments for codeql suppressions

* additional codeql fixes and suppressions

* add codeql fixes

* add comments for codeql

* add comments for codeql

* switch from debug to error log messages

* fix another merge conflict

fix line endings in gss-sspi.c

* add null check in channels.c

* address PR feedback

* address additional review feedback

* add CodeQL comments to common code

* fix unittest-win32compat

* fix unit test

* address review feedback

* remove suppression

Author: tgauth

addr.c

@@ -396,7 +396,7 @@ addr_pton_cidr(const char *p, struct xaddr *n, u_int *l)
 	if ((mp = strchr(addrbuf, '/')) != NULL) {
 		*mp = '\0';
 		mp++;
-		masklen = strtoul(mp, &cp, 10);
+		masklen = strtoul(mp, &cp, 10); // CodeQL [SM02313]: strtoul will initialize cp
 		if (*mp < '0' || *mp > '9' || *cp != '\0' || masklen > 128)
 			return -1;
 	}

auth2-passwd.c

@@ -66,7 +66,7 @@ userauth_passwd(struct ssh *ssh, const char *method)
 
 	if (change)
 		logit("password change not supported");
-	else if (PRIVSEP(auth_password(ssh, password)) == 1)
+	else if (PRIVSEP(auth_password(ssh, password)) == 1) // CodeQL [SM01714] false positive: password is null terminated
 		authenticated = 1;
 	freezero(password, len);
 	return authenticated;

channels.c

@@ -1175,8 +1175,9 @@ x11_open_helper(struct ssh *ssh, struct sshbuf *b)
 		return 0;
 
 	/* Parse the lengths of variable-length fields. */
-	ucp = sshbuf_mutable_ptr(b);
-	if (ucp[0] == 0x42) {	/* Byte order MSB first. */
+	if ((ucp = sshbuf_mutable_ptr(b)) == NULL) // fix CodeQL SM02311
+		return 0;
+	if (ucp[0] == 0x42) { /* Byte order MSB first. */
 		proto_len = 256 * ucp[6] + ucp[7];
 		data_len = 256 * ucp[8] + ucp[9];
 	} else if (ucp[0] == 0x6c) {	/* Byte order LSB first. */
@@ -1291,6 +1292,10 @@ channel_decode_socks4(Channel *c, struct sshbuf *input, struct sshbuf *output)
 	if (have < len)
 		return 0;
 	p = sshbuf_ptr(input);
+	if (p == NULL) { // fix CodeQL SM02311 
+		error("channel %d: invalid input", c->self);
+		return -1;
+	}
 
 	need = 1;
 	/* SOCKS4A uses an invalid IP address 0.0.0.x */
@@ -1324,7 +1329,7 @@ channel_decode_socks4(Channel *c, struct sshbuf *input, struct sshbuf *output)
 	}
 	have = sshbuf_len(input);
 	p = sshbuf_ptr(input);
-	if (memchr(p, '\0', have) == NULL) {
+	if (p == NULL || memchr(p, '\0', have) == NULL) { // fix CodeQL SM02311 
 		error("channel %d: decode socks4: unterminated user", c->self);
 		return -1;
 	}
@@ -1342,7 +1347,7 @@ channel_decode_socks4(Channel *c, struct sshbuf *input, struct sshbuf *output)
 	} else {				/* SOCKS4A: two strings */
 		have = sshbuf_len(input);
 		p = sshbuf_ptr(input);
-		if (memchr(p, '\0', have) == NULL) {
+		if (p == NULL || memchr(p, '\0', have) == NULL) { // fix CodeQL SM02311 
 			error("channel %d: decode socks4a: host not nul "
 			    "terminated", c->self);
 			return -1;
@@ -1406,7 +1411,7 @@ channel_decode_socks5(Channel *c, struct sshbuf *input, struct sshbuf *output)
 
 	debug2("channel %d: decode socks5", c->self);
 	p = sshbuf_ptr(input);
-	if (p[0] != 0x05)
+	if (p == NULL || p[0] != 0x05) // fix CodeQL SM02311 
 		return -1;
 	have = sshbuf_len(input);
 	if (!(c->flags & SSH_SOCKS5_AUTHDONE)) {
@@ -1559,6 +1564,8 @@ channel_pre_dynamic(struct ssh *ssh, Channel *c)
 	}
 	/* try to guess the protocol */
 	p = sshbuf_ptr(c->input);
+	if (p == NULL) // fix CodeQL SM02311 
+		return;
 	/* XXX sshbuf_peek_u8? */
 	switch (p[0]) {
 	case 0x04:
@@ -1621,6 +1628,8 @@ channel_before_prepare_io_rdynamic(struct ssh *ssh, Channel *c)
 		return;
 	/* try to guess the protocol */
 	p = sshbuf_ptr(c->output);
+	if (p == NULL) // fix CodeQL SM02311 
+		return;
 	switch (p[0]) {
 	case 0x04:
 		/* switch input/output for reverse forwarding */

contrib/win32/win32compat/ansiprsr.c

@@ -670,7 +670,7 @@ ParseANSI(unsigned char * pszBuffer, unsigned char * pszBufferEnd, unsigned char
 					}
 				} else if (iParam[0] == 6) {
 					char * szStatus = GetCursorPositionReport();
-					if (respbuf != NULL) {
+					if (szStatus != NULL && respbuf != NULL) {
 						*respbuf = szStatus;
 						if (resplen != NULL)
 							*resplen = strlen(szStatus);

contrib/win32/win32compat/console.c

@@ -182,7 +182,7 @@ ConEnterRawMode()
 	if (FALSE == isConHostParserEnabled || !SetConsoleMode(GetConsoleOutputHandle(), dwAttributes)) /* Windows NT */
 		isAnsiParsingRequired = TRUE;
 
-	GetConsoleScreenBufferInfo(GetConsoleOutputHandle(), &csbi);
+	BOOL gcsbRet = GetConsoleScreenBufferInfo(GetConsoleOutputHandle(), &csbi);
 
 	/* We track the view port, if conpty is not supported */
 	if (!is_conpty_supported())
@@ -192,6 +192,12 @@ ConEnterRawMode()
 	 *  so that the clearscreen will not erase any lines.
 	 */
 	if (TRUE == isAnsiParsingRequired) {
+		if (gcsbRet == 0)
+		{
+			dwRet = GetLastError();
+			error("GetConsoleScreenBufferInfo on GetConsoleOutputHandle() failed with %d", dwRet);
+			return;
+		}
 		SavedViewRect = csbi.srWindow;
 		debug("console doesn't support the ansi parsing");
 	} else {
@@ -621,12 +627,17 @@ ConWriteString(char* pszString, int cbString)
 	if ((needed = MultiByteToWideChar(CP_UTF8, 0, pszString, cbString, NULL, 0)) == 0 ||
 	    (utf16 = malloc(needed * sizeof(wchar_t))) == NULL ||
 	    (cnt = MultiByteToWideChar(CP_UTF8, 0, pszString, cbString, utf16, needed)) == 0) {
-		Result = (DWORD)printf_s(pszString);
-	} else {
+		const char* pszStringConst = pszString;
+		Result = (DWORD)printf_s(pszStringConst);
+	}
+	else {
 		if (GetConsoleOutputHandle())
 			WriteConsoleW(GetConsoleOutputHandle(), utf16, cnt, &Result, 0);
 		else
-			Result = (DWORD)wprintf_s(utf16);
+		{
+			const wchar_t* utf16Const = utf16;
+			Result = (DWORD)wprintf_s(utf16Const);
+		}
 	}
 
 	if (utf16)

contrib/win32/win32compat/fileio.c

@@ -1189,7 +1189,7 @@ fileio_readlink(const char *path, char *buf, size_t bufsiz)
 	}
 
 	/* allocate the maximum possible size the reparse buffer size could be */
-	reparse_buffer = (PREPARSE_DATA_BUFFER_SYMLINK)malloc(MAXIMUM_REPARSE_DATA_BUFFER_SIZE);
+	reparse_buffer = (PREPARSE_DATA_BUFFER_SYMLINK)malloc(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); // CodeQL [SM02320]: DeviceIoControl will set reparse_buffer
 	if (reparse_buffer == NULL) {
 		errno = ENOMEM;
 		goto cleanup;

contrib/win32/win32compat/gss-sspi.c

@@ -444,8 +444,10 @@ gss_acquire_cred(_Out_ OM_uint32 *minor_status, _In_opt_ gss_name_t desired_name
 	/* determine expiration if requested */
 	if (time_rec != NULL) {
 		FILETIME current_time;
-		SystemTimeToFileTime(&current_time_system, &current_time);
-		*time_rec = (OM_uint32) (expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		if (SystemTimeToFileTime(&current_time_system, &current_time) != 0)
+			*time_rec = (OM_uint32)(expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		else
+			error("SystemTimeToFileTime failed with %d", GetLastError());
 	}
 
 	/* set actual supported mechs if requested */
@@ -597,8 +599,10 @@ gss_init_sec_context(
 	/* if requested, translate the expiration time to number of second */
 	if (time_rec != NULL) {
 		FILETIME current_time;
-		SystemTimeToFileTime(&current_time_system, &current_time);
-		*time_rec = (OM_uint32)(expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		if (SystemTimeToFileTime(&current_time_system, &current_time) != 0)
+			*time_rec = (OM_uint32)(expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		else
+			error("SystemTimeToFileTime failed with %d", GetLastError());
 	}
 
 	/* if requested, return the supported mechanism oid */
@@ -907,8 +911,10 @@ gss_accept_sec_context(_Out_ OM_uint32 * minor_status, _Inout_opt_ gss_ctx_id_t
 	/* if requested, translate the expiration time to number of second */
 	if (time_rec != NULL) {
 		FILETIME current_time;
-		SystemTimeToFileTime(&current_time_system, &current_time);
-		*time_rec = (OM_uint32)(expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		if (SystemTimeToFileTime(&current_time_system, &current_time) != 0)
+			*time_rec = (OM_uint32)(expiry.QuadPart - ((PLARGE_INTEGER)&current_time)->QuadPart) / 10000;
+		else
+			error("SystemTimeToFileTime failed with %d", GetLastError());
 	}
 
 	/* only do checks on the finalized context (no continue needed) */
@@ -1072,6 +1078,11 @@ ssh_gssapi_krb5_userok(ssh_gssapi_client *client, char *name)
 	 * onto the next available method.
 	 */
 	struct passwd * user = getpwnam(name);
+	if (user == NULL)
+	{
+		error("sspi getpwnam failed to get user from user-provided, resolved user '%s'", name);
+		return 0;
+	}
 	if (_stricmp(client->displayname.value, user->pw_name) != 0) {
 		/* check failed */
 		debug("sspi user '%s' did not match user-provided, resolved user '%s'", 

contrib/win32/win32compat/inc/time.h

@@ -1,4 +1,9 @@
 #include "crtheaders.h"
 #include TIME_H
 
+#define localtime w32_localtime
+#define ctime w32_ctime
+
 struct tm *localtime_r(const time_t *, struct tm *);
+struct tm *w32_localtime(const time_t* sourceTime);
+char *w32_ctime(const time_t* sourceTime);

contrib/win32/win32compat/misc.c

@@ -410,8 +410,10 @@ char*
 		* stop reading until reach '\n' or the converted utf8 string length is n-1
 		*/
 		do {
-			if (str_tmp)
-				free(str_tmp);			
+			if (str_tmp) {
+				free(str_tmp);
+				str_tmp = NULL;
+			}
 			if (fgetws(str_w, 2, stream) == NULL)
 				goto cleanup;
 			if ((str_tmp = utf16_to_utf8(str_w)) == NULL) {
@@ -1485,6 +1487,28 @@ localtime_r(const time_t *timep, struct tm *result)
 	return localtime_s(result, timep) == 0 ? result : NULL;
 }
 
+struct tm *
+w32_localtime(const time_t* sourceTime)
+{
+	struct tm* destTime = (struct tm*)malloc(sizeof(struct tm));
+	if (destTime == NULL)
+	{
+		return NULL;
+	}
+	return localtime_s(destTime, sourceTime) == 0 ? destTime : NULL;
+}
+
+char*
+w32_ctime(const time_t* sourceTime)
+{
+	char *destTime = malloc(26);
+	if (destTime == NULL)
+	{
+		return NULL;
+	}
+	return ctime_s(destTime, 26, sourceTime) == 0 ? destTime : NULL;
+}
+
 void
 freezero(void *ptr, size_t sz)
 {
@@ -1578,8 +1602,11 @@ am_system()
 
 	if (OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &proc_token) == FALSE ||
 		GetTokenInformation(proc_token, TokenUser, NULL, 0, &info_len) == TRUE ||
-		(info = (TOKEN_USER*)malloc(info_len)) == NULL ||
-		GetTokenInformation(proc_token, TokenUser, info, info_len, &info_len) == FALSE)
+		(info = (TOKEN_USER*)malloc(info_len)) == NULL) {
+			fatal("unable to know if I am running as system");
+	}
+
+	if (GetTokenInformation(proc_token, TokenUser, info, info_len, &info_len) == FALSE) 
 		fatal("unable to know if I am running as system");
 
 	if (IsWellKnownSid(info->User.Sid, WinLocalSystemSid))
@@ -1609,7 +1636,7 @@ lookup_sid(const wchar_t* name_utf16, PSID psid, DWORD * psid_len)
 	wchar_t* name_utf16_modified = NULL;
 	BOOL resolveAsAdminsSid = 0, r;
 
-	LookupAccountNameW(NULL, name_utf16, NULL, &sid_len, dom, &dom_len, &n_use);
+	LookupAccountNameW(NULL, name_utf16, NULL, &sid_len, dom, &dom_len, &n_use); // CodeQL [SM02313]: false positive n_use will not be uninitialized
 
 	if (sid_len == 0 && _wcsicmp(name_utf16, L"administrators") == 0) {
 		CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, NULL, &sid_len);
@@ -1663,6 +1690,12 @@ lookup_sid(const wchar_t* name_utf16, PSID psid, DWORD * psid_len)
 		debug3_f("local user name is same as machine name");
 		size_t name_size = wcslen(name_utf16) * 2U + 2U;
 		name_utf16_modified = malloc(name_size * sizeof(wchar_t));
+		if (name_utf16_modified == NULL)
+		{
+			errno = ENOMEM;
+			error_f("Failed to allocate memory");
+			goto cleanup;
+		}
 		name_utf16_modified[0] = L'\0';
 		wcscat_s(name_utf16_modified, name_size, name_utf16);
 		wcscat_s(name_utf16_modified, name_size, L"\\");
@@ -1709,15 +1742,15 @@ get_sid(const char* name)
 	}
 	else {
 		if (OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token) == FALSE ||
-		    GetTokenInformation(token, TokenUser, NULL, 0, &info_len) == TRUE) {
+		    GetTokenInformation(token, TokenUser, NULL, 0, &info_len) == TRUE) { // CodeQL [SM02320]: GetTokenInformation will initialize info
 			errno = EOTHER;
 			goto cleanup;
 		}
 
-		if ((info = (TOKEN_USER*)malloc(info_len)) == NULL) {
+		if ((info = (TOKEN_USER*)malloc(info_len)) == NULL) { 
 			errno = ENOMEM;
 			goto cleanup;
-		}
+		};
 
 		if (GetTokenInformation(token, TokenUser, info, info_len, &info_len) == FALSE) {
 			errno = errno_from_Win32LastError();

contrib/win32/win32compat/shell-host.c

@@ -891,7 +891,7 @@ ProcessEvent(void *p)
 	if (child_out == INVALID_HANDLE_VALUE || child_out == NULL)
 		return ERROR_INVALID_PARAMETER;
 
-	GetWindowThreadProcessId(hwnd, &dwProcessId);
+	GetWindowThreadProcessId(hwnd, &dwProcessId); // CodeQL [SM02313]: false positive dwProcessId will not be uninitialized
 
 	if (childProcessId != dwProcessId)
 		return ERROR_SUCCESS;
@@ -1114,7 +1114,7 @@ QueueEvent(DWORD event, HWND hwnd, LONG idObject, LONG idChild)
 	consoleEvent* current = NULL;
 
 	EnterCriticalSection(&criticalSection);
-	current = malloc(sizeof(consoleEvent));
+	current = malloc(sizeof(consoleEvent)); // CodeQL [SM02320]: current struct fields initialized below
 	if (current) {
 		if (!head) {
 			current->event = event;
@@ -1128,7 +1128,8 @@ QueueEvent(DWORD event, HWND hwnd, LONG idObject, LONG idChild)
 
 			head = current;
 			tail = current;
-		} else {
+		}
+		else {
 			current->event = event;
 			current->hwnd = hwnd;
 			current->idChild = idChild;