Re: [9front] [PATCH] Permissions for child boards in /srv

[Jacob Moody <moody@mail.posixcafe.org>]

Found a bug here(and in the committed version), we need
to decref on any remove call regardless of if it succeeds.

I'd like to get this committed soon to fix this bug.


Thanks,
moody

diff 9e363c506eccb783dd4d1d31f14c9d8dba98f65e uncommitted
--- a/sys/src/9/port/devsrv.c
+++ b/sys/src/9/port/devsrv.c
@@ -13,6 +13,8 @@
 	void 	*link;
 	char 	*name;
 	ulong 	path;
+	char	*owner;
+	ulong	perm;
 };

 typedef struct Srv Srv;
@@ -19,9 +21,6 @@
 struct Srv
 {
 	Link;
-
-	char	*owner;
-	ulong	perm;
 	Chan	*chan;
 };

@@ -140,6 +139,7 @@

 		b = ch->parent;
 		free(ch->name);
+		free(ch->owner);
 		wunlock(ch);
 		free(ch);
 	}
@@ -162,14 +162,17 @@
 	rlock(b);
 	if(waserror()){
 		runlock(b);
-		return -1;
+		nexterror();
 	}
-	if(s == DEVDOTDOT){
+	switch(s){
+	case -2: /* dot */
+		ch = b;
+		goto Child;
+	case DEVDOTDOT:
 		ch = b->parent;
 		if(ch == nil)
 			ch = &root;
 		goto Child;
-		
 	}
 	if(name != nil){
 		if(strcmp("clone", name) == 0)
@@ -194,21 +197,25 @@
 		devdir(c, q, up->genbuf, 0, sp->owner, sp->perm, dp);
 	} else if(ch != nil){
 Child:
+		if(name != nil || s == DEVDOTDOT){
+			devpermcheck(ch->owner, ch->perm, OEXEC);
+			c->aux = ch;
+		}
 		kstrcpy(up->genbuf, ch->name, sizeof up->genbuf);
 		q.vers = ch->id;
 		q.path = NETQID(q.vers, ch->path);
 		q.type = QTDIR;
-		devdir(c, q, up->genbuf, 0, eve, 0555|DMDIR, dp);
-		/* dirread's and stats shouldn't alter c->aux */
-		if(name != nil)
-			c->aux = ch;
+		devdir(c, q, up->genbuf, 0, ch->owner, ch->perm|DMDIR, dp);
 	} else if(0){
 Clone:
 		q.vers = NETID(c->qid.path);
 		q.path = NETQID(q.vers, Qclone);
 		devdir(c, q, "clone", 0, eve, 0444, dp);
-	} else
-		error(Enonexist);
+	} else {
+		runlock(b);
+		poperror();
+		return -1;
+	}

 	runlock(b);
 	poperror();
@@ -220,6 +227,8 @@
 {
 	root.qidpath = Qend;
 	root.name = "#s";
+	root.perm = 0777;
+	kstrdup(&root.owner, eve);
 }

 static Chan*
@@ -253,6 +262,17 @@
 static int
 srvstat(Chan *c, uchar *db, int n)
 {
+	Dir d;
+
+	/* devstat cheats for dir stats, we care about our dir perms */
+	if(c->qid.type == QTDIR){
+		srvgen(c, nil, nil, 0, -2, &d);
+		n = convD2M(&d, db, n);
+		if(n == 0)
+			error(Ebadarg);
+		return n;
+	}
+
 	return devstat(c, db, n, 0, 0, srvgen);
 }

@@ -286,16 +306,6 @@
 	Chan *nc;
 	char buf[64];

-	if(c->qid.type == QTDIR){
-		if(omode & ORCLOSE)
-			error(Eperm);
-		if(omode != OREAD)
-			error(Eisdir);
-		c->mode = omode;
-		c->flag |= COPEN;
-		c->offset = 0;
-		return c;
-	}
 	if(omode&OTRUNC)
 		error(Eexist);
 	if(omode&ORCLOSE)
@@ -311,6 +321,8 @@
 		ch = smalloc(sizeof *ch);
 		ch->qidpath = Qend;
 		ch->ref = 1;
+		ch->perm = 0770;
+		kstrdup(&ch->owner, up->user);
 		do {
 			qlock(&boards);
 			ch->id = ++boards.path;
@@ -336,6 +348,19 @@
 		runlock(b);
 		nexterror();
 	}
+	if(c->qid.type == QTDIR){
+		if(omode & ORCLOSE)
+			error(Eperm);
+		if(omode != OREAD)
+			error(Eisdir);
+		devpermcheck(b->owner, b->perm, omode);
+		c->mode = omode;
+		c->flag |= COPEN;
+		c->offset = 0;
+		runlock(b);
+		poperror();
+		return c;
+	}
 	if(b->closed)
 		error(Eexpired);

@@ -369,6 +394,9 @@
 	if(strlen(name) >= sizeof(up->genbuf))
 		error(Etoolong);

