openssh bugs - Aug 2009 - [Bug 1637] New: Change the context when starting internal-sftp

https://bugzilla.mindrot.org/show_bug.cgi?id=1637

           Summary: Change the context when starting internal-sftp
           Product: Portable OpenSSH
           Version: 5.2p1
          Platform: Other
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: sftp-server
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: jchadima at redhat.com


--- Comment #0 from jchadima at redhat.com 2009-08-28 15:38:36 EST ---
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637



--- Comment #1 from jchadima at redhat.com 2009-08-28 15:39:49 EST ---
Created an attachment (id=1681)
Patch solving the problem

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637



--- Comment #2 from Darren Tucker <dtucker at zip.com.au> 2009-08-28
17:38:39 EST ---
(From update of attachment 1681)
>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 at 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));
> 	}
>
-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637



--- Comment #3 from jchadima at redhat.com 2009-08-31 15:55:45 EST ---
Created an attachment (id=1683)
patch version 2

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637


Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1681|0                           |1
        is obsolete|                            |
   Attachment #1683|0                           |1
        is obsolete|                            |


--- Comment #4 from Darren Tucker <dtucker at zip.com.au> 2009-08-31
18:55:36 EST ---
Created an attachment (id=1687)
Move code to port-linux.c, give variables meaningful names, correct
strlcpy bounds check

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637


Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au


--- Comment #5 from Darren Tucker <dtucker at zip.com.au> 2009-09-01
18:33:56 EST ---
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637


jchadima at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jchadima at redhat.com


--- Comment #6 from jchadima at redhat.com 2009-09-02 05:44:49 EST ---
Everything is ok (only the newlen is 6 bytes longer than needed... but
it's nothing)

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1626
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #7 from Darren Tucker <dtucker at zip.com.au> 2009-10-24
15:04:37 EST ---
Thanks.  Patch applied, it will be in the 5.4p1 release.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1637

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #8 from Darren Tucker <dtucker at zip.com.au> 2010-03-26
10:51:22 EST ---
With the release of 5.4p1, this bug is now considered closed.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.