From: Mike K. <mi...@pa...> - 2011-12-12 20:01:46
|
Hi, I'd like to propose support for an extra mode for '-o idmap', "auto". When "auto" mode is set, the UID/GID of each file gets translated to a local UID/GID corresponding to the owner's name and group's name of the file on the remote server. I have a situation where I want to mount, via sshfs, a remote file system from a server which uses different username/groupname to uid/gid mappings than the local server, but which will have some overlap in user names & group names. I want to be able to mount this remote file system, and have all my stat() calls, etc show the correctly remapped UID on this local system. Using '-o idmap=none' won't work because the remote and local UID/GIDs don't always match. Using '-o idmap=user' won't work because some mounted directories contain files with multiple owners, in an unpredictable fashion (e.g. the unprivileged Apache user). I can think of several ways to handle this: 1) Use SFTP protocol version 4 or later, which only refers to owners/groups by name, instead of by uid/gid. However, this likely would require a large overhaul of OpenSSH and SSHFS, and it seems unlikely that either party would want to do this, since they'd still probably need to support protocol version 3 indefinitely. 2) Extend the SFTP protocol, using one of the existing mechanisms defined for this purpose. a) Use SSH_FILEXFER_ATTR_EXTENDED[1], etc to pass new username and groupname attributes, in addition to the uid and gid. SSH_FXP_SETSTAT calls could be made that omitted the SSH_FILEXFER_ATTR_UIDGID flag and uid and gid, and just passed the username and groupname, which could be looked up on the remote server before making the chown call. SSH_FXP_STAT calls, when received locally, could ignore the uid/gid provided if the given username and groupname can be resolved on the local server. b) Use SSH_FXP_EXTENDED[2] and define a new set of *stat, readdir calls that pass the username and groupname around instead of the uid & gid. Either of these approaches would also require the server advertising that it provides such an extension at connection time, and making sshfs die early if 'auto' mode is on, but the server won't support it. 3) Don't extend the SFTP protocol at all. Instead, also add an '-o uidmap=/path/to/something -o gidmap=/path/to/something-else', which would be a pair of local files which map remote UIDs to remote usernames. 4) Like #3, but would fetch the files from the remote server. Could even default to grabbing the info from /etc/passwd and /etc/groups on the remote server. My guess is that #2.a is probably the best approach, followed by #3. I'd also suggest that some options may need to exist for handling UIDs which don't map to a valid user on the local server. Options might be: 1) Just return an EPERM on any operations on these files by non-root. 2) Add an option to remap any UIDs like that to some specific local UID. 3) Just pass them as-is. This has the potential to expose files to some other local user that just happens to have the same UID. I'd be willing to develop patch(es) for SSHFS (and OpenSSH, as applicable) for whichever approach is most acceptable. I would like my patches to be merged upstream, but if this isn't a feature that's desired, any feedback on which approach(es) are the least problematic would still be appreciated. [1] http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-5 [2] http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-8 -- Mike Kelly |
From: Miklos S. <mi...@sz...> - 2011-12-13 08:40:55
|
Mike Kelly <mi...@pa...> writes: > Hi, > > I'd like to propose support for an extra mode for '-o idmap', "auto". > When "auto" mode is set, the UID/GID of each file gets translated to a > local UID/GID corresponding to the owner's name and group's name of the > file on the remote server. > > I have a situation where I want to mount, via sshfs, a remote file > system from a server which uses different username/groupname to uid/gid > mappings than the local server, but which will have some overlap in user > names & group names. I want to be able to mount this remote file system, > and have all my stat() calls, etc show the correctly remapped UID on > this local system. > > Using '-o idmap=none' won't work because the remote and local UID/GIDs > don't always match. Using '-o idmap=user' won't work because some > mounted directories contain files with multiple owners, in an > unpredictable fashion (e.g. the unprivileged Apache user). > > I can think of several ways to handle this: > > 1) Use SFTP protocol version 4 or later, which only refers to > owners/groups by name, instead of by uid/gid. However, this likely would > require a large overhaul of OpenSSH and SSHFS, and it seems unlikely > that either party would want to do this, since they'd still probably > need to support protocol version 3 indefinitely. > > 2) Extend the SFTP protocol, using one of the existing mechanisms > defined for this purpose. > > a) Use SSH_FILEXFER_ATTR_EXTENDED[1], etc to pass new username and > groupname attributes, in addition to the uid and gid. SSH_FXP_SETSTAT > calls could be made that omitted the SSH_FILEXFER_ATTR_UIDGID flag and > uid and gid, and just passed the username and groupname, which could be > looked up on the remote server before making the chown call. > SSH_FXP_STAT calls, when received locally, could ignore the uid/gid > provided if the given username and groupname can be resolved on the > local server. > > b) Use SSH_FXP_EXTENDED[2] and define a new set of *stat, readdir > calls that pass the username and groupname around instead of the uid & gid. > > Either of these approaches would also require the server advertising > that it provides such an extension at connection time, and making sshfs > die early if 'auto' mode is on, but the server won't support it. > > 3) Don't extend the SFTP protocol at all. Instead, also add an '-o > uidmap=/path/to/something -o gidmap=/path/to/something-else', which > would be a pair of local files which map remote UIDs to remote usernames. > > 4) Like #3, but would fetch the files from the remote server. Could even > default to grabbing the info from /etc/passwd and /etc/groups on the > remote server. > > My guess is that #2.a is probably the best approach, followed by #3. > > I'd also suggest that some options may need to exist for handling UIDs > which don't map to a valid user on the local server. Options might be: > > 1) Just return an EPERM on any operations on these files by non-root. > > 2) Add an option to remap any UIDs like that to some specific local UID. > > 3) Just pass them as-is. This has the potential to expose files to some > other local user that just happens to have the same UID. > > I'd be willing to develop patch(es) for SSHFS (and OpenSSH, as > applicable) for whichever approach is most acceptable. I would like my > patches to be merged upstream, but if this isn't a feature that's > desired, any feedback on which approach(es) are the least problematic > would still be appreciated. The non-extensions are the least problematic. I like both #3 and #4. If you want to go for a protocol extension, please consult with the openSSH guys first. Thanks, Miklos |
From: Mike K. <mi...@sm...> - 2011-12-13 15:45:09
|
On 12/13/2011 03:39 AM, Miklos Szeredi wrote: > Mike Kelly <mi...@pa...> writes: > >> Hi, >> >> I'd like to propose support for an extra mode for '-o idmap', "auto". >> When "auto" mode is set, the UID/GID of each file gets translated to a >> local UID/GID corresponding to the owner's name and group's name of the >> file on the remote server. >> [...] > > The non-extensions are the least problematic. I like both #3 and #4. > > If you want to go for a protocol extension, please consult with the > openSSH guys first. I posted to the <ope...@mi...> list last week looking for feedback on the protocol extension approach, but I haven't received any response yet: http://thread.gmane.org/gmane.network.openssh.devel/18363 -- Mike Kelly |
From: Mike K. <mi...@pa...> - 2011-12-16 20:17:53
|
These options allow you to create a pair of local files, with username:uid/groupname:gid pairs, one per line. Alternatively, files can be in standard /etc/passwd / /etc/group format. The uid/gids are for the remote server, their local counterparts are looked up with a local getpwnam/getgrnam call. Any stat() calls will show with the remapped local uid/gid, and any chown() calls will be remapped back to the remote uid/gid. --- ChangeLog | 7 ++ sshfs.1 | 12 +++ sshfs.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 240 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9b3fb9c..2fa431e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2011-12-16 Mike Kelly <mi...@pa...> + + * Add -o idmap=file, -o uidmap=FILE, -o gidmap=FILE. These options + allow you to create a pair of local files, similar to /etc/passwd or + /etc/group files from the remote server, and use those to remap all + the given UIDs/GIDs. + 2011-11-25 Miklos Szeredi <mi...@sz...> * Make chown respect the UID mapping policy. Reported and tested diff --git a/sshfs.1 b/sshfs.1 index d103ad3..9dcd8aa 100644 --- a/sshfs.1 +++ b/sshfs.1 @@ -102,6 +102,18 @@ no translation of the ID space (default) .TP user only translate UID of connecting user +.TP +file +translate UIDs/GIDs based upon the contents of \fBuidfile \fR and +\fBgidfile\fR +.RE +.TP +\fB\-o\fR uidfile=FILE +file containing username:uid mappings for \fBidmap=file\fR +.RE +.TP +\fB\-o\fR gidfile=FILE +file containing groupname:gid mappings for \fBidmap=file\fR .RE .TP \fB\-o\fR ssh_command=CMD diff --git a/sshfs.c b/sshfs.c index d77d8f7..0dade1b 100644 --- a/sshfs.c +++ b/sshfs.c @@ -35,6 +35,8 @@ #include <netinet/in.h> #include <netinet/tcp.h> #include <glib.h> +#include <pwd.h> +#include <grp.h> #include "cache.h" @@ -198,6 +200,13 @@ struct sshfs { int follow_symlinks; int no_check_root; int detect_uid; + int idmap; + char *uid_file; + char *gid_file; + GHashTable *uid_map; + GHashTable *gid_map; + GHashTable *r_uid_map; + GHashTable *r_gid_map; unsigned max_read; unsigned max_write; unsigned ssh_ver; @@ -306,6 +315,12 @@ enum { KEY_CONFIGFILE, }; +enum { + IDMAP_NONE, + IDMAP_USER, + IDMAP_FILE, +}; + #define SSHFS_OPT(t, p, v) { t, offsetof(struct sshfs, p), v } static struct fuse_opt sshfs_opts[] = { @@ -317,8 +332,11 @@ static struct fuse_opt sshfs_opts[] = { SSHFS_OPT("ssh_protocol=%u", ssh_ver, 0), SSHFS_OPT("-1", ssh_ver, 1), SSHFS_OPT("workaround=%s", workarounds, 0), - SSHFS_OPT("idmap=none", detect_uid, 0), - SSHFS_OPT("idmap=user", detect_uid, 1), + SSHFS_OPT("idmap=none", idmap, IDMAP_NONE), + SSHFS_OPT("idmap=user", idmap, IDMAP_USER), + SSHFS_OPT("idmap=file", idmap, IDMAP_FILE), + SSHFS_OPT("uidfile=%s", uid_file, 0), + SSHFS_OPT("gidfile=%s", gid_file, 0), SSHFS_OPT("sshfs_sync", sync_write, 1), SSHFS_OPT("no_readahead", sync_read, 1), SSHFS_OPT("sshfs_debug", debug, 1), @@ -444,6 +462,15 @@ static int list_empty(const struct list_head *head) return head->next == head; } +/* given a pointer to the uid/gid, and the mapping table, remap the + * uid/gid, if necessary */ +static inline void translate_id(uint32_t *id, GHashTable *map) +{ + gpointer id_p; + if (g_hash_table_lookup_extended(map, GUINT_TO_POINTER(*id), NULL, &id_p)) + *id = GPOINTER_TO_UINT(id_p); +} + static inline void buf_init(struct buffer *buf, size_t size) { if (size) { @@ -681,6 +708,10 @@ static int buf_get_attrs(struct buffer *buf, struct stat *stbuf, int *flagsp) if (sshfs.remote_uid_detected && uid == sshfs.remote_uid) uid = sshfs.local_uid; + if (sshfs.idmap == IDMAP_FILE && sshfs.uid_map) + translate_id(&uid, sshfs.uid_map); + if (sshfs.idmap == IDMAP_FILE && sshfs.gid_map) + translate_id(&gid, sshfs.gid_map); memset(stbuf, 0, sizeof(struct stat)); stbuf->st_mode = mode; @@ -2158,6 +2189,10 @@ static int sshfs_chown(const char *path, uid_t uid, gid_t gid) if (sshfs.remote_uid_detected && uid == sshfs.local_uid) uid = sshfs.remote_uid; + if (sshfs.idmap == IDMAP_FILE && sshfs.r_uid_map) + translate_id(&uid, sshfs.r_uid_map); + if (sshfs.idmap == IDMAP_FILE && sshfs.r_gid_map) + translate_id(&gid, sshfs.r_gid_map); buf_init(&buf, 0); buf_add_path(&buf, path); @@ -3105,6 +3140,9 @@ static void usage(const char *progname) " -o idmap=TYPE user/group ID mapping, possible types are:\n" " none no translation of the ID space (default)\n" " user only translate UID of connecting user\n" +" file translate UIDs/GIDs contained in uidfile/gidfile\n" +" -o uidfile=FILE file containing username:remote_uid mappings\n" +" -o gidfile=FILE file containing groupname:remote_gid mappings\n" " -o ssh_command=CMD execute CMD instead of 'ssh'\n" " -o ssh_protocol=N ssh protocol to use (default: 2)\n" " -o sftp_server=SERV path to sftp server or subsystem (default: sftp)\n" @@ -3421,6 +3459,166 @@ static int ssh_connect(void) return 0; } +/* remove trailing '\n', like the perl func of the same name */ +static inline void chomp(char *line) +{ + char *p = line; + if ((p = strrchr(line, '\n'))) + *p = '\0'; +} + +/* number of ':' separated fields in a passwd/group file that we care + * about */ +#define IDMAP_FIELDS 3 + +/* given a line from a uidmap or gidmap, parse out the name and id */ +static void parse_idmap_line(char *line, const char* filename, + const unsigned int lineno, uint32_t *ret_id, char **ret_name) +{ + chomp(line); + char *tokens[IDMAP_FIELDS]; + char *tok; + int i; + for (i = 0; (tok = strsep(&line, ":")) && (i < IDMAP_FIELDS) ; i++) { + tokens[i] = tok; + } + + char *name_tok, *id_tok; + if (i == 2) { + /* assume name:id format */ + name_tok = tokens[0]; + id_tok = tokens[1]; + } else if (i >= IDMAP_FIELDS) { + /* assume passwd/group file format */ + name_tok = tokens[0]; + id_tok = tokens[2]; + } else { + fprintf(stderr, "Unknown format at line %u of '%s'\n", + lineno, filename); + exit(1); + } + + errno = 0; + uint32_t remote_id = strtoul(id_tok, NULL, 10); + if (errno) { + fprintf(stderr, "Invalid id number on line %u of '%s': %s\n", + lineno, filename, strerror(errno)); + exit(1); + } + + *ret_name = strdup(name_tok); + *ret_id = remote_id; +} + +/* read a uidmap or gidmap */ +static void read_id_map(char *file, uint32_t *(*map_fn)(char *), + const char *name_id, GHashTable **idmap, GHashTable **r_idmap) +{ + *idmap = g_hash_table_new(NULL, NULL); + *r_idmap = g_hash_table_new(NULL, NULL); + FILE *fp; + char *line = NULL; + size_t len = 0; + unsigned int lineno = 0; + + fp = fopen(file, "r"); + if (fp == NULL) { + fprintf(stderr, "failed to open '%s': %s\n", + file, strerror(errno)); + exit(1); + } + + while (getline(&line, &len, fp) != EOF) { + lineno++; + uint32_t remote_id; + char *name; + + parse_idmap_line(line, file, lineno, &remote_id, &name); + + uint32_t *local_id = map_fn(name); + if (local_id == NULL) { + /* not found */ + DEBUG("%s(%u): no local %s\n", name, remote_id, name_id); + free(name); + continue; + } + + DEBUG("%s: remote %s %u => local %s %u\n", + name, name_id, remote_id, name_id, *local_id); + g_hash_table_insert(*idmap, GUINT_TO_POINTER(remote_id), GUINT_TO_POINTER(*local_id)); + g_hash_table_insert(*r_idmap, GUINT_TO_POINTER(*local_id), GUINT_TO_POINTER(remote_id)); + free(name); + free(local_id); + } + + if (fclose(fp) == EOF) { + fprintf(stderr, "failed to close '%s': %s", + file, strerror(errno)); + exit(1); + } + + if (line) + free(line); +} + +/* given a username, return a pointer to its uid, or NULL if it doesn't + * exist on this system */ +static uint32_t *username_to_uid(char *name) +{ + errno = 0; + struct passwd *pw = getpwnam(name); + if (pw == NULL) { + if (errno == 0) { + /* "does not exist" */ + return NULL; + } + fprintf(stderr, "Failed to look up user '%s': %s\n", + name, strerror(errno)); + exit(1); + } + uint32_t *r = malloc(sizeof(uint32_t)); + if (r == NULL) { + fprintf(stderr, "sshfs: memory allocation failed\n"); + abort(); + } + *r = pw->pw_uid; + return r; +} + +/* given a groupname, return a pointer to its gid, or NULL if it doesn't + * exist on this system */ +static uint32_t *groupname_to_gid(char *name) +{ + errno = 0; + struct group *gr = getgrnam(name); + if (gr == NULL) { + if (errno == 0) { + /* "does not exist" */ + return NULL; + } + fprintf(stderr, "Failed to look up group '%s': %s\n", + name, strerror(errno)); + exit(1); + } + uint32_t *r = malloc(sizeof(uint32_t)); + if (r == NULL) { + fprintf(stderr, "sshfs: memory allocation failed\n"); + abort(); + } + *r = gr->gr_gid; + return r; +} + +static inline void load_uid_map(void) +{ + read_id_map(sshfs.uid_file, &username_to_uid, "uid", &sshfs.uid_map, &sshfs.r_uid_map); +} + +static inline void load_gid_map(void) +{ + read_id_map(sshfs.gid_file, &groupname_to_gid, "gid", &sshfs.gid_map, &sshfs.r_gid_map); +} + int main(int argc, char *argv[]) { int res; @@ -3447,6 +3645,8 @@ int main(int argc, char *argv[]) sshfs.ptyfd = -1; sshfs.ptyslavefd = -1; sshfs.delay_connect = 0; + sshfs.detect_uid = 0; + sshfs.idmap = IDMAP_NONE; ssh_add_arg("ssh"); ssh_add_arg("-x"); ssh_add_arg("-a"); @@ -3456,6 +3656,25 @@ int main(int argc, char *argv[]) parse_workarounds() == -1) exit(1); + if (sshfs.idmap == IDMAP_USER) + sshfs.detect_uid = 1; + else if (sshfs.idmap == IDMAP_FILE) { + sshfs.uid_map = NULL; + sshfs.gid_map = NULL; + sshfs.r_uid_map = NULL; + sshfs.r_gid_map = NULL; + if (!sshfs.uid_file && !sshfs.gid_file) { + fprintf(stderr, "need a uid_file or gid_file with idmap=file\n"); + exit(1); + } + if (sshfs.uid_file) + load_uid_map(); + if (sshfs.gid_file) + load_gid_map(); + } + free(sshfs.uid_file); + free(sshfs.gid_file); + DEBUG("SSHFS version %s\n", PACKAGE_VERSION); if (sshfs.password_stdin) { -- 1.7.7.3 |
From: Miklos S. <mi...@sz...> - 2011-12-20 14:48:48
|
Mike Kelly <mi...@pa...> writes: > These options allow you to create a pair of local files, with > username:uid/groupname:gid pairs, one per line. Alternatively, files can > be in standard /etc/passwd / /etc/group format. > > The uid/gids are for the remote server, their local counterparts are > looked up with a local getpwnam/getgrnam call. Any stat() calls will > show with the remapped local uid/gid, and any chown() calls will be > remapped back to the remote uid/gid. Okay, applied. I'm not sure how portable the getline() function is. Doesn't glib have something similar? Thanks, Miklos > --- > ChangeLog | 7 ++ > sshfs.1 | 12 +++ > sshfs.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 240 insertions(+), 2 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 9b3fb9c..2fa431e 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,10 @@ > +2011-12-16 Mike Kelly <mi...@pa...> > + > + * Add -o idmap=file, -o uidmap=FILE, -o gidmap=FILE. These options > + allow you to create a pair of local files, similar to /etc/passwd or > + /etc/group files from the remote server, and use those to remap all > + the given UIDs/GIDs. > + > 2011-11-25 Miklos Szeredi <mi...@sz...> > > * Make chown respect the UID mapping policy. Reported and tested > diff --git a/sshfs.1 b/sshfs.1 > index d103ad3..9dcd8aa 100644 > --- a/sshfs.1 > +++ b/sshfs.1 > @@ -102,6 +102,18 @@ no translation of the ID space (default) > .TP > user > only translate UID of connecting user > +.TP > +file > +translate UIDs/GIDs based upon the contents of \fBuidfile \fR and > +\fBgidfile\fR > +.RE > +.TP > +\fB\-o\fR uidfile=FILE > +file containing username:uid mappings for \fBidmap=file\fR > +.RE > +.TP > +\fB\-o\fR gidfile=FILE > +file containing groupname:gid mappings for \fBidmap=file\fR > .RE > .TP > \fB\-o\fR ssh_command=CMD > diff --git a/sshfs.c b/sshfs.c > index d77d8f7..0dade1b 100644 > --- a/sshfs.c > +++ b/sshfs.c > @@ -35,6 +35,8 @@ > #include <netinet/in.h> > #include <netinet/tcp.h> > #include <glib.h> > +#include <pwd.h> > +#include <grp.h> > > #include "cache.h" > > @@ -198,6 +200,13 @@ struct sshfs { > int follow_symlinks; > int no_check_root; > int detect_uid; > + int idmap; > + char *uid_file; > + char *gid_file; > + GHashTable *uid_map; > + GHashTable *gid_map; > + GHashTable *r_uid_map; > + GHashTable *r_gid_map; > unsigned max_read; > unsigned max_write; > unsigned ssh_ver; > @@ -306,6 +315,12 @@ enum { > KEY_CONFIGFILE, > }; > > +enum { > + IDMAP_NONE, > + IDMAP_USER, > + IDMAP_FILE, > +}; > + > #define SSHFS_OPT(t, p, v) { t, offsetof(struct sshfs, p), v } > > static struct fuse_opt sshfs_opts[] = { > @@ -317,8 +332,11 @@ static struct fuse_opt sshfs_opts[] = { > SSHFS_OPT("ssh_protocol=%u", ssh_ver, 0), > SSHFS_OPT("-1", ssh_ver, 1), > SSHFS_OPT("workaround=%s", workarounds, 0), > - SSHFS_OPT("idmap=none", detect_uid, 0), > - SSHFS_OPT("idmap=user", detect_uid, 1), > + SSHFS_OPT("idmap=none", idmap, IDMAP_NONE), > + SSHFS_OPT("idmap=user", idmap, IDMAP_USER), > + SSHFS_OPT("idmap=file", idmap, IDMAP_FILE), > + SSHFS_OPT("uidfile=%s", uid_file, 0), > + SSHFS_OPT("gidfile=%s", gid_file, 0), > SSHFS_OPT("sshfs_sync", sync_write, 1), > SSHFS_OPT("no_readahead", sync_read, 1), > SSHFS_OPT("sshfs_debug", debug, 1), > @@ -444,6 +462,15 @@ static int list_empty(const struct list_head *head) > return head->next == head; > } > > +/* given a pointer to the uid/gid, and the mapping table, remap the > + * uid/gid, if necessary */ > +static inline void translate_id(uint32_t *id, GHashTable *map) > +{ > + gpointer id_p; > + if (g_hash_table_lookup_extended(map, GUINT_TO_POINTER(*id), NULL, &id_p)) > + *id = GPOINTER_TO_UINT(id_p); > +} > + > static inline void buf_init(struct buffer *buf, size_t size) > { > if (size) { > @@ -681,6 +708,10 @@ static int buf_get_attrs(struct buffer *buf, struct stat *stbuf, int *flagsp) > > if (sshfs.remote_uid_detected && uid == sshfs.remote_uid) > uid = sshfs.local_uid; > + if (sshfs.idmap == IDMAP_FILE && sshfs.uid_map) > + translate_id(&uid, sshfs.uid_map); > + if (sshfs.idmap == IDMAP_FILE && sshfs.gid_map) > + translate_id(&gid, sshfs.gid_map); > > memset(stbuf, 0, sizeof(struct stat)); > stbuf->st_mode = mode; > @@ -2158,6 +2189,10 @@ static int sshfs_chown(const char *path, uid_t uid, gid_t gid) > > if (sshfs.remote_uid_detected && uid == sshfs.local_uid) > uid = sshfs.remote_uid; > + if (sshfs.idmap == IDMAP_FILE && sshfs.r_uid_map) > + translate_id(&uid, sshfs.r_uid_map); > + if (sshfs.idmap == IDMAP_FILE && sshfs.r_gid_map) > + translate_id(&gid, sshfs.r_gid_map); > > buf_init(&buf, 0); > buf_add_path(&buf, path); > @@ -3105,6 +3140,9 @@ static void usage(const char *progname) > " -o idmap=TYPE user/group ID mapping, possible types are:\n" > " none no translation of the ID space (default)\n" > " user only translate UID of connecting user\n" > +" file translate UIDs/GIDs contained in uidfile/gidfile\n" > +" -o uidfile=FILE file containing username:remote_uid mappings\n" > +" -o gidfile=FILE file containing groupname:remote_gid mappings\n" > " -o ssh_command=CMD execute CMD instead of 'ssh'\n" > " -o ssh_protocol=N ssh protocol to use (default: 2)\n" > " -o sftp_server=SERV path to sftp server or subsystem (default: sftp)\n" > @@ -3421,6 +3459,166 @@ static int ssh_connect(void) > return 0; > } > > +/* remove trailing '\n', like the perl func of the same name */ > +static inline void chomp(char *line) > +{ > + char *p = line; > + if ((p = strrchr(line, '\n'))) > + *p = '\0'; > +} > + > +/* number of ':' separated fields in a passwd/group file that we care > + * about */ > +#define IDMAP_FIELDS 3 > + > +/* given a line from a uidmap or gidmap, parse out the name and id */ > +static void parse_idmap_line(char *line, const char* filename, > + const unsigned int lineno, uint32_t *ret_id, char **ret_name) > +{ > + chomp(line); > + char *tokens[IDMAP_FIELDS]; > + char *tok; > + int i; > + for (i = 0; (tok = strsep(&line, ":")) && (i < IDMAP_FIELDS) ; i++) { > + tokens[i] = tok; > + } > + > + char *name_tok, *id_tok; > + if (i == 2) { > + /* assume name:id format */ > + name_tok = tokens[0]; > + id_tok = tokens[1]; > + } else if (i >= IDMAP_FIELDS) { > + /* assume passwd/group file format */ > + name_tok = tokens[0]; > + id_tok = tokens[2]; > + } else { > + fprintf(stderr, "Unknown format at line %u of '%s'\n", > + lineno, filename); > + exit(1); > + } > + > + errno = 0; > + uint32_t remote_id = strtoul(id_tok, NULL, 10); > + if (errno) { > + fprintf(stderr, "Invalid id number on line %u of '%s': %s\n", > + lineno, filename, strerror(errno)); > + exit(1); > + } > + > + *ret_name = strdup(name_tok); > + *ret_id = remote_id; > +} > + > +/* read a uidmap or gidmap */ > +static void read_id_map(char *file, uint32_t *(*map_fn)(char *), > + const char *name_id, GHashTable **idmap, GHashTable **r_idmap) > +{ > + *idmap = g_hash_table_new(NULL, NULL); > + *r_idmap = g_hash_table_new(NULL, NULL); > + FILE *fp; > + char *line = NULL; > + size_t len = 0; > + unsigned int lineno = 0; > + > + fp = fopen(file, "r"); > + if (fp == NULL) { > + fprintf(stderr, "failed to open '%s': %s\n", > + file, strerror(errno)); > + exit(1); > + } > + > + while (getline(&line, &len, fp) != EOF) { > + lineno++; > + uint32_t remote_id; > + char *name; > + > + parse_idmap_line(line, file, lineno, &remote_id, &name); > + > + uint32_t *local_id = map_fn(name); > + if (local_id == NULL) { > + /* not found */ > + DEBUG("%s(%u): no local %s\n", name, remote_id, name_id); > + free(name); > + continue; > + } > + > + DEBUG("%s: remote %s %u => local %s %u\n", > + name, name_id, remote_id, name_id, *local_id); > + g_hash_table_insert(*idmap, GUINT_TO_POINTER(remote_id), GUINT_TO_POINTER(*local_id)); > + g_hash_table_insert(*r_idmap, GUINT_TO_POINTER(*local_id), GUINT_TO_POINTER(remote_id)); > + free(name); > + free(local_id); > + } > + > + if (fclose(fp) == EOF) { > + fprintf(stderr, "failed to close '%s': %s", > + file, strerror(errno)); > + exit(1); > + } > + > + if (line) > + free(line); > +} > + > +/* given a username, return a pointer to its uid, or NULL if it doesn't > + * exist on this system */ > +static uint32_t *username_to_uid(char *name) > +{ > + errno = 0; > + struct passwd *pw = getpwnam(name); > + if (pw == NULL) { > + if (errno == 0) { > + /* "does not exist" */ > + return NULL; > + } > + fprintf(stderr, "Failed to look up user '%s': %s\n", > + name, strerror(errno)); > + exit(1); > + } > + uint32_t *r = malloc(sizeof(uint32_t)); > + if (r == NULL) { > + fprintf(stderr, "sshfs: memory allocation failed\n"); > + abort(); > + } > + *r = pw->pw_uid; > + return r; > +} > + > +/* given a groupname, return a pointer to its gid, or NULL if it doesn't > + * exist on this system */ > +static uint32_t *groupname_to_gid(char *name) > +{ > + errno = 0; > + struct group *gr = getgrnam(name); > + if (gr == NULL) { > + if (errno == 0) { > + /* "does not exist" */ > + return NULL; > + } > + fprintf(stderr, "Failed to look up group '%s': %s\n", > + name, strerror(errno)); > + exit(1); > + } > + uint32_t *r = malloc(sizeof(uint32_t)); > + if (r == NULL) { > + fprintf(stderr, "sshfs: memory allocation failed\n"); > + abort(); > + } > + *r = gr->gr_gid; > + return r; > +} > + > +static inline void load_uid_map(void) > +{ > + read_id_map(sshfs.uid_file, &username_to_uid, "uid", &sshfs.uid_map, &sshfs.r_uid_map); > +} > + > +static inline void load_gid_map(void) > +{ > + read_id_map(sshfs.gid_file, &groupname_to_gid, "gid", &sshfs.gid_map, &sshfs.r_gid_map); > +} > + > int main(int argc, char *argv[]) > { > int res; > @@ -3447,6 +3645,8 @@ int main(int argc, char *argv[]) > sshfs.ptyfd = -1; > sshfs.ptyslavefd = -1; > sshfs.delay_connect = 0; > + sshfs.detect_uid = 0; > + sshfs.idmap = IDMAP_NONE; > ssh_add_arg("ssh"); > ssh_add_arg("-x"); > ssh_add_arg("-a"); > @@ -3456,6 +3656,25 @@ int main(int argc, char *argv[]) > parse_workarounds() == -1) > exit(1); > > + if (sshfs.idmap == IDMAP_USER) > + sshfs.detect_uid = 1; > + else if (sshfs.idmap == IDMAP_FILE) { > + sshfs.uid_map = NULL; > + sshfs.gid_map = NULL; > + sshfs.r_uid_map = NULL; > + sshfs.r_gid_map = NULL; > + if (!sshfs.uid_file && !sshfs.gid_file) { > + fprintf(stderr, "need a uid_file or gid_file with idmap=file\n"); > + exit(1); > + } > + if (sshfs.uid_file) > + load_uid_map(); > + if (sshfs.gid_file) > + load_gid_map(); > + } > + free(sshfs.uid_file); > + free(sshfs.gid_file); > + > DEBUG("SSHFS version %s\n", PACKAGE_VERSION); > > if (sshfs.password_stdin) { |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:12
|
Miklos Szeredi <mi...@sz...> wrote: > Okay, applied. > > I'm not sure how portable the getline() function is. Doesn't glib > have something similar? getline() is part of POSIX 2008, but that doesn't make it as portable as I actually needed it to be (figured it out yesterday while testing elsewhere). This next patch series uses fgets() instead, which should be good enough for this use case, and tweaks the line parsing in a few other, minor ways. The other thing these patches add is the ability to error out when we try to work with UIDs that we don't have a mapping for. In many situations, this is probably the safer thing to do. Otherwise, someone with a UID on the local server that happens to correspond to some other user on the other server could mess with their files. I also noticed a typo in one of my error messages. |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:07
|
--- sshfs.c | 25 ++++++++++++------------- 1 files changed, 12 insertions(+), 13 deletions(-) diff --git a/sshfs.c b/sshfs.c index 359fbf2..2ba4659 100644 --- a/sshfs.c +++ b/sshfs.c @@ -3484,23 +3484,23 @@ static int ssh_connect(void) return 0; } -/* remove trailing '\n', like the perl func of the same name */ -static inline void chomp(char *line) -{ - char *p = line; - if ((p = strrchr(line, '\n'))) - *p = '\0'; -} - /* number of ':' separated fields in a passwd/group file that we care * about */ #define IDMAP_FIELDS 3 /* given a line from a uidmap or gidmap, parse out the name and id */ static void parse_idmap_line(char *line, const char* filename, - const unsigned int lineno, uint32_t *ret_id, char **ret_name) + const unsigned int lineno, uint32_t *ret_id, char **ret_name, + const int eof) { - chomp(line); + /* chomp off the trailing newline */ + char *p = line; + if ((p = strrchr(line, '\n'))) + *p = '\0'; + else if (!eof) { + fprintf(stderr, "%s:%u: line too long\n", filename, lineno); + exit(1); + } char *tokens[IDMAP_FIELDS]; char *tok; int i; @@ -3518,8 +3518,7 @@ static void parse_idmap_line(char *line, const char* filename, name_tok = tokens[0]; id_tok = tokens[2]; } else { - fprintf(stderr, "Unknown format at line %u of '%s'\n", - lineno, filename); + fprintf(stderr, "%s:%u: unknown format\n", filename, lineno); exit(1); } @@ -3557,7 +3556,7 @@ static void read_id_map(char *file, uint32_t *(*map_fn)(char *), uint32_t remote_id; char *name; - parse_idmap_line(line, file, lineno, &remote_id, &name); + parse_idmap_line(line, file, lineno, &remote_id, &name, feof(fp)); uint32_t *local_id = map_fn(name); if (local_id == NULL) { -- 1.7.7.3 |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:10
|
--- sshfs.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/sshfs.c b/sshfs.c index 2ba4659..ef2208d 100644 --- a/sshfs.c +++ b/sshfs.c @@ -3556,6 +3556,10 @@ static void read_id_map(char *file, uint32_t *(*map_fn)(char *), uint32_t remote_id; char *name; + /* skip blank lines */ + if (line[0] == '\n' || line[0] == '\0') + continue; + parse_idmap_line(line, file, lineno, &remote_id, &name, feof(fp)); uint32_t *local_id = map_fn(name); -- 1.7.7.3 |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:11
|
--- sshfs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sshfs.c b/sshfs.c index 749e134..8111f9c 100644 --- a/sshfs.c +++ b/sshfs.c @@ -3689,7 +3689,7 @@ int main(int argc, char *argv[]) sshfs.r_uid_map = NULL; sshfs.r_gid_map = NULL; if (!sshfs.uid_file && !sshfs.gid_file) { - fprintf(stderr, "need a uid_file or gid_file with idmap=file\n"); + fprintf(stderr, "need a uidfile or gidfile with idmap=file\n"); exit(1); } if (sshfs.uid_file) -- 1.7.7.3 |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:12
|
getline() isn't widely available yet, use fgets() instead --- sshfs.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-) diff --git a/sshfs.c b/sshfs.c index 8111f9c..359fbf2 100644 --- a/sshfs.c +++ b/sshfs.c @@ -37,6 +37,7 @@ #include <glib.h> #include <pwd.h> #include <grp.h> +#include <limits.h> #include "cache.h" @@ -3541,8 +3542,7 @@ static void read_id_map(char *file, uint32_t *(*map_fn)(char *), *idmap = g_hash_table_new(NULL, NULL); *r_idmap = g_hash_table_new(NULL, NULL); FILE *fp; - char *line = NULL; - size_t len = 0; + char line[LINE_MAX]; unsigned int lineno = 0; fp = fopen(file, "r"); @@ -3552,7 +3552,7 @@ static void read_id_map(char *file, uint32_t *(*map_fn)(char *), exit(1); } - while (getline(&line, &len, fp) != EOF) { + while (fgets(line, LINE_MAX, fp) != NULL) { lineno++; uint32_t remote_id; char *name; @@ -3580,9 +3580,6 @@ static void read_id_map(char *file, uint32_t *(*map_fn)(char *), file, strerror(errno)); exit(1); } - - if (line) - free(line); } /* given a username, return a pointer to its uid, or NULL if it doesn't -- 1.7.7.3 |
From: Mike K. <mi...@pa...> - 2011-12-20 15:28:13
|
add a '-o nomap=ignore|error' option, which defaults to 'error' --- sshfs.1 | 11 ++++++++ sshfs.c | 83 +++++++++++++++++++++++++++++++++++++++++---------------------- 2 files changed, 65 insertions(+), 29 deletions(-) diff --git a/sshfs.1 b/sshfs.1 index 9dcd8aa..27dd8bc 100644 --- a/sshfs.1 +++ b/sshfs.1 @@ -116,6 +116,17 @@ file containing username:uid mappings for \fBidmap=file\fR file containing groupname:gid mappings for \fBidmap=file\fR .RE .TP +\fB\-o\fR nomap=TYPE +with idmap=file, how to handle missing mappings +.RS 8 +.TP +ignore +don't do any re-mapping +.TP +error +return an error (default) +.RE +.TP \fB\-o\fR ssh_command=CMD execute CMD instead of 'ssh' .TP diff --git a/sshfs.c b/sshfs.c index bf7e956..749e134 100644 --- a/sshfs.c +++ b/sshfs.c @@ -201,6 +201,7 @@ struct sshfs { int no_check_root; int detect_uid; int idmap; + int nomap; char *uid_file; char *gid_file; GHashTable *uid_map; @@ -321,6 +322,11 @@ enum { IDMAP_FILE, }; +enum { + NOMAP_IGNORE, + NOMAP_ERROR, +}; + #define SSHFS_OPT(t, p, v) { t, offsetof(struct sshfs, p), v } static struct fuse_opt sshfs_opts[] = { @@ -337,6 +343,8 @@ static struct fuse_opt sshfs_opts[] = { SSHFS_OPT("idmap=file", idmap, IDMAP_FILE), SSHFS_OPT("uidfile=%s", uid_file, 0), SSHFS_OPT("gidfile=%s", gid_file, 0), + SSHFS_OPT("nomap=ignore", nomap, NOMAP_IGNORE), + SSHFS_OPT("nomap=error", nomap, NOMAP_ERROR), SSHFS_OPT("sshfs_sync", sync_write, 1), SSHFS_OPT("no_readahead", sync_read, 1), SSHFS_OPT("sshfs_debug", debug, 1), @@ -464,11 +472,20 @@ static int list_empty(const struct list_head *head) /* given a pointer to the uid/gid, and the mapping table, remap the * uid/gid, if necessary */ -static inline void translate_id(uint32_t *id, GHashTable *map) +static inline int translate_id(uint32_t *id, GHashTable *map) { gpointer id_p; - if (g_hash_table_lookup_extended(map, GUINT_TO_POINTER(*id), NULL, &id_p)) + if (g_hash_table_lookup_extended(map, GUINT_TO_POINTER(*id), NULL, &id_p)) { *id = GPOINTER_TO_UINT(id_p); + return 0; + } + switch (sshfs.nomap) { + case NOMAP_ERROR: return -1; + case NOMAP_IGNORE: return 0; + default: + fprintf(stderr, "internal error\n"); + abort(); + } } static inline void buf_init(struct buffer *buf, size_t size) @@ -672,36 +689,36 @@ static int buf_get_attrs(struct buffer *buf, struct stat *stbuf, int *flagsp) uint32_t mode = S_IFREG | 0777; if (buf_get_uint32(buf, &flags) == -1) - return -1; + return -EIO; if (flagsp) *flagsp = flags; if ((flags & SSH_FILEXFER_ATTR_SIZE) && buf_get_uint64(buf, &size) == -1) - return -1; + return -EIO; if ((flags & SSH_FILEXFER_ATTR_UIDGID) && (buf_get_uint32(buf, &uid) == -1 || buf_get_uint32(buf, &gid) == -1)) - return -1; + return -EIO; if ((flags & SSH_FILEXFER_ATTR_PERMISSIONS) && buf_get_uint32(buf, &mode) == -1) - return -1; + return -EIO; if ((flags & SSH_FILEXFER_ATTR_ACMODTIME)) { if (buf_get_uint32(buf, &atime) == -1 || buf_get_uint32(buf, &mtime) == -1) - return -1; + return -EIO; } if ((flags & SSH_FILEXFER_ATTR_EXTENDED)) { uint32_t extcount; unsigned i; if (buf_get_uint32(buf, &extcount) == -1) - return -1; + return -EIO; for (i = 0; i < extcount; i++) { struct buffer tmp; if (buf_get_data(buf, &tmp) == -1) - return -1; + return -EIO; buf_free(&tmp); if (buf_get_data(buf, &tmp) == -1) - return -1; + return -EIO; buf_free(&tmp); } } @@ -709,9 +726,11 @@ static int buf_get_attrs(struct buffer *buf, struct stat *stbuf, int *flagsp) if (sshfs.remote_uid_detected && uid == sshfs.remote_uid) uid = sshfs.local_uid; if (sshfs.idmap == IDMAP_FILE && sshfs.uid_map) - translate_id(&uid, sshfs.uid_map); + if (translate_id(&uid, sshfs.uid_map) == -1) + return -EPERM; if (sshfs.idmap == IDMAP_FILE && sshfs.gid_map) - translate_id(&gid, sshfs.gid_map); + if (translate_id(&gid, sshfs.gid_map) == -1) + return -EPERM; memset(stbuf, 0, sizeof(struct stat)); stbuf->st_mode = mode; @@ -778,7 +797,7 @@ static int buf_get_entries(struct buffer *buf, fuse_cache_dirh_t h, unsigned i; if (buf_get_uint32(buf, &count) == -1) - return -1; + return -EIO; for (i = 0; i < count; i++) { int err = -1; @@ -786,16 +805,16 @@ static int buf_get_entries(struct buffer *buf, fuse_cache_dirh_t h, char *longname; struct stat stbuf; if (buf_get_string(buf, &name) == -1) - return -1; + return -EIO; if (buf_get_string(buf, &longname) != -1) { free(longname); - if (buf_get_attrs(buf, &stbuf, NULL) != -1) { + err = buf_get_attrs(buf, &stbuf, NULL); + if (!err) { if (sshfs.follow_symlinks && S_ISLNK(stbuf.st_mode)) { stbuf.st_mode = 0; } filler(h, name, &stbuf); - err = 0; } } free(name); @@ -1552,7 +1571,7 @@ static void sftp_detect_uid() fprintf(stderr, "failed to stat home directory (%i)\n", serr); goto out; } - if (buf_get_attrs(&buf, &stbuf, &flags) == -1) + if (buf_get_attrs(&buf, &stbuf, &flags) != 0) goto out; if (!(flags & SSH_FILEXFER_ATTR_UIDGID)) @@ -1610,8 +1629,12 @@ static int sftp_check_root(const char *base_path) goto out; } - if (buf_get_attrs(&buf, &stbuf, &flags) == -1) + + int err2 = buf_get_attrs(&buf, &stbuf, &flags); + if (err2) { + err = err2; goto out; + } if (!(flags & SSH_FILEXFER_ATTR_PERMISSIONS)) goto out; @@ -1867,8 +1890,7 @@ static int sshfs_getattr(const char *path, struct stat *stbuf) err = sftp_request(sshfs.follow_symlinks ? SSH_FXP_STAT : SSH_FXP_LSTAT, &buf, SSH_FXP_ATTRS, &outbuf); if (!err) { - if (buf_get_attrs(&outbuf, stbuf, NULL) == -1) - err = -EIO; + err = buf_get_attrs(&outbuf, stbuf, NULL); buf_free(&outbuf); } buf_free(&buf); @@ -1990,8 +2012,7 @@ static int sshfs_getdir(const char *path, fuse_cache_dirh_t h, struct buffer name; err = sftp_request(SSH_FXP_READDIR, &handle, SSH_FXP_NAME, &name); if (!err) { - if (buf_get_entries(&name, h, filler) == -1) - err = -EIO; + err = buf_get_entries(&name, h, filler); buf_free(&name); } } while (!err); @@ -2190,9 +2211,11 @@ static int sshfs_chown(const char *path, uid_t uid, gid_t gid) if (sshfs.remote_uid_detected && uid == sshfs.local_uid) uid = sshfs.remote_uid; if (sshfs.idmap == IDMAP_FILE && sshfs.r_uid_map) - translate_id(&uid, sshfs.r_uid_map); + if(translate_id(&uid, sshfs.r_uid_map) == -1) + return -EPERM; if (sshfs.idmap == IDMAP_FILE && sshfs.r_gid_map) - translate_id(&gid, sshfs.r_gid_map); + if (translate_id(&gid, sshfs.r_gid_map) == -1) + return -EPERM; buf_init(&buf, 0); buf_add_path(&buf, path); @@ -2314,8 +2337,7 @@ static int sshfs_open_common(const char *path, mode_t mode, type = sshfs.follow_symlinks ? SSH_FXP_STAT : SSH_FXP_LSTAT; err2 = sftp_request(type, &buf, SSH_FXP_ATTRS, &outbuf); if (!err2) { - if (buf_get_attrs(&outbuf, &stbuf, NULL) == -1) - err2 = -EIO; + err2 = buf_get_attrs(&outbuf, &stbuf, NULL); buf_free(&outbuf); } err = sftp_request_wait(open_req, SSH_FXP_OPEN, SSH_FXP_HANDLE, @@ -2927,8 +2949,7 @@ static int sshfs_fgetattr(const char *path, struct stat *stbuf, buf_add_buf(&buf, &sf->handle); err = sftp_request(SSH_FXP_FSTAT, &buf, SSH_FXP_ATTRS, &outbuf); if (!err) { - if (buf_get_attrs(&outbuf, stbuf, NULL) == -1) - err = -EIO; + err = buf_get_attrs(&outbuf, stbuf, NULL); buf_free(&outbuf); } buf_free(&buf); @@ -3143,6 +3164,9 @@ static void usage(const char *progname) " file translate UIDs/GIDs contained in uidfile/gidfile\n" " -o uidfile=FILE file containing username:remote_uid mappings\n" " -o gidfile=FILE file containing groupname:remote_gid mappings\n" +" -o nomap=TYPE with idmap=file, how to handle missing mappings\n" +" ignore don't do any re-mapping\n" +" error return an error (default)\n" " -o ssh_command=CMD execute CMD instead of 'ssh'\n" " -o ssh_protocol=N ssh protocol to use (default: 2)\n" " -o sftp_server=SERV path to sftp server or subsystem (default: sftp)\n" @@ -3452,7 +3476,7 @@ static int ssh_connect(void) return -1; if (!sshfs.no_check_root && - sftp_check_root(sshfs.base_path) == -1) + sftp_check_root(sshfs.base_path) != 0) return -1; } @@ -3647,6 +3671,7 @@ int main(int argc, char *argv[]) sshfs.delay_connect = 0; sshfs.detect_uid = 0; sshfs.idmap = IDMAP_NONE; + sshfs.nomap = NOMAP_ERROR; ssh_add_arg("ssh"); ssh_add_arg("-x"); ssh_add_arg("-a"); -- 1.7.7.3 |
From: Mikael S. <mi...@st...> - 2011-12-30 09:36:51
|
On 2011-12-20 16:27, Mike Kelly wrote: > The other thing these patches add is the ability to error out when we > try to work with UIDs that we don't have a mapping for. In many > situations, this is probably the safer thing to do. Otherwise, someone > with a UID on the local server that happens to correspond to some other > user on the other server could mess with their files. Without knowing all the details about your patches, I would say that you should never rely on the client side (sshfs) to enforce security. Anyone who wishes to mess with someone elses files could easily bypass sshfs and make a direct sftp connection to the server. Preventing any operation on the client side that would actually be accepted by the server will only hide security problems on the server (either by bad implementation or misconfiguration). Permission checking on the client side should only be an optimization to avoid making remote calls that are known to fail. |
From: Mike K. <mi...@pa...> - 2012-01-04 16:39:00
|
On Fri 30 Dec 2011 04:21:19 AM EST, Mikael Ståldal wrote: > On 2011-12-20 16:27, Mike Kelly wrote: >> The other thing these patches add is the ability to error out when we >> try to work with UIDs that we don't have a mapping for. In many >> situations, this is probably the safer thing to do. Otherwise, someone >> with a UID on the local server that happens to correspond to some other >> user on the other server could mess with their files. > > Without knowing all the details about your patches, I would say that you > should never rely on the client side (sshfs) to enforce security. Anyone > who wishes to mess with someone elses files could easily bypass sshfs > and make a direct sftp connection to the server. > > Preventing any operation on the client side that would actually be > accepted by the server will only hide security problems on the server > (either by bad implementation or misconfiguration). > > Permission checking on the client side should only be an optimization to > avoid making remote calls that are known to fail. The main use case I have is basically something more akin to NFS. So, I'd be running the mount locally as root (using the `-o allow_other` switch, as well as `-o default_permissions`, `-o ro`, etc), and connecting to the remote server as root (using ssh keys). So, yes, there's a certain amount of implicit trust of the remote server here. Why not just use NFS? Because this allows us to work better in a heterogeneous environment, specifically one where users may exist with different UIDs on different servers. No NFS tools seemed to exist that I saw that would allow this level of UID remapping. -- Mike Kelly |
From: Mikael S. <mi...@st...> - 2012-01-04 20:25:33
|
On 2012-01-04 17:38, Mike Kelly wrote: > The main use case I have is basically something more akin to NFS. So, > I'd be running the mount locally as root (using the `-o allow_other` > switch, as well as `-o default_permissions`, `-o ro`, etc), and > connecting to the remote server as root (using ssh keys). So, yes, > there's a certain amount of implicit trust of the remote server here. The NFS security model with its implicit trust is seriously broken. Or to be fair, not at all suitable for today's Internet (it was maybe OK when it was first introduced in 1984.) If I understand it correctly, NFSv4 is moving away from that security model. SSHFS should not try to mimic the old (pre-v4) NFS security model, SSHFS should not utilize implicit trust. |
From: Mike K. <mi...@sm...> - 2012-01-04 20:44:49
|
On 01/04/2012 03:25 PM, Mikael Ståldal wrote: > On 2012-01-04 17:38, Mike Kelly wrote: >> The main use case I have is basically something more akin to NFS. So, >> I'd be running the mount locally as root (using the `-o allow_other` >> switch, as well as `-o default_permissions`, `-o ro`, etc), and >> connecting to the remote server as root (using ssh keys). So, yes, >> there's a certain amount of implicit trust of the remote server here. > > The NFS security model with its implicit trust is seriously broken. Or > to be fair, not at all suitable for today's Internet (it was maybe OK > when it was first introduced in 1984.) If I understand it correctly, > NFSv4 is moving away from that security model. > > SSHFS should not try to mimic the old (pre-v4) NFS security model, SSHFS > should not utilize implicit trust. I agree with that sentiment in general. However, do you know of an alternative solution which supports my needs? Because I haven't found one. All of the UID/GID mapping options I've seen for NFS are not able to do what I require. From what I remember finding, they only support "no remapping" or "remap everything to this specific UID/GID", and not "arbitrary UID/GID remapping". -- Mike Kelly |
From: Mikael S. <mi...@st...> - 2012-01-04 22:21:56
|
On 2012-01-04 21:44, Mike Kelly wrote: > I agree with that sentiment in general. However, do you know of an > alternative solution which supports my needs? Because I haven't found > one. All of the UID/GID mapping options I've seen for NFS are not able > to do what I require. From what I remember finding, they only support > "no remapping" or "remap everything to this specific UID/GID", and not > "arbitrary UID/GID remapping". IMHO, the proper way of using SSHFS in your use case is that each user of the client system have his own SSH(FS) connection to his own account on the server, and each user have his own set of SSH keys. And each server should be responsible for giving each user access to the proper set of files on that server. Using NFSv3 or SSHFS as root as you describe is not a proper solution to your problem, and it will likely cause security holes. If SSHFS as user is not feasible, then you should try NFSv4 or AFS, and use Kerberos rather than SSH. |
From: Paul W. <pa...@cy...> - 2012-01-04 23:16:11
|
On 1/4/2012 2:21 PM, Mikael Ståldal wrote: > On 2012-01-04 21:44, Mike Kelly wrote: >> I agree with that sentiment in general. However, do you know of an >> alternative solution which supports my needs? Because I haven't found >> one. All of the UID/GID mapping options I've seen for NFS are not able >> to do what I require. From what I remember finding, they only support >> "no remapping" or "remap everything to this specific UID/GID", and not >> "arbitrary UID/GID remapping". > > IMHO, the proper way of using SSHFS in your use case is that each user > of the client system have his own SSH(FS) connection to his own account > on the server, and each user have his own set of SSH keys. And each > server should be responsible for giving each user access to the proper > set of files on that server. > > Using NFSv3 or SSHFS as root as you describe is not a proper solution to > your problem, and it will likely cause security holes. > > If SSHFS as user is not feasible, then you should try NFSv4 or AFS, and > use Kerberos rather than SSH. The problem that a lot of people have is they forget the meaning of FUSE, namely Filesystem in USERSPACE. The key word is USERSPACE. This tool was designed to handle a problem where a user needs access to some remote data as a filesystem, but the server or local operating system does not support that directly. The user is able to use FUSE and one of the plugins, such as sshfs, to access that remote data through a translation layer that makes that data look like a filesystem. This works so well, that it is natural to forget the USERSPACE part and attempt to implement functionality that generally requires KERNELSPACE and a server that is aware of what is going on. Problems crop up if all the magic occurs on the client in USERSPACE and the remote server is oblivious to that magic. In this case, all the remote server knows is that some user has authenticated via ssh and is issuing file manipulation commands. It has no concept of multiple users or access permissions for multiple users over that single ssh session. To accomplish that would require each user to initiate their own ssh connection with their own credentials. It is simply wrong to expect a server such as sshd to authenticate a connection from one user and then understand that it should give access based on the credentials of a different user. If this functionality is needed then a different protocol should be used. |
From: Mikael S. <mi...@st...> - 2012-01-05 10:46:13
|
On 2012-01-04 23:59, Paul Wallingford wrote: > It is simply wrong to expect a server such as sshd to authenticate a > connection from one user and then understand that it should give access > based on the credentials of a different user. Exactly. And it is equally wrong to expect SSHFS to emulate this behavior on the client side without cooperation from the server. > If this functionality is needed then a different protocol should be used. Yes, such as AFS (I have used it and it works), or possibly NFSv4 (I have not tried it, I'm not sure if it works for this case). |
From: Mike K. <mi...@pa...> - 2011-12-16 20:21:16
|
sshfs.c: In function 'sshfs_read_end': sshfs.c:2383: warning: 'serr' is used uninitialized in this function --- sshfs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sshfs.c b/sshfs.c index 323ab6c..374f905 100644 --- a/sshfs.c +++ b/sshfs.c @@ -2380,7 +2380,7 @@ static void sshfs_read_end(struct request *req) rreq->res = -EIO; if (req->reply_type == SSH_FXP_STATUS) { - uint32_t serr; + uint32_t serr = 0; if (buf_get_uint32(&req->reply, &serr) != -1) { if (serr == SSH_FX_EOF) rreq->res = 0; -- 1.7.7.3 |
From: Miklos S. <mi...@sz...> - 2011-12-20 14:46:11
|
Mike Kelly <mi...@pa...> writes: > sshfs.c: In function 'sshfs_read_end': > sshfs.c:2383: warning: 'serr' is used uninitialized in this function > --- > sshfs.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/sshfs.c b/sshfs.c > index 323ab6c..374f905 100644 > --- a/sshfs.c > +++ b/sshfs.c > @@ -2380,7 +2380,7 @@ static void sshfs_read_end(struct request *req) > rreq->res = -EIO; > > if (req->reply_type == SSH_FXP_STATUS) { > - uint32_t serr; > + uint32_t serr = 0; That just hides the bug. Warnings are not always bogus ;) This patch should actually fix it: --- sshfs.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sshfs.c b/sshfs.c index 323ab6c..22c1e2d 100644 --- a/sshfs.c +++ b/sshfs.c @@ -2384,8 +2384,8 @@ static void sshfs_read_end(struct request *req) if (buf_get_uint32(&req->reply, &serr) != -1) { if (serr == SSH_FX_EOF) rreq->res = 0; - } else { - rreq->res = -sftp_error_to_errno(serr); + else + rreq->res = -sftp_error_to_errno(serr); } } else if (req->reply_type == SSH_FXP_DATA) { uint32_t retsize; -- 1.7.7 |
From: Mike K. <mi...@pa...> - 2011-12-16 20:22:54
|
Attached are a few patches for minor bug fixes I noticed, but the main patch is #4, which implements "#3" from my original post. |
From: Mike K. <mi...@pa...> - 2011-12-16 20:16:14
|
s/FD_CLOESEC/FD_CLOEXEC/ --- sshfs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sshfs.c b/sshfs.c index 374f905..59b7e4b 100644 --- a/sshfs.c +++ b/sshfs.c @@ -3554,7 +3554,7 @@ int main(int argc, char *argv[]) res = fcntl(fuse_chan_fd(ch), F_SETFD, FD_CLOEXEC); if (res == -1) - perror("WARNING: failed to set FD_CLOESEC on fuse device"); + perror("WARNING: failed to set FD_CLOEXEC on fuse device"); fuse = fuse_new(ch, &args, cache_init(&sshfs_oper), sizeof(struct fuse_operations), NULL); -- 1.7.7.3 |
From: Mike K. <mi...@pa...> - 2011-12-16 20:24:06
|
--- sshfs.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/sshfs.c b/sshfs.c index 59b7e4b..d77d8f7 100644 --- a/sshfs.c +++ b/sshfs.c @@ -1382,16 +1382,20 @@ static int sftp_init_reply_ok(struct buffer *buf, uint32_t *version) struct buffer buf2; buf_init(&buf2, len - 5); - if (do_read(&buf2) == -1) + if (do_read(&buf2) == -1) { + buf_free(&buf2); return -1; + } do { char *ext; char *extdata; if (buf_get_string(&buf2, &ext) == -1 || - buf_get_string(&buf2, &extdata) == -1) + buf_get_string(&buf2, &extdata) == -1) { + buf_free(&buf2); return -1; + } DEBUG("Extension: %s <%s>\n", ext, extdata); @@ -1407,6 +1411,7 @@ static int sftp_init_reply_ok(struct buffer *buf, uint32_t *version) strcmp(extdata, "1") == 0) sshfs.ext_hardlink = 1; } while (buf2.len < buf2.size); + buf_free(&buf2); } return 0; } -- 1.7.7.3 |
From: Mike K. <mi...@sm...> - 2011-12-16 20:59:45
|
On 12/16/2011 02:52 PM, Mike Kelly wrote: > Attached are a few patches for minor bug fixes I noticed, but the main > patch is #4, which implements "#3" from my original post. Along with these patches, I've noticed that the only enforcement of file permissions seems to happen on the remote server's end. While that's important, it seems that, at least optionally, permissions should be enforced locally, based upon any translated UIDs/GIDs. I can already see that if, say, I try to 'touch sshfsmount/tmp/bar', then LSTAT calls are made against "/", "/tmp", and "/tmp/bar". Actually, "/tmp/bar" is LSTAT'd twice. So, it seems like we should be getting the information we need to locally enforce permissions, without the need for extra remote calls. We just need to enforce EPERM in here somehow. Any suggestions on where to add that would be appreciated. -- Mike Kelly |
From: Mike K. <mi...@pa...> - 2011-12-17 14:30:45
|
On Fri, Dec 16, 2011 at 03:59:38PM -0500, Mike Kelly wrote: > On 12/16/2011 02:52 PM, Mike Kelly wrote: > > Attached are a few patches for minor bug fixes I noticed, but the main > > patch is #4, which implements "#3" from my original post. > > Along with these patches, I've noticed that the only enforcement of file > permissions seems to happen on the remote server's end. While that's > important, it seems that, at least optionally, permissions should be > enforced locally, based upon any translated UIDs/GIDs. Nevermind, just realized what '-o default_permissions' does... -- Mike Kelly |