The sshd run with ssdh_t context. The sftpd runs with sftpd_t context. Internal-sftp do not use exec.* (2) syscall, so there is a need to switch context manually.
Created attachment 1681 [details] Patch solving the problem
Comment on attachment 1681 [details] Patch solving the problem >diff -up openssh-5.2p1/session.c.sesftp openssh-5.2p1/session.c >--- openssh-5.2p1/session.c.sesftp 2009-01-28 06:29:49.000000000 +0100 >+++ openssh-5.2p1/session.c 2009-08-08 13:13:54.670122454 +0200 >@@ -58,6 +58,7 @@ > #include <stdlib.h> > #include <string.h> > #include <unistd.h> >+#include <selinux/selinux.h> > > #include "openbsd-compat/sys-queue.h" > #include "xmalloc.h" >@@ -1791,8 +1792,8 @@ do_child(Session *s, const char *command > > if (s->is_subsystem == SUBSYSTEM_INT_SFTP) { > extern int optind, optreset; >- int i; >- char *p, *args; >+ int i, l; please don't use "l" as a variable name, my eyeballs easily misparse as a 1. >+ char *p, *args, *c1, *c2, *cx; > > setproctitle("%s@internal-sftp-server", s->pw->pw_name); > args = xstrdup(command ? command : "sftp-server"); >@@ -1802,6 +1803,27 @@ do_child(Session *s, const char *command > argv[i] = NULL; > optind = optreset = 1; > __progname = argv[0]; >+ if (getcon (&c1) < 0) { getcon is a linux (in fact, selinux) specific function, so having this here this will break on every other platform. Please put this in its own function in port-linux.c with the rest of the selinux code and wrap the call in #ifdef WITH_SELINUX Also, the man page for getcon says the returned context must be freed. It also says that it takes a security_context_t not a char * (typedefs in the headers notwithstanding). >+ logit("do_child: getcon failed witch %s", strerror (errno)); >+ } else { >+ c2 = xmalloc (strlen (c1) + 8); 8 is a magic number. I assume it's sizeof("sftpd_t"), in which case you should make sftpd_t a #define and use the sizeof. c2 is never freed. >+ if (!(cx = index (c1, ':'))) >+ goto badcontext; >+ if (!(cx = index (cx + 1, ':'))) { >+badcontext: >+ logit ("do_child: unparseable context %s", c1); >+ } else { >+ l = cx - c1 + 1; >+ memcpy (c2, c1, l); >+ strcpy (c2 + l, "sftpd_t"); unbounded str* functions are poor form even if this particular one is safe. Please use strl{cat,cpy}. >+ if ((cx = index (cx + 1, ':'))) >+ strcat (c2, cx); ditto. >+ if (setcon (c2) < 0) >+ logit("do_child: setcon failed witch %s", strerror (errno)); s/witch/with/ >+ >+ } >+ } >+ > exit(sftp_server_main(i, argv, s->pw)); > } >
Created attachment 1683 [details] patch version 2
Created attachment 1687 [details] Move code to port-linux.c, give variables meaningful names, correct strlcpy bounds check
Could you please confirm that patch #1687 behaves as you expect? Based on the debug output it seems to do the right thing for me. Also does the function name make sense? Thanks.
Everything is ok (only the newlen is 6 bytes longer than needed... but it's nothing)
Thanks. Patch applied, it will be in the 5.4p1 release.
With the release of 5.4p1, this bug is now considered closed.