From: Andrew Deason <adeason@sinenomine.net>
Date: Wed, 4 May 2011 17:34:20 +0000 (-0500)
Subject: libafs: Avoid using changing unixuser ticket data
X-Git-Tag: upstream/1.6.1.pre1^2~4
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=8ca56700a8c3835bc735f34e8e82476e7a4c0792;p=packages%2Fo%2Fopenafs.git

libafs: Avoid using changing unixuser ticket data

PSetTokens was afs_osi_Alloc'ing after afs_osi_Free'ing the previous
token data. This can sleep, causing tu->stp to be pointing to garbage
while we wait to alloc. Additionally, rxkad_NewClientSecurityObject
can sleep while waiting to alloc memory, and so the given tu->stp
pointer given to it by afs_ConnBySA may be invalid by the time it
actually uses the data.

To fix this, we could implement unixuser locking to ensure mutual
exclusion of these events. However, this implements a more
conservative change for the 1.4 and 1.6 branches. In PSetTokens we
alloc the new memory before we change anything, and in afs_ConnBySA we
make copies of the ticket data before giving it to rxkad. With these
changes, the glock gives us enough serialization to avoid issues with
tu->stp changing underneath us.

This change is specific to 1.4 and 1.6. On the master branch, this
issue is fixed by implementing unixuser locks in change
Idd66d72f716b7e7dc08faa31ae43e9a23639bae3.

Reviewed-on: http://gerrit.openafs.org/4649
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
(cherry picked from commit 1465946bb6863430bf0efebd024d394549a8775f)

Change-Id: Icab5176bf685c408447f0f32ad65c5b003299d3d
Reviewed-on: http://gerrit.openafs.org/6345
Reviewed-by: Derrick Brashear <shadow@dementix.org>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
---

diff --git a/src/afs/afs_conn.c b/src/afs/afs_conn.c
index ffcd132da..3de6c18dd 100644
--- a/src/afs/afs_conn.c
+++ b/src/afs/afs_conn.c
@@ -72,13 +72,27 @@ afs_pickSecurityObject(struct afs_conn *conn, int *secLevel)
 
     /* Do we have tokens ? */
     if (conn->user->vid != UNDEFVID) {
+	char *ticket;
+	struct ClearToken ct;
+
 	*secLevel = 2;
+
+	/* Make a copy of the ticket data to give to rxkad, because the
+	 * the ticket data could change while rxkad is sleeping for memory
+	 * allocation. We should implement locking on unixuser
+	 * structures to fix this properly, but for now, this is easier. */
+	ticket = afs_osi_Alloc(MAXKTCTICKETLEN);
+	memcpy(ticket, conn->user->stp, conn->user->stLen);
+	memcpy(&ct, &conn->user->ct, sizeof(ct));
+
 	/* kerberos tickets on channel 2 */
 	secObj = rxkad_NewClientSecurityObject(
 		    cryptall ? rxkad_crypt : rxkad_clear,
-                    (struct ktc_encryptionKey *)conn->user->ct.HandShakeKey,
-		    conn->user->ct.AuthHandle,
-		    conn->user->stLen, conn->user->stp);
+                    (struct ktc_encryptionKey *)ct.HandShakeKey,
+		    ct.AuthHandle,
+		    conn->user->stLen, ticket);
+
+	afs_osi_Free(ticket, MAXKTCTICKETLEN);
      }
      if (secObj == NULL) {
 	*secLevel = 0;
diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c
index bc6f5e24f..c3ff965d4 100644
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -1790,9 +1790,9 @@ DECL_PIOCTL(PSetTokens)
     struct unixuser *tu;
     struct ClearToken clear;
     struct cell *tcell;
-    char *stp;
+    char *stp, *stpNew;
     char *cellName;
-    int stLen;
+    int stLen, stLenOld;
     struct vrequest treq;
     afs_int32 flag, set_parent_pag = 0;
 
@@ -1866,18 +1866,21 @@ DECL_PIOCTL(PSetTokens)
 	    areq = &treq;
 	}
     }
+
+    stpNew = (char *)afs_osi_Alloc(stLen);
+    if (stpNew == NULL) {
+	return ENOMEM;
+    }
+    memcpy(stpNew, stp, stLen);
+
     /* now we just set the tokens */
     tu = afs_GetUser(areq->uid, i, WRITE_LOCK);	/* i has the cell # */
+    stp = tu->stp;
+    stLenOld = tu->stLen;
+
     tu->vid = clear.ViceId;
-    if (tu->stp != NULL) {
-	afs_osi_Free(tu->stp, tu->stLen);
-    }
-    tu->stp = (char *)afs_osi_Alloc(stLen);
-    if (tu->stp == NULL) {
-	return ENOMEM;
-    }
+    tu->stp = stpNew;
     tu->stLen = stLen;
-    memcpy(tu->stp, stp, stLen);
     tu->ct = clear;
 #ifndef AFS_NOSTATS
     afs_stats_cmfullperf.authent.TicketUpdates++;
@@ -1891,6 +1894,10 @@ DECL_PIOCTL(PSetTokens)
     afs_NotifyUser(tu, UTokensObtained);
     afs_PutUser(tu, WRITE_LOCK);
 
+    if (stp) {
+	afs_osi_Free(stp, stLenOld);
+    }
+
     return 0;
 
   nocell: