chroot sftp-server [PATCH]
Patrick Higgins
--=-eXcnP+XwYgXaBAwLx1kt
Content-Type: text/plain
I'm working on setting up a semi-trusted sftp service, and to get it
working, I need chroot capability.
I've taken the /./ wuftpd magic token code from contrib/chroot.diff and
put it into the sftp server. The main problem is that privileges have
been dropped by the time the subsystem is exec'ed, so my patch requires
that sftp-server be setuid root. Not ideal, I know, but I drop all
privileges immediately after chroot'ing.
There's probably a better way to find out what the home directory should
be, but I'm currently just using $HOME (only chrooting if it contains
/./, though). I can't use getpwuid(getuid()) because I'm mapping several
users (with different $HOME's) to a single uid. Any ideas?
I've attached my patch. Hopefully it's useful to someone else.
Have you given more thought to how you'd ultimately like this to work,
Markus?
-Patrick Higgins
--=-eXcnP+XwYgXaBAwLx1kt
Content-Type: text/plain
Content-Disposition: attachment; filename=sftp-chroot.diff
Content-ID: 990663893.2...@phiggins.transzap.com
Content-Transfer-Encoding: 7bit
--- sftp-server.c Fri Apr 13 08:28:42 2001
+++ sftp-server.c.chroot Wed May 23 18:16:07 2001
@@ -33,6 +33,8 @@
#include "sftp.h"
#include "sftp-common.h"
+#define CHROOT
+
/* helper */
#define get_int64() buffer_get_int64(&iqueue);
#define get_int() buffer_get_int(&iqueue);
@@ -1024,6 +1026,36 @@
}
}
+#ifdef CHROOT
+void
+chroot_init(void)
+{
+ char *user_dir, *new_root;
+
+ user_dir = getenv("HOME");
+ if (!user_dir)
+ fatal("HOME isn't in environment");
+
+ new_root = user_dir + 1;
+
+ while ((new_root = strchr(new_root, '.')) != NULL) {
+ new_root--;
+ if (strncmp(new_root, "/./", 3) == 0) {
+ *new_root = '\0';
+ new_root += 2;
+
+ if (chroot(user_dir) != 0)
+ fatal("Couldn't chroot to user directory %s: %s",
+ user_dir, strerror(errno));
+
+ setenv("HOME", new_root, 1);
+ break;
+ }
+ new_root += 2;
+ }
+}
+#endif /* CHROOT */
+
int
main(int ac, char **av)
{
@@ -1039,6 +1071,12 @@
#ifdef DEBUG_SFTP_SERVER
log_init("sftp-server", SYSLOG_LEVEL_DEBUG1, SYSLOG_FACILITY_AUTH, 0);
#endif
+
+#ifdef CHROOT
+ chroot_init();
+#endif
+ if (setuid(getuid()) != 0)
+ fatal("Couldn't drop privileges: %s", strerror(errno));
in = dup(STDIN_FILENO);
out = dup(STDOUT_FILENO);
--=-eXcnP+XwYgXaBAwLx1kt--
mou...@etoh.eviladmin.org
On 23 May 2001, Patrick Higgins wrote:
> I'm working on setting up a semi-trusted sftp service, and to get it
> working, I need chroot capability.
>
Actually I was looking at it from a different point of view.
Instead of requiring setuid sftp-sever and the use of chroot(). Carefully
crafted realpath() usage and strncmp() should do the same thing.
This is a VERY VERY limited test. (As in.. compiles.. and looks like it
works.=)
I know it can be cleaned up.. but it's where I left off in my testing.
Markus, is there anything else I should worry about using this method?
- Ben
--- ../cvs/OpenSSH/src/usr.bin/ssh/sftp-server.c Thu Apr 5 05:42:53 2001
+++ sftp-server.c Wed May 23 19:54:06 2001
@@ -357,6 +357,33 @@
/* parse incoming */
+char *jailpath;
+
+char*
+getpath(u_int32_t id)
+{
+ char resolvedpath[MAXPATHLEN];
+ char *path;
+
+ path = get_string(NULL);
+
+ if (realpath(path, resolvedpath) == NULL) {
+ send_status(id, errno_to_portable(errno));
+ xfree(path);
+ return(NULL);
+ }
+ xfree(path);
+
+ if (jailpath) {
+ if (strncmp(resolvedpath, jailpath, strlen(jailpath))) {
+ send_status(id,SSH2_FX_PERMISSION_DENIED);
+ return(NULL);
+ }
+ }
+
+ return(xstrdup(resolvedpath));
+}
+
void
process_init(void)
{
@@ -380,7 +407,10 @@
int handle, fd, flags, mode, status = SSH2_FX_FAILURE;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
pflags = get_int(); /* portable flags */
a = get_attrib();
flags = flags_from_portable(pflags);
@@ -505,7 +535,10 @@
int ret, status = SSH2_FX_FAILURE;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
TRACE("%sstat id %d name %s", do_lstat ? "l" : "", id, name);
ret = do_lstat ? lstat(name, &st) : stat(name, &st);
if (ret < 0) {
@@ -580,7 +613,10 @@
int status = SSH2_FX_OK;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
a = get_attrib();
TRACE("setstat id %d name %s", id, name);
if (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) {
@@ -646,7 +682,10 @@
u_int32_t id;
id = get_int();
- path = get_string(NULL);
+ path = getpath(id);
+ if (path == NULL)
+ return;
+
TRACE("opendir id %d path %s", id, path);
dirp = opendir(path);
if (dirp == NULL) {
@@ -768,7 +807,10 @@
int ret;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
TRACE("remove id %d name %s", id, name);
ret = unlink(name);
status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
@@ -785,7 +827,10 @@
int ret, mode, status = SSH2_FX_FAILURE;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
a = get_attrib();
mode = (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) ?
a->perm & 0777 : 0777;
@@ -804,7 +849,10 @@
int ret, status;
id = get_int();
- name = get_string(NULL);
+ name = getpath(id);
+ if (name == NULL)
+ return;
+
TRACE("rmdir id %d name %s", id, name);
ret = rmdir(name);
status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
@@ -820,7 +868,10 @@
char *path;
id = get_int();
- path = get_string(NULL);
+ path = getpath(id);
+ if (path == NULL)
+ return;
+
if (path[0] == '\0') {
xfree(path);
path = xstrdup(".");
@@ -846,8 +897,14 @@
int ret, status = SSH2_FX_FAILURE;
id = get_int();
- oldpath = get_string(NULL);
- newpath = get_string(NULL);
+ oldpath = getpath(id);
+ if (oldpath == NULL)
+ return;
+
+ newpath = getpath(id);
+ if (newpath == NULL)
+ return;
+
TRACE("rename id %d old %s new %s", id, oldpath, newpath);
/* fail if 'newpath' exists */
if (stat(newpath, &st) == -1) {
@@ -867,7 +924,10 @@
char *path;
id = get_int();
- path = get_string(NULL);
+ path = getpath(id);
+ if (path == NULL)
+ return;
+
TRACE("readlink id %d path %s", id, path);
if (readlink(path, link, sizeof(link) - 1) == -1)
send_status(id, errno_to_portable(errno));
@@ -891,8 +951,14 @@
int ret, status = SSH2_FX_FAILURE;
id = get_int();
- oldpath = get_string(NULL);
- newpath = get_string(NULL);
+ oldpath = getpath(id);
+ if (oldpath == NULL)
+ return;
+
+ newpath = getpath(id);
+ if (newpath == NULL)
+ return;
+
TRACE("symlink id %d old %s new %s", id, oldpath, newpath);
/* fail if 'newpath' exists */
if (stat(newpath, &st) == -1) {
@@ -1004,6 +1070,32 @@
}
}
+char*
+jail_init(void)
+{
+ char *user_dir, *new_root;
+
+ user_dir = getenv("HOME");
+ if (!user_dir)
+ fatal("HOME isn't in environment");
+
+ new_root = user_dir + 1;
+
+ while ((new_root = strchr(new_root, '.')) != NULL) {
+ new_root--;
+ if (strncmp(new_root, "/./", 3) == 0) {
+ *new_root = '\0';
+ new_root += 2;
+
+ return(xstrdup(user_dir));
+ /*setenv("HOME", new_root, 1);*/
+ break;
+ }
+ new_root += 2;
+ }
+ return NULL;
+}
+
int
main(int ac, char **av)
{
@@ -1018,6 +1110,8 @@
#ifdef DEBUG_SFTP_SERVER
log_init("sftp-server", SYSLOG_LEVEL_DEBUG1, SYSLOG_FACILITY_AUTH, 0);
#endif
+
+ jailpath = jail_init();
mou...@etoh.eviladmin.org
Outside the fact that realpath() requires the file aspect of the path to
exist which breaks 'rename' and 'symlink' =)
- Ben
Andrew Bartlett
and was looking at implementing a while ago. I'm glad sombodies taken a
stab at it.
I run SFTP specificly becouse it does not require a ROOT deamon (apart
from OpenSSH, which I run already) nor does it require a set-uid
binary. Hence my interest in this patch.
Andrew Bartlett
--
Andrew Bartlett
abar...@pcug.org.au
mou...@etoh.eviladmin.org
There are few issues I need to sort out. As I said.. symlink and
rename commands break under my current patch. And I need to find a better
way of doing error checking. I'd like to also include the option to
'jail' someone in a subdirectory off their home directory. Thus removing
their ability to modify dot files in their home directory. But that may
require changes to sshd.
I'm sure it will work fine. Just when I get done I'll need a few other
people to look over the patch to ensure I did not miss any edge cases.
I'll have a more robust patch in a day or two.
- Ben
Patrick Higgins
going to need greater configurability. I noticed the comment at the
beginning that it should use getopt(), but in order to get that to work,
the code which spawns it is going to need some new features...
It seems possible to put options into the sftp subsystem definition in
your sshd_config, but they would be the same for all users. It would be
nice to have some kind of variable syntax to express things that change
(like home directories). Perhaps a full-blown config file for
sftp-server would be more appropriate?
While I'm on the topic of how the subsystem is exec'ed, I'd like to ask
why it's exec'ed with the user's shell instead of /bin/sh. This seems to
prevent me from giving sftp access but *not* ssh. It appears to have
been done more or less for code reuse--perhaps do_exec_*_pty() and
do_child() should take the shell to use as an argument, so that
session_subsystem_req() could always specify /bin/sh?
Perhaps I just don't know enough about the protocol--does it even make
sense to grant subsystem access without granting normal ssh access? It
seems to work if I make the user's login shell be
/usr/lib/libexec/sftp-server, so I'm assume it does...
Andrew Bartlett
>
> All of this should be quite possible, but I think the sftp-server is
> going to need greater configurability. I noticed the comment at the
> beginning that it should use getopt(), but in order to get that to work,
> the code which spawns it is going to need some new features...
>
> It seems possible to put options into the sftp subsystem definition in
> your sshd_config, but they would be the same for all users. It would be
> nice to have some kind of variable syntax to express things that change
> (like home directories). Perhaps a full-blown config file for
> sftp-server would be more appropriate?
>
> While I'm on the topic of how the subsystem is exec'ed, I'd like to ask
> why it's exec'ed with the user's shell instead of /bin/sh. This seems to
> prevent me from giving sftp access but *not* ssh. It appears to have
> been done more or less for code reuse--perhaps do_exec_*_pty() and
> do_child() should take the shell to use as an argument, so that
> session_subsystem_req() could always specify /bin/sh?
Quite the opposite. I give my users a restricted shell, and this way I
can limit per-user use. I simply let my taint-mode perl script accept
the -c option, compare it with the list of permitted programs (currently
just sftp-server) and run it with the appropriate option. If I so
cared, I could configure the shell to only allow sftp to certain users,
all from the restricted shell. We should be careful that we always use
the user's shell, for backward compatibility with setups that expect
that their restricted shell actually restricts their users in some way.
>
> Perhaps I just don't know enough about the protocol--does it even make
> sense to grant subsystem access without granting normal ssh access? It
> seems to work if I make the user's login shell be
> /usr/lib/libexec/sftp-server, so I'm assume it does...
Almost exactly what I do, except that I also allow users to change their
passwords, and have an auto Maildir fixup hack. I might add a remote
pine option in the future.
--
Andrew Bartlett
abar...@pcug.org.au
Damien Miller
> Is there any way of making this work? This is the method I much prefer,
> and was looking at implementing a while ago. I'm glad sombodies taken a
> stab at it.
>
> I run SFTP specificly becouse it does not require a ROOT deamon (apart
> from OpenSSH, which I run already) nor does it require a set-uid
> binary. Hence my interest in this patch.
I am not to fussed about a setuid sftp-server, so long as it does
does chdir,chroot,setuid as its first actions. IMO this is preferable
to patch-checking schemes which introduce complexity and may be
possible to fool.
-d
--
| Damien Miller <d...@mindrot.org> \ ``E-mail attachments are the poor man's
| http://www.mindrot.org / distributed filesystem'' - Dan Geer
Andrew Bartlett
>
> On Fri, 25 May 2001, Andrew Bartlett wrote:
>
> > Is there any way of making this work? This is the method I much prefer,
> > and was looking at implementing a while ago. I'm glad sombodies taken a
> > stab at it.
> >
> > I run SFTP specificly becouse it does not require a ROOT deamon (apart
> > from OpenSSH, which I run already) nor does it require a set-uid
> > binary. Hence my interest in this patch.
>
> I am not to fussed about a setuid sftp-server, so long as it does
> does chdir,chroot,setuid as its first actions. IMO this is preferable
> to patch-checking schemes which introduce complexity and may be
> possible to fool.
>
Unfortunetly it would (if I understand it correctly) break things like
symbolic links, if they were so unfortunate as to be absolute, rather
than relitive, would it not?
For example, i have a 'shared folder' system that uses links from
~/groupname to /home/groups/groupname. I was intending to restirct my
users to files under /home with a patch like this, as it seemed the best
solution.
Anyway, thats my two bobs worth.
Andrew Bartlett
--
Andrew Bartlett
abar...@pcug.org.au
mou...@etoh.eviladmin.org
On Fri, 25 May 2001, Andrew Bartlett wrote:
> Damien Miller wrote:
> >
> > On Fri, 25 May 2001, Andrew Bartlett wrote:
> >
> > > Is there any way of making this work? This is the method I much prefer,
> > > and was looking at implementing a while ago. I'm glad sombodies taken a
> > > stab at it.
> > >
> > > I run SFTP specificly becouse it does not require a ROOT deamon (apart
> > > from OpenSSH, which I run already) nor does it require a set-uid
> > > binary. Hence my interest in this patch.
> >
> > I am not to fussed about a setuid sftp-server, so long as it does
> > does chdir,chroot,setuid as its first actions. IMO this is preferable
> > to patch-checking schemes which introduce complexity and may be
> > possible to fool.
> >
>
That is my main concern also. However, I don't think that the patch I'm
working on introduces that much complexity. And as long as 'realpath()'
does it job then it should be fairly secure.
> Unfortunetly it would (if I understand it correctly) break things like
> symbolic links, if they were so unfortunate as to be absolute, rather
> than relitive, would it not?
>
> For example, i have a 'shared folder' system that uses links from
> ~/groupname to /home/groups/groupname. I was intending to restirct my
> users to files under /home with a patch like this, as it seemed the best
> solution.
>
It really depends on how your OS handles symlinks. In the symlink tests I
did linking /tmp to ~/tmp I found that I could not cd ~/tmp because it
happen to be a soft link and realpath() resolved it correctly and it was
denied.
- Ben
Andrew Bartlett
>
> On Fri, 25 May 2001, Andrew Bartlett wrote:
>
> > Damien Miller wrote:
> > >
> > > On Fri, 25 May 2001, Andrew Bartlett wrote:
> > >
> > > > Is there any way of making this work? This is the method I much prefer,
> > > > and was looking at implementing a while ago. I'm glad sombodies taken a
> > > > stab at it.
> > > >
> > > > I run SFTP specificly becouse it does not require a ROOT deamon (apart
> > > > from OpenSSH, which I run already) nor does it require a set-uid
> > > > binary. Hence my interest in this patch.
> > >
> > > I am not to fussed about a setuid sftp-server, so long as it does
> > > does chdir,chroot,setuid as its first actions. IMO this is preferable
> > > to patch-checking schemes which introduce complexity and may be
> > > possible to fool.
> > >
> >
> That is my main concern also. However, I don't think that the patch I'm
> working on introduces that much complexity. And as long as 'realpath()'
> does it job then it should be fairly secure.
I like it. And the patch looks pretty sane to me.
>
> > Unfortunetly it would (if I understand it correctly) break things like
> > symbolic links, if they were so unfortunate as to be absolute, rather
> > than relitive, would it not?
> >
> > For example, i have a 'shared folder' system that uses links from
> > ~/groupname to /home/groups/groupname. I was intending to restirct my
> > users to files under /home with a patch like this, as it seemed the best
> > solution.
> >
>
> It really depends on how your OS handles symlinks. In the symlink tests I
> did linking /tmp to ~/tmp I found that I could not cd ~/tmp because it
> happen to be a soft link and realpath() resolved it correctly and it was
> denied.
>
> - Ben
Excellent. My concern was what an absolute symlink would do with the
chroot ideas that were floating about, as directories would no longer be
in the same place...
Keep up the good work,
Markus Friedl
> On Fri, 25 May 2001, Andrew Bartlett wrote:
>
> > Is there any way of making this work? This is the method I much prefer,
> > and was looking at implementing a while ago. I'm glad sombodies taken a
> > stab at it.
> >
> > I run SFTP specificly becouse it does not require a ROOT deamon (apart
> > from OpenSSH, which I run already) nor does it require a set-uid
> > binary. Hence my interest in this patch.
>
> I am not to fussed about a setuid sftp-server, so long as it does
> does chdir,chroot,setuid as its first actions. IMO this is preferable
> to patch-checking schemes which introduce complexity and may be
> possible to fool.
i think i agree with Damien on this issue.
if we want a restricted sftp-server, that the OS should take care
about what files can be accessed. an the simplest way to acheive
this is to have a croot() at the start of sftp-server.
this is much simpler that adding checks to every place in sftp-server
where pathnames are handled.
why not reuse this nice feature of the OS? why invent a new mechanism
if a nice and elegant mechanism already exists (with chroot)?
-markus