+	if(strcmp("clone", name) == 0)
+		error("reserved name");
+
 	sp = smalloc(sizeof *sp);
 	kstrdup(&sp->name, name);
 	kstrdup(&sp->owner, up->user);
@@ -384,6 +412,7 @@
 	}
 	if(b->closed)
 		error(Eexpired);
+	devpermcheck(b->owner, b->perm, OWRITE);
 	if(lookup(b->srv, name, ~0UL) != nil)
 		error(Eexist);
 	if(lookup(b->child, name, ~0UL) != nil)
@@ -414,6 +443,12 @@
 	Board *b;
 	Srv *sp;

+	b = c->aux;
+	wlock(b);
+	if(waserror()){
+		boardclunk(b, 0); //unlock
+		nexterror();
+	}
 	if(c->qid.type == QTDIR)
 		error(Eperm);
 	switch(NETTYPE(c->qid.path)){
@@ -422,28 +457,13 @@
 		error(Eperm);
 	}

-	b = c->aux;
-	wlock(b);
-	if(waserror()){
-		wunlock(b);
-		nexterror();
-	}
 	sp = lookup(b->srv, nil, c->qid.path);
 	if(sp == nil)
 		error(Enonexist);

-	/*
-	 * Only eve can remove system services.
-	 */
-	if(strcmp(sp->owner, eve) == 0 && !iseve())
+	if(strcmp(sp->owner, up->user) != 0 && !iseve())
 		error(Eperm);

-	/*
-	 * No removing personal services.
-	 */
-	if((sp->perm&7) != 7 && strcmp(sp->owner, up->user) && !iseve())
-		error(Eperm);
-
 	remove((Link**)&b->srv, nil, c->qid.path);

 	boardclunk(b, 0); //unlock
@@ -459,18 +479,18 @@
 static int
 srvwstat(Chan *c, uchar *dp, int n)
 {
-	Board *b;
+	Board *b, *s;
 	char *strs;
-	Srv *sp;
 	Dir d;
+	Link *lp;

-	if(c->qid.type & QTDIR)
-		error(Eperm);
 	switch(NETTYPE(c->qid.path)){
 	case Qlease:
 	case Qclone:
 		error(Eperm);
 	}
+	if(c->qid.type == QTDIR && c->aux == &root)
+		error(Eperm);

 	strs = smalloc(n);
 	if(waserror()){
@@ -490,29 +510,50 @@
 	if(b->closed)
 		error(Eexpired);

-	sp = lookup(b->srv, nil, c->qid.path);
-	if(sp == nil)
+	if(c->qid.type == QTDIR){
+		lp = b;
+		/* we share ownership of our stats with our parent */
+		assert(b->parent != nil);
+		wlock(b->parent);
+		if(waserror()){
+			wunlock(b->parent);
+			nexterror();
+		}
+	} else
+		lp = lookup(b->srv, nil, c->qid.path);
+	if(lp == nil)
 		error(Enonexist);

-	if(strcmp(sp->owner, up->user) != 0 && !iseve())
+	if(strcmp(lp->owner, up->user) != 0 && !iseve())
 		error(Eperm);

-	if(d.name != nil && *d.name && strcmp(sp->name, d.name) != 0) {
+	if(d.name != nil && *d.name && strcmp(lp->name, d.name) != 0) {
 		if(strchr(d.name, '/') != nil)
 			error(Ebadchar);
 		if(strlen(d.name) >= sizeof(up->genbuf))
 			error(Etoolong);
-		if(lookup(b->srv, d.name, ~0UL) != nil)
+
+		//Ensure new name doesn't conflict with old names
+		if(c->qid.type == QTDIR)
+			s = b->parent;
+		else
+			s = b;
+		if(lookup(s->srv, d.name, ~0UL) != nil)
 			error(Eexist);
-		if(lookup(b->child, d.name, ~0UL) != nil)
+		if(lookup(s->child, d.name, ~0UL) != nil)
 			error(Eexist);
-		kstrdup(&sp->name, d.name);
+		kstrdup(&lp->name, d.name);
 	}
 	if(d.uid != nil && *d.uid)
-		kstrdup(&sp->owner, d.uid);
+		kstrdup(&lp->owner, d.uid);
 	if(d.mode != ~0UL)
-		sp->perm = d.mode & 0777;
+		lp->perm = d.mode & 0777;

+	if(c->qid.type == QTDIR){
+		wunlock(b->parent);
+		poperror();
+	}
+
 	wunlock(b);
 	poperror();

@@ -538,12 +579,12 @@
 	 	 * which is immutable, all is well.
 	 	 */
 		if(waserror())
-			goto Clunk;
+			return;
 		srvremove(c);
 		poperror();
 		return;
 	}
-Clunk:
+
 	b = c->aux;
 	wlock(b);
 	boardclunk(b, expired); //unlock
@@ -644,6 +685,7 @@

 	b = &root;
 	wlock(b);
+	kstrdup(&b->owner, new);
 	for(sp = b->srv; sp != nil; sp = sp->link) {
 		if(sp->owner != nil && strcmp(old, sp->owner) == 0)
 			kstrdup(&sp->owner, new);