From: Jeffrey Altman <jaltman@secure-endpoints.com>
Date: Mon, 17 Aug 2009 16:33:09 +0000 (-0400)
Subject: Windows: Be more conservative about checking error conditions
X-Git-Tag: openafs-devel-1_5_62~42
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=cbaf97a953dc91e54d7148ad4294f003b8deef9c;p=packages%2Fo%2Fopenafs.git

Windows: Be more conservative about checking error conditions

It has been reported that winlogon.exe is crashing on some
systems.  The reports indicate that the failure is somewhere
in GetLogonDomainOptions.  This commit ensures that we are
more conservative about the assumptions that are made regarding
which Lsa operations can fail.

LICENSE MIT

Reviewed-on: http://gerrit.openafs.org/321
Reviewed-by: Asanka Herath <asanka@secure-endpoints.com>
Tested-by: Asanka Herath <asanka@secure-endpoints.com>
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>
Tested-by: Jeffrey Altman <jaltman@openafs.org>
---

diff --git a/src/WINNT/afsd/afslogon.c b/src/WINNT/afsd/afslogon.c
index af03d500a..43363a36a 100644
--- a/src/WINNT/afsd/afslogon.c
+++ b/src/WINNT/afsd/afslogon.c
@@ -359,8 +359,8 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
     DWORD dwSize;
     DWORD dwType;
     DWORD dwDummy;
-    char computerName[MAX_COMPUTERNAME_LENGTH + 1];
-    char *effDomain;
+    char computerName[MAX_COMPUTERNAME_LENGTH + 1]="";
+    char *effDomain = NULL;
 
     memset(opt, 0, sizeof(LogonOptions_t));
 
@@ -368,17 +368,14 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
     /* If the domain is the same as the Netbios computer name, we use the LOCALHOST domain name*/
     opt->flags = LOGON_FLAG_REMOTE;
     if(domain) {
-        dwSize = MAX_COMPUTERNAME_LENGTH;
+        dwSize = MAX_COMPUTERNAME_LENGTH + 1;
         if(GetComputerName(computerName, &dwSize)) {
             if(!cm_stricmp_utf8(computerName, domain)) {
                 effDomain = "LOCALHOST";
                 opt->flags = LOGON_FLAG_LOCAL;
             }
-            else
-                effDomain = domain;
         }
-    } else
-        effDomain = NULL;
+    }
 
     rv = RegOpenKeyEx( HKEY_LOCAL_MACHINE, AFSREG_CLT_SVC_PARAM_SUBKEY, 0, KEY_READ, &hkParm );
     if(rv != ERROR_SUCCESS) {
@@ -424,7 +421,7 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
     rv = RegQueryValueEx(hkParm, REG_CLIENT_FAIL_SILENTLY_PARM, 0, &dwType, (LPBYTE) &dwDummy, &dwSize);
     if (rv != ERROR_SUCCESS)
         LOOKUPKEYCHAIN(dwDummy, REG_DWORD, DEFAULT_FAIL_SILENTLY, REG_CLIENT_FAIL_SILENTLY_PARM);
-    opt->failSilently = !!dwDummy;
+    opt->failSilently = dwDummy ? 1 :0;
 
     /* Retry interval */
     LOOKUPKEYCHAIN(opt->retryInterval, REG_DWORD, DEFAULT_RETRY_INTERVAL, REG_CLIENT_RETRY_INTERVAL_PARM);
@@ -432,29 +429,38 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
     /* Sleep interval */
     LOOKUPKEYCHAIN(opt->sleepInterval, REG_DWORD, DEFAULT_SLEEP_INTERVAL, REG_CLIENT_SLEEP_INTERVAL_PARM);
 
-    opt->logonScript = NULL;
-    opt->smbName = NULL;
-
     if(!ISLOGONINTEGRATED(opt->LogonOption)) {
+        DebugEvent0("Integrated logon disabled");
         goto cleanup; /* no need to lookup the logon script */
     }
 
     /* come up with SMB username */
     if(ISHIGHSECURITY(opt->LogonOption)) {
+        DebugEvent0("High Security Mode active");
         opt->smbName = malloc( MAXRANDOMNAMELEN );
+        if (opt->smbName == NULL)
+            goto cleanup;
         GenRandomName(opt->smbName);
     } else if (lpLogonId) {
         /* username and domain for logon session is not necessarily the same as
            username and domain passed into network provider. */
-        PSECURITY_LOGON_SESSION_DATA plsd;
-        char lsaUsername[MAX_USERNAME_LENGTH];
-        char lsaDomain[MAX_DOMAIN_LENGTH];
+        PSECURITY_LOGON_SESSION_DATA plsd=NULL;
+        char lsaUsername[MAX_USERNAME_LENGTH]="";
+        char lsaDomain[MAX_DOMAIN_LENGTH]="";
         size_t len, tlen;
+        NTSTATUS Status;
 
-        LsaGetLogonSessionData(lpLogonId, &plsd);
+        Status = LsaGetLogonSessionData(lpLogonId, &plsd);
+        if ( FAILED(Status) || plsd == NULL ) {
+            DebugEvent("LsaGetLogonSessionData failed [0x%x]", Status);
+            goto bad_strings;
+        }
         
-        UnicodeStringToANSI(plsd->UserName, lsaUsername, MAX_USERNAME_LENGTH);
-        UnicodeStringToANSI(plsd->LogonDomain, lsaDomain, MAX_DOMAIN_LENGTH);
+        if (!UnicodeStringToANSI(plsd->UserName, lsaUsername, MAX_USERNAME_LENGTH))
+            goto bad_strings;
+
+        if (!UnicodeStringToANSI(plsd->LogonDomain, lsaDomain, MAX_DOMAIN_LENGTH))
+            goto bad_strings;
 
         DebugEvent("PLSD username[%s] domain[%s]",lsaUsername,lsaDomain);
 
@@ -471,6 +477,8 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
         len += 2;
 
         opt->smbName = malloc(len);
+        if (opt->smbName == NULL)
+            goto cleanup;
 
         StringCbCopy(opt->smbName, len, lsaDomain);
         StringCbCat(opt->smbName, len, "\\");
@@ -479,20 +487,24 @@ GetDomainLogonOptions( PLUID lpLogonId, char * username, char * domain, LogonOpt
         strlwr(opt->smbName);
 
       bad_strings:
-        LsaFreeReturnBuffer(plsd);
-    } else {
+        if (plsd)
+            LsaFreeReturnBuffer(plsd);
+    }
+    if (opt->smbName == NULL) {
         size_t len;
 
-        DebugEvent("No LUID given. Constructing username using [%s] and [%s]",
+        DebugEvent("Constructing username using [%s] and [%s]",
                    username, domain);
  
         len = strlen(username) + strlen(domain) + 2;
 
         opt->smbName = malloc(len);
+        if (opt->smbName == NULL)
+            goto cleanup;
 
-        StringCbCopy(opt->smbName, len, username);
+        StringCbCopy(opt->smbName, len, domain);
         StringCbCat(opt->smbName, len, "\\");
-        StringCbCat(opt->smbName, len, domain);
+        StringCbCat(opt->smbName, len, username);
 
         strlwr(opt->smbName);
     }