* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
@ 2023-01-06 14:21 ` cinap_lenrek
2023-01-06 14:44 ` cinap_lenrek
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 14:21 UTC (permalink / raw)
To: 9front
it looks ok, but it is still wrong no?
The create request asks the file server to create a new file
with the name supplied, in the directory (dir) represented
by fid, and requires write permission in the directory. The
owner of the file is the implied user id of the request, the
group of the file is the same as dir, and the permissions
are the value of
perm & (~0666 | (dir.perm & 0666))
if a regular file is being created and
perm & (~0777 | (dir.perm & 0777))
if a directory is being created. This means, for example,
that if the create allows read permission to others, but the
containing directory does not, then the created file will
not allow others to read the file.
we'r changing the uid to the directory owner, but it should the the uid
of the drawterm process.
--
cinap
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
2023-01-06 14:21 ` cinap_lenrek
@ 2023-01-06 14:44 ` cinap_lenrek
2023-01-06 15:30 ` Xiao-Yong Jin
2023-01-06 15:40 ` cinap_lenrek
2023-01-06 18:20 ` cinap_lenrek
3 siblings, 1 reply; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 14:44 UTC (permalink / raw)
To: 9front
how about this?
--- a/./kern/devfs-posix.c
+++ b/./kern/devfs-posix.c
@@ -244,8 +244,8 @@
fd = open(path, 0);
if(fd >= 0) {
- chmod(path, perm & 0777);
- chown(path, uif->uid, uif->uid);
+ chmod(path, uif->mode & perm & 0777);
+ chown(path, -1, uif->gid);
}
close(fd);
@@ -260,8 +260,8 @@
close(fd);
fd = open(path, m);
}
- chmod(path, perm & 0777);
- chown(path, uif->uid, uif->gid);
+ chmod(path, uif->mode & perm & 0666);
+ chown(path, -1, uif->gid);
}
if(fd < 0)
error(strerror(errno));
--
cinap
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
` (2 preceding siblings ...)
2023-01-06 15:40 ` cinap_lenrek
@ 2023-01-06 18:20 ` cinap_lenrek
3 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 18:20 UTC (permalink / raw)
To: 9front
ok, i found some other issues with the code and
committed the following:
94f8d9a60fabb07dd7e37c806ca39eadbd883339 devfs: create handle OEXCL, mask permissions with parent directory, fix leaks
Hash: 94f8d9a60fabb07dd7e37c806ca39eadbd883339
Date: Fri Jan 6 19:17:58 CET 2023
Author: cinap_lenrek <cinap_lenrek@felloff.net>
Message: devfs: create handle OEXCL, mask permissions with parent directory, fix leaks
OEXCL has not been handled correctly. We have to
return an error if the file already exists.
The file-permissions should only be applied when
we create a new file.
When creating a new file, we mask te permissions
with the permissions of the parent directory
(see open(5)).
There where a few potential leaks. After creating
file file, we do a stat to get the qid information,
but this can potentially fail, eigther leaving the
Chan in the wrong state or leaking memory. Fix it.
diff -u .git/fs/object/eeeb7632dd77d90d1439bb31ca198c5061950301/tree/kern/devfs-posix.c .git/fs/object/94f8d9a60fabb07dd7e37c806ca39eadbd883339/tree/kern/devfs-posix.c
--- .git/fs/object/eeeb7632dd77d90d1439bb31ca198c5061950301/tree/kern/devfs-posix.c
+++ .git/fs/object/94f8d9a60fabb07dd7e37c806ca39eadbd883339/tree/kern/devfs-posix.c
@@ -170,51 +170,29 @@
static Chan*
fsopen(Chan *c, int mode)
{
- int m, isdir;
Ufsinfo *uif;
+ int omode;
-/*print("fsopen %s\n", chanpath(c));*/
- m = mode & (OTRUNC|3);
- switch(m) {
- case 0:
- break;
- case 1:
- case 1|16:
- break;
- case 2:
- case 0|16:
- case 2|16:
- break;
- case 3:
- break;
- default:
- error(Ebadarg);
- }
+ omode = openmode(mode);
- isdir = c->qid.type & QTDIR;
-
- if(isdir && mode != OREAD)
- error(Eperm);
-
- m = fsomode(m & 3);
- c->mode = openmode(mode);
-
uif = c->aux;
- if(isdir) {
- uif->dir = opendir(uif->path);
- if(uif->dir == 0)
+ if(c->qid.type & QTDIR) {
+ if(omode != OREAD)
+ error(Eperm);
+ if((uif->dir = opendir(uif->path)) == NULL)
error(strerror(errno));
}
else {
+ int m = fsomode(omode & 3);
if(mode & OTRUNC)
m |= O_TRUNC;
- uif->fd = open(uif->path, m, 0666);
- if(uif->fd < 0)
+ if((uif->fd = open(uif->path, m, 0666)) < 0)
error(strerror(errno));
}
uif->offset = 0;
c->offset = 0;
+ c->mode = omode;
c->flag |= COPEN;
return c;
}
@@ -222,12 +200,13 @@
static Chan*
fscreate(Chan *c, char *name, int mode, ulong perm)
{
- int fd, m;
- char *path;
struct stat stbuf;
Ufsinfo *uif;
+ char *path;
+ DIR *dir;
+ int fd, omode;
- m = fsomode(mode&3);
+ omode = openmode(mode & ~OEXCL);
uif = c->aux;
path = catpath(uif->path, name);
@@ -235,49 +214,48 @@
free(path);
nexterror();
}
+ fd = -1;
+ dir = NULL;
if(perm & DMDIR) {
- if(m)
+ if(omode != OREAD)
error(Eperm);
-
- if(mkdir(path, perm & 0777) < 0)
- error(strerror(errno));
-
- fd = open(path, 0);
- if(fd >= 0) {
- chmod(path, perm & 0777);
- chown(path, uif->uid, uif->uid);
+ if(mkdir(path, uif->mode & perm & 0777) < 0){
+ if(errno != EEXIST || mode & OEXCL)
+ error(strerror(errno));
}
- close(fd);
-
- uif->dir = opendir(path);
- if(uif->dir == 0)
+ if((dir = opendir(path)) == NULL)
error(strerror(errno));
+ if(waserror()){
+ closedir(dir);
+ nexterror();
+ }
}
else {
- fd = open(path, O_WRONLY|O_CREAT|O_TRUNC, 0666);
- if(fd >= 0) {
- if(m != 1) {
- close(fd);
- fd = open(path, m);
- }
- chmod(path, perm & 0777);
- chown(path, uif->uid, uif->gid);
- }
- if(fd < 0)
+ int m = fsomode(omode & 3) | O_CREAT | O_TRUNC;
+ if(mode & OEXCL)
+ m |= O_EXCL;
+ if((fd = open(path, m, uif->mode & perm & 0666)) < 0)
error(strerror(errno));
- uif->fd = fd;
+ if(waserror()){
+ close(fd);
+ nexterror();
+ }
}
if(stat(path, &stbuf) < 0)
error(strerror(errno));
+ c->qid = fsqid(&stbuf);
+ uif->fd = fd;
+ uif->dir = dir;
+ poperror();
free(uif->path);
uif->path = path;
+ uif->offset = 0;
poperror();
- c->qid = fsqid(&stbuf);
c->offset = 0;
+ c->mode = omode;
c->flag |= COPEN;
- c->mode = openmode(mode);
return c;
}
@@ -604,13 +582,13 @@
fsomode(int m)
{
switch(m) {
- case 0: /* OREAD */
- case 3: /* OEXEC */
- return 0;
- case 1: /* OWRITE */
- return 1;
- case 2: /* ORDWR */
- return 2;
+ case OREAD:
+ case OEXEC:
+ return O_RDONLY;
+ case OWRITE:
+ return O_WRONLY;
+ case ORDWR:
+ return O_RDWR;
}
error(Ebadarg);
return 0;
^ permalink raw reply [flat|nested] 6+ messages in thread