From 70b76b3a1cff1dabe9b10b8222cd84fc207b6704 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Thu, 19 Jan 2006 23:07:50 +0000 Subject: [PATCH] windows-integrated-logon-hack-fix-for-proper-refcounts-20060119 The Integrated Logon hack of setting a token for a smb name different than the one associated with the current smb session fails when smb virtual circuits, sessions and username objects are properly reference counted. When refcounts are not leaked the constructed smb_username_t is destroyed immediately after the token is set since there are not references to it from a current session. The fix is to mark the smb_username_t object with a flag indicating that it was created by the Network Provider. This flag prevents the destruction when the refcount is zero so that it will be available at the time the smb session is created (just a moment or two later.) During the binding of the smb_username_t to the smb_vc_t the flag is cleared allowing the tokens to be destroyed when the smb session is closed. --- src/WINNT/afsd/afslogon.c | 7 ++-- src/WINNT/afsd/cm_ioctl.c | 3 +- src/WINNT/afsd/cm_user.c | 37 ++++++++++------- src/WINNT/afsd/cm_user.h | 3 +- src/WINNT/afsd/smb.c | 87 +++++++++++++++++++++++++++++++-------- src/WINNT/afsd/smb.h | 8 ++-- src/WINNT/afsd/smb3.c | 24 +++++++---- src/WINNT/afsd/smb3.h | 2 +- 8 files changed, 120 insertions(+), 51 deletions(-) diff --git a/src/WINNT/afsd/afslogon.c b/src/WINNT/afsd/afslogon.c index dbe35ef42..bcd700f59 100644 --- a/src/WINNT/afsd/afslogon.c +++ b/src/WINNT/afsd/afslogon.c @@ -801,7 +801,8 @@ DWORD APIENTRY NPLogonNotify( { if ( KFW_is_available() ) { code = KFW_AFS_get_cred(uname, cell, password, 0, opt.smbName, &reason); - DebugEvent("KFW_AFS_get_cred uname=[%s] smbname=[%s] cell=[%s] code=[%d]",uname,opt.smbName,cell,code); + DebugEvent("KFW_AFS_get_cred uname=[%s] smbname=[%s] cell=[%s] code=[%d]", + uname,opt.smbName,cell,code); if (code == 0 && opt.theseCells) { char * principal, *p; @@ -830,8 +831,8 @@ DWORD APIENTRY NPLogonNotify( code = ka_UserAuthenticateGeneral2(KA_USERAUTH_VERSION+KA_USERAUTH_AUTHENT_LOGON, uname, "", cell, password, opt.smbName, 0, &pw_exp, 0, &reason); - DebugEvent("AFS AfsLogon - (INTEGRATED only)ka_UserAuthenticateGeneral2","Code[%x] uname[%s] Cell[%s]", - code,uname,cell); + DebugEvent("AFS AfsLogon - (INTEGRATED only)ka_UserAuthenticateGeneral2 Code[%x] uname[%s] smbname=[%s] Cell[%s] PwExp=[%d] Reason=[%s]", + code,uname,opt.smbName,cell,pw_exp,reason?reason:""); } if ( code && code != KTC_NOCM && code != KTC_NOCMRPC && !lowercased_name ) { for ( ctemp = uname; *ctemp ; ctemp++) { diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index f62b171a8..daf580271 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -1985,7 +1985,8 @@ long cm_IoctlSetToken(struct smb_ioctl *ioctlp, struct cm_user *userp) } if (flags & PIOCTL_LOGON) { - userp = smb_FindCMUserByName(smbname, ioctlp->fidp->vcp->rname); + userp = smb_FindCMUserByName(smbname, ioctlp->fidp->vcp->rname, + SMB_FLAG_CREATE|SMB_FLAG_AFSLOGON); release_userp = 1; } diff --git a/src/WINNT/afsd/cm_user.c b/src/WINNT/afsd/cm_user.c index 6cf8e969a..54d3c25bd 100644 --- a/src/WINNT/afsd/cm_user.c +++ b/src/WINNT/afsd/cm_user.c @@ -39,14 +39,13 @@ void cm_InitUser(void) cm_user_t *cm_NewUser(void) { - cm_user_t *up; + cm_user_t *userp; - up = malloc(sizeof(*up)); - memset(up, 0, sizeof(*up)); - up->refCount = 1; - up->vcRefs = 1; /* from caller */ - lock_InitializeMutex(&up->mx, "cm_user_t"); - return up; + userp = malloc(sizeof(*userp)); + memset(userp, 0, sizeof(*userp)); + userp->refCount = 1; + lock_InitializeMutex(&userp->mx, "cm_user_t"); + return userp; } /* must be called with locked userp */ @@ -98,31 +97,39 @@ void cm_HoldUser(cm_user_t *up) lock_ReleaseWrite(&cm_userLock); } -void cm_ReleaseUser(cm_user_t *up) +void cm_ReleaseUser(cm_user_t *userp) { cm_ucell_t *ucp; cm_ucell_t *ncp; - if (up == NULL) + if (userp == NULL) return; lock_ObtainWrite(&cm_userLock); - osi_assert(up->refCount-- > 0); - if (up->refCount == 0) { - lock_FinalizeMutex(&up->mx); - for (ucp = up->cellInfop; ucp; ucp = ncp) { + osi_assert(userp->refCount-- > 0); + if (userp->refCount == 0) { + lock_FinalizeMutex(&userp->mx); + for (ucp = userp->cellInfop; ucp; ucp = ncp) { ncp = ucp->nextp; if (ucp->ticketp) free(ucp->ticketp); free(ucp); } - free(up); + free(userp); } lock_ReleaseWrite(&cm_userLock); } + +void cm_HoldUserVCRef(cm_user_t *userp) +{ + lock_ObtainMutex(&userp->mx); + userp->vcRefs++; + lock_ReleaseMutex(&userp->mx); +} + /* release the count of the # of connections that use this user structure. - * When this hits zero, we know we won't be getting an new requests from + * When this hits zero, we know we won't be getting any new requests from * this user, and thus we can start GC'ing connections. Ref count on user * won't hit zero until all cm_conn_t's have been GC'd, since they hold * refCount references to userp. diff --git a/src/WINNT/afsd/cm_user.h b/src/WINNT/afsd/cm_user.h index 48ea5ba84..f9f28b5b2 100644 --- a/src/WINNT/afsd/cm_user.h +++ b/src/WINNT/afsd/cm_user.h @@ -48,7 +48,6 @@ typedef struct cm_user { } cm_user_t; #define CM_USERFLAG_DELETE 1 /* delete on last reference */ -#define CM_USERFLAG_WASLOGON 2 /* was logon DLL user */ extern void cm_InitUser(void); @@ -60,6 +59,8 @@ extern cm_ucell_t *cm_FindUCell(cm_user_t *userp, int iterator); extern void cm_HoldUser(cm_user_t *up); +extern void cm_HoldUserVCRef(cm_user_t *up); + extern void cm_ReleaseUser(cm_user_t *up); extern void cm_ReleaseUserVCRef(cm_user_t *up); diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index a5cfe6b5e..c13a6e5d3 100644 --- a/src/WINNT/afsd/smb.c +++ b/src/WINNT/afsd/smb.c @@ -935,6 +935,8 @@ int smb_IsStarMask(char *maskp) void smb_ReleaseVCInternal(smb_vc_t *vcp) { + smb_vc_t **vcpp; + #ifdef DEBUG osi_assert(vcp->refCount-- != 0); #else @@ -942,6 +944,14 @@ void smb_ReleaseVCInternal(smb_vc_t *vcp) #endif if (vcp->refCount == 0) { + /* remove VCP from smb_deadVCsp */ + for (vcpp = &smb_deadVCsp; *vcpp; vcpp = &((*vcpp)->nextp)) { + if (*vcpp == vcp) { + *vcpp = vcp->nextp; + break; + } + } + memset(vcp,0,sizeof(smb_vc_t)); free(vcp); } @@ -985,9 +995,9 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) smb_tid_t *tidpNext; smb_tid_t *tidp; unsigned short tid; - smb_user_t *userpIter; - smb_user_t *userpNext; - smb_user_t *userp; + smb_user_t *uidpIter; + smb_user_t *uidpNext; + smb_user_t *uidp; unsigned short uid; smb_vc_t **vcpp; @@ -1034,24 +1044,24 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) lock_ObtainRead(&smb_rctLock); } - for (userpIter = vcp->usersp; userpIter; userpIter = userpNext) { - userpNext = userpIter->nextp; + for (uidpIter = vcp->usersp; uidpIter; uidpIter = uidpNext) { + uidpNext = uidpIter->nextp; - if (userpIter->flags & SMB_USERFLAG_DELETE) + if (uidpIter->flags & SMB_USERFLAG_DELETE) continue; - uid = userpIter->userID; - osi_Log2(smb_logp, " Cleanup UID %d (userp=0x%x)", uid, userpIter); + uid = uidpIter->userID; + osi_Log2(smb_logp, " Cleanup UID %d (uidp=0x%x)", uid, uidpIter); lock_ReleaseRead(&smb_rctLock); - userp = smb_FindUID(vcp, uid, 0); - osi_assert(userp); + uidp = smb_FindUID(vcp, uid, 0); + osi_assert(uidp); - lock_ObtainMutex(&userp->mx); - userp->flags |= SMB_USERFLAG_DELETE; - lock_ReleaseMutex(&userp->mx); + lock_ObtainMutex(&uidp->mx); + uidp->flags |= SMB_USERFLAG_DELETE; + lock_ReleaseMutex(&uidp->mx); - smb_ReleaseUID(userp); + smb_ReleaseUID(uidp); lock_ObtainRead(&smb_rctLock); } @@ -1060,6 +1070,12 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) for (vcpp = &smb_allVCsp; *vcpp; vcpp = &((*vcpp)->nextp)) { if (*vcpp == vcp) { *vcpp = vcp->nextp; + vcp->nextp = smb_deadVCsp; + smb_deadVCsp = vcp; + /* We intentionally do not keep a reference to the + * vcp once it is placed on the deadVCsp list. This + * allows the refcount to reach 0 so we can delete + * it. */ smb_ReleaseVCNoLock(vcp); break; } @@ -1154,7 +1170,7 @@ smb_user_t *smb_FindUID(smb_vc_t *vcp, unsigned short uid, int flags) return uidp; } -smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags) +smb_username_t *smb_FindUserByName(char *usern, char *machine, afs_uint32 flags) { smb_username_t *unp= NULL; @@ -1175,7 +1191,10 @@ smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags) unp->machine = strdup(machine); usernamesp = unp; lock_InitializeMutex(&unp->mx, "username_t mutex"); + if (flags & SMB_FLAG_AFSLOGON) + unp->flags = SMB_USERFLAG_AFSLOGON; } + lock_ReleaseWrite(&smb_rctLock); return unp; } @@ -1208,7 +1227,7 @@ void smb_ReleaseUsername(smb_username_t *unp) lock_ObtainWrite(&smb_rctLock); osi_assert(unp->refCount-- > 0); - if (unp->refCount == 0) { + if (unp->refCount == 0 && !(unp->flags & SMB_USERFLAG_AFSLOGON)) { lupp = &usernamesp; for(up = *lupp; up; lupp = &up->nextp, up = *lupp) { if (up == unp) @@ -1225,7 +1244,6 @@ void smb_ReleaseUsername(smb_username_t *unp) lock_ReleaseWrite(&smb_rctLock); if (userp) { - cm_ReleaseUserVCRef(userp); cm_ReleaseUser(userp); } } @@ -1253,8 +1271,11 @@ void smb_ReleaseUID(smb_user_t *uidp) } lock_ReleaseWrite(&smb_rctLock); - if (unp) + if (unp) { + if (unp->userp) + cm_ReleaseUserVCRef(unp->userp); smb_ReleaseUsername(unp); + } } /* retrieve a held reference to a user structure corresponding to an incoming @@ -8754,6 +8775,36 @@ int smb_DumpVCP(FILE *outputFile, char *cookie, int lock) sprintf(output, "done dumping smb_vc_t\n"); WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + sprintf(output, "begin dumping DEAD smb_vc_t\n"); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + + for (vcp = smb_deadVCsp; vcp; vcp=vcp->nextp) + { + smb_fid_t *fidp; + + sprintf(output, "%s vcp=0x%p, refCount=%d, flags=%d, vcID=%d, lsn=%d, uidCounter=%d, tidCounter=%d, fidCounter=%d\n", + cookie, vcp, vcp->refCount, vcp->flags, vcp->vcID, vcp->lsn, vcp->uidCounter, vcp->tidCounter, vcp->fidCounter); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + + sprintf(output, "begin dumping smb_fid_t\n"); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + + for (fidp = vcp->fidsp; fidp; fidp = (smb_fid_t *) osi_QNext(&fidp->q)) + { + sprintf(output, "%s -- smb_fidp=0x%p, refCount=%d, fid=%d, vcp=0x%p, scp=0x%p, ioctlp=0x%p, NTopen_pathp=%s, NTopen_wholepathp=%s\n", + cookie, fidp, fidp->refCount, fidp->fid, fidp->vcp, fidp->scp, fidp->ioctlp, + fidp->NTopen_pathp ? fidp->NTopen_pathp : "NULL", + fidp->NTopen_wholepathp ? fidp->NTopen_wholepathp : "NULL"); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + } + + sprintf(output, "done dumping smb_fid_t\n"); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + } + + sprintf(output, "done dumping DEAD smb_vc_t\n"); + WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL); + if (lock) lock_ReleaseRead(&smb_rctLock); return 0; diff --git a/src/WINNT/afsd/smb.h b/src/WINNT/afsd/smb.h index fbfdda86b..e2002a0b5 100644 --- a/src/WINNT/afsd/smb.h +++ b/src/WINNT/afsd/smb.h @@ -91,6 +91,7 @@ typedef struct smb { /* flags for functions */ #define SMB_FLAG_CREATE 1 /* create the structure if necessary */ +#define SMB_FLAG_AFSLOGON 2 /* operating on behalf of afslogon.dll */ /* max # of bytes we'll receive in an incoming SMB message */ /* the maximum is 2^18-1 for NBT and 2^25-1 for Raw transport messages */ @@ -246,6 +247,7 @@ typedef struct smb_username { } smb_username_t; #define SMB_USERFLAG_DELETE 1 /* delete struct when ref count zero */ +#define SMB_USERFLAG_AFSLOGON 2 /* do not delete when the refCount reaches zero */ #define SMB_MAX_USERNAME_LENGTH 256 @@ -499,14 +501,10 @@ extern void smb_ReleaseTID(smb_tid_t *tidp); extern smb_user_t *smb_FindUID(smb_vc_t *vcp, unsigned short uid, int flags); -extern smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags); +extern smb_username_t *smb_FindUserByName(char *usern, char *machine, afs_uint32 flags); extern smb_user_t *smb_FindUserByNameThisSession(smb_vc_t *vcp, char *usern); -extern smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags); - -extern smb_user_t *smb_FindUserByNameThisSession(smb_vc_t *vcp, char *usern); - extern void smb_ReleaseUsername(smb_username_t *unp); extern void smb_ReleaseUID(smb_user_t *uidp); diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c index 003c3ef8e..784928524 100644 --- a/src/WINNT/afsd/smb3.c +++ b/src/WINNT/afsd/smb3.c @@ -662,7 +662,6 @@ long smb_ReceiveV3SessionSetupX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t * smb_user_t *uidp; unsigned short newUid; unsigned long caps = 0; - cm_user_t *userp; smb_username_t *unp; char *s1 = " "; long code = 0; @@ -842,20 +841,31 @@ long smb_ReceiveV3SessionSetupX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t * uidp = smb_FindUserByNameThisSession(vcp, usern); if (uidp) { /* already there, so don't create a new one */ unp = uidp->unp; - userp = unp->userp; newUid = uidp->userID; - osi_Log3(smb_logp,"smb_ReceiveV3SessionSetupX FindUserByName:Lana[%d],lsn[%d],userid[%d]",vcp->lana,vcp->lsn,newUid); + osi_Log3(smb_logp,"smb_ReceiveV3SessionSetupX FindUserByName:Lana[%d],lsn[%d],userid[%d]", + vcp->lana,vcp->lsn,newUid); smb_ReleaseUID(uidp); } else { - /* do a global search for the username/machine name pair */ + cm_user_t *userp; + + /* do a global search for the username/machine name pair */ unp = smb_FindUserByName(usern, vcp->rname, SMB_FLAG_CREATE); + lock_ObtainMutex(&unp->mx); + if (unp->flags & SMB_USERFLAG_AFSLOGON) { + /* clear the afslogon flag so that the tickets can now + * be freed when the refCount returns to zero. + */ + unp->flags &= ~SMB_USERFLAG_AFSLOGON; + } + lock_ReleaseMutex(&unp->mx); /* Create a new UID and cm_user_t structure */ userp = unp->userp; if (!userp) userp = cm_NewUser(); - lock_ObtainMutex(&vcp->mx); + cm_HoldUserVCRef(userp); + lock_ObtainMutex(&vcp->mx); if (!vcp->uidCounter) vcp->uidCounter++; /* handle unlikely wraparounds */ newUid = (strlen(usern)==0)?0:vcp->uidCounter++; @@ -7149,12 +7159,12 @@ void smb3_Init() lock_InitializeMutex(&smb_Dir_Watch_Lock, "Directory Watch List Lock"); } -cm_user_t *smb_FindCMUserByName(char *usern, char *machine) +cm_user_t *smb_FindCMUserByName(char *usern, char *machine, afs_uint32 flags) { smb_username_t *unp; cm_user_t * userp; - unp = smb_FindUserByName(usern, machine, SMB_FLAG_CREATE); + unp = smb_FindUserByName(usern, machine, flags); if (!unp->userp) { lock_ObtainMutex(&unp->mx); unp->userp = cm_NewUser(); diff --git a/src/WINNT/afsd/smb3.h b/src/WINNT/afsd/smb3.h index c9b5ac443..62a9c9269 100644 --- a/src/WINNT/afsd/smb3.h +++ b/src/WINNT/afsd/smb3.h @@ -192,7 +192,7 @@ extern long smb_ReceiveNTRename(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t * extern int smb_V3MatchMask(char *namep, char *maskp, int flags); extern void smb3_Init(); -extern cm_user_t *smb_FindCMUserByName(/*smb_vc_t *vcp,*/ char *usern, char *machine); +extern cm_user_t *smb_FindCMUserByName(char *usern, char *machine, afs_uint32 flags); /* SMB auth related functions */ extern void smb_NegotiateExtendedSecurity(void ** secBlob, int * secBlobLength); -- 2.39.5