Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: OpenSSH: Dev

New Subsystem criteria for Match option block in OpenSSH server

 

 

OpenSSH dev RSS feed   Index | Next | Previous | View Threaded


nicola.muto at cryptolab

May 17, 2012, 7:19 AM

Post #1 of 25 (1813 views)
Permalink
New Subsystem criteria for Match option block in OpenSSH server

Hello everybody,

I'm a C/C++ consultant working for Ericsson.

I changed the OpenSSH-Portable code to add a new criteria
into the Match sshd_config option read by the sshd server.

The new criteria is "Subsystem"; so a conditional block based
on subsystem client request can now be added to the sshd_config
configuration server file to override settings in its global
section.

Ericsson requested this new Subsystem criteria because it needs
to "chroot" all the sftp session into a well defined directory
for all users logging into the server from an sftp client.

For details on the Ericsson needs, please refer to the following
OpenSSH mailing list thread

http://marc.info/?t=132698105300002&r=1&w=2

A little example of the Match Subsystem usage follows

Match Subsystem sftp
ChrootDirectory /path/to/sftp/data/jail/on/the/server
X11Forwarding no
AllowTcpForwarding no
ForceCommand internal-sftp

To ensure that the Match Subsystem conditional block will work
properly, that is, the server "chroots" into the directory
specified by the ChrootDirectory directive above, you must also
disable the privilege separation using the config directive

UsePrivilegeSeparation no

into the sshd_config file. At least I think so; I checked that
the sshd server will skip calling the safely_chroot function if I
keep the privilege separation enabled and login using a no-root user.

I worked on the portable branch of OpenSSH because this feature must be
installed on a SLES (SuSE linux Enterprise Server) distribution.

For the code changes I started from the openssh-6.0p1 released on April
22, 2012, and I followed changes recently made by Darren Tucker to add
other two criteria for the Match option; i.e. LocalAddress and
LocalPort.
So, my changes are minimized and will be gladly accepted by the OpenSSH
community. :-)
Thank you Darren.

I do not know who should accept and confirm my code changes. Also I
don't have
an account to check-in the changes into the OpenBSD SVC repository
Hans can you help me?

BRs
\\nm

The diff code list follows

===============================================================================
diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/auth.c
src/auth.c
===============================================================================
546a547
> ConnectionInfo connection_info;
548,549c549,554
< parse_server_match_config(&options, user,
< get_canonical_hostname(options.use_dns), get_remote_ipaddr());
---
> connection_info.user = user;
> connection_info.host = get_canonical_hostname(options.use_dns);
> connection_info.address = get_remote_ipaddr();
> connection_info.subsystem = NULL;
>
> parse_server_match_config(&options, &connection_info);

=======================================================================================
diff -r
/home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.c
src/servconf.c
=======================================================================================
601,602c601
< match_cfg_line(char **condition, int line, const char *user, const
char *host,
< const char *address)
---
> match_cfg_line(char **condition, int line, ConnectionInfo *ci)
606a606,614
> const char *user = NULL;
> const char *host = NULL;
> const char *address = NULL;
> const char *subsystem = NULL;
>
> /* Here the condition is hold
> * ci==NULL ==> user==NULL
> * that is, it is not possible that ci==NULL but user!=NULL
> */
608c616
< if (user == NULL)
---
> if (ci == NULL)
610,611c618,624
< else
< debug3("checking match for '%s' user %s host %s addr %s", cp,
---
> else {
> user = ci->user;
> host = ci->host;
> address = ci->address;
> subsystem = ci->subsystem;
>
> debug3("checking match for '%s' user %s host %s addr %s subsystem
> %s", cp,
613c626,627
< address ? address : "(null)");
---
> address ? address : "(null)", subsystem ? subsystem :
> "(null)");
> }
622c636,639
< if (!user) {
---
> if (!user) { /* For clarity: it should be "if (!ci || !user) {"
> but the two expressions have
> * exactly the same values under the above condition that
> * (ci==NULL) ==> (user==NULL).
> */
631a649,652
> if (!user) { /* Same comment as above */
> result = 0;
> continue;
> }
639c660
< if (!host) {
---
> if (!host) { /* Same comment on the condition as user above */
648a670,673
> if (!address) { /* Same comment on the condition as user above */
> result = 0;
> continue;
> }
660a686,695
> } else if (strcasecmp(attrib, "subsystem") == 0) {
> if (!subsystem) {
> result = 0;
> continue;
> }
> if (match_pattern_list(subsystem, arg, len, 0) != 1)
> result = 0;
> else
> debug("subsystem %.100s matched 'Subsystem %.100s' at "
> "line %d", subsystem, arg, line);
666c701,702
< if (user != NULL)
---
>
> if (ci != NULL)
667a704
>
713,714c750
< const char *filename, int linenum, int *activep, const char
*user,
< const char *host, const char *address)
---
> const char *filename, int linenum, int *activep, ConnectionInfo
> *conninfo)
745c781
< if (user == NULL) {
---
> if (conninfo == NULL) {
1316c1352,1354
< value = match_cfg_line(&cp, linenum, user, host, address);
---
>
> value = match_cfg_line(&cp, linenum, conninfo);
>
1454,1455c1492
< parse_server_match_config(ServerOptions *options, const char *user,
< const char *host, const char *address)
---
> parse_server_match_config(ServerOptions *options, ConnectionInfo
> *conninfo)
1460c1497
< parse_server_config(&mo, "reprocess config", &cfg, user, host,
address);
---
> parse_server_config(&mo, "reprocess config", &cfg, conninfo);
1463a1501,1536
> int
> parse_server_match_testspec(ConnectionInfo *ci, char *spec)
> {
> char *p;
>
> while ((p = strsep(&spec, ",")) && *p != '\0') {
> if (strncmp(p, "addr=", 5) == 0)
> ci->address = xstrdup(p + 5);
> else if (strncmp(p, "host=", 5) == 0)
> ci->host = xstrdup(p + 5);
> else if (strncmp(p, "user=", 5) == 0)
> ci->user = xstrdup(p + 5);
> else if (strncmp(p, "subsystem=", 10) == 0)
> ci->subsystem = xstrdup(p + 10);
> else {
> fprintf(stderr, "Invalid test mode specification %s\n", p);
> return -1;
> }
> }
> return 0;
> }
>
> /*
> * returns 1 for a complete spec, 0 for partial spec and -1 for an
> * empty spec.
> */
> int
> server_match_spec_complete(ConnectionInfo *ci)
> {
> if (ci->user && ci->host && ci->address)
> return 1; /* complete */
> if (!ci->user && !ci->host && !ci->address)
> return -1; /* empty */
> return 0; /* partial */
> }
>
1537c1610
< const char *user, const char *host, const char *address)
---
> ConnectionInfo *conninfo)
1545c1618,1620
< active = user ? 0 : 1;
---
>
> active = conninfo ? 0 : 1;
>
1549c1624
< linenum++, &active, user, host, address) != 0)
---
> linenum++, &active, conninfo) != 0)

=======================================================================================
diff -r
/home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.h
src/servconf.h
=======================================================================================
170a171,178
> /* Information about the incoming connection as used by Match */
> typedef struct {
> const char *user;
> const char *host; /* possibly resolved hostname */
> const char *address; /* remote address */
> const char *subsystem; /* Subsystem service requested by the remote
> client application */
> } ConnectionInfo;
>
186a195
>
188c197,198
< int *, const char *, const char *, const char *);
---
> int *, ConnectionInfo *);
>
191,193c201,207
< const char *, const char *, const char *);
< void parse_server_match_config(ServerOptions *, const char *, const
char *,
< const char *);
---
> ConnectionInfo *);
>
> void parse_server_match_config(ServerOptions *, ConnectionInfo *);
>
> int parse_server_match_testspec(ConnectionInfo *, char *);
> int server_match_spec_complete(ConnectionInfo *);
>

=====================================================================================
diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/session.c
src/session.c
=====================================================================================
571d570
<
825a825
>
2089a2090
> /* Searching for subsystem into the options repository */
2091,2105c2092,2120
< if (strcmp(subsys, options.subsystem_name[i]) == 0) {
< prog = options.subsystem_command[i];
< cmd = options.subsystem_args[i];
< if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
< s->is_subsystem = SUBSYSTEM_INT_SFTP;
< debug("subsystem: %s", prog);
< } else {
< if (stat(prog, &st) < 0)
< debug("subsystem: cannot stat %s: %s",
< prog, strerror(errno));
< s->is_subsystem = SUBSYSTEM_EXT;
< debug("subsystem: exec() %s", cmd);
< }
< success = do_exec(s, cmd) == 0;
< break;
---
> if (strcmp(subsys, options.subsystem_name[i]) == 0) break;
> }
>
> if (i < options.num_subsystems) { /* subsystem found */
> ConnectionInfo connection_info;
>
> connection_info.user = NULL;
> connection_info.host = NULL;
> connection_info.address = NULL;
> connection_info.subsystem = subsys;
>
> logit("subsystem request: Parsing server match config options: user
> == '%s', host == '%s', address == '%s', subsystem == '%s'",
> connection_info.user, connection_info.host,
> connection_info.address, connection_info.subsystem);
>
> parse_server_match_config(&options, &connection_info);
>
> prog = options.subsystem_command[i];
> cmd = options.subsystem_args[i];
>
> if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
> s->is_subsystem = SUBSYSTEM_INT_SFTP;
> debug("subsystem: %s", prog);
> } else {
> if (stat(prog, &st) < 0)
> debug("subsystem: cannot stat %s: %s",
> prog, strerror(errno));
> s->is_subsystem = SUBSYSTEM_EXT;
> debug("subsystem: exec() %s", cmd);
2106a2122,2123
>
> success = do_exec(s, cmd) == 0;

===============================================================================
diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.8
src/sshd.8
===============================================================================
111c111
< that would apply to the specified user, host, and address will be set
before
---
> that would apply to the specified user, host, address, and subsystem
> will be set before
116a117
> .Dq addr ,
118,119c119,127
< .Dq addr .
< All are required and may be supplied in any order, either with
multiple
---
> .Dq subsystem .
> Only
> .Dq user ,
> .Dq host ,
> and
> .Dq addr
> keywords are required;
> .Dq subsystem
> is optional. These parameters may be supplied in any order, either
> with multiple

===============================================================================
diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.c
src/sshd.c
===============================================================================
1323d1322
< char *test_user = NULL, *test_host = NULL, *test_addr = NULL;
1325c1324
< char *line, *p, *cp;
---
> char *line;
1330a1330
> ConnectionInfo connection_info;
1359a1360,1362
> connection_info.address = connection_info.host =
> connection_info.subsystem =
> connection_info.user = NULL;
>
1452,1465c1455,1456
< cp = optarg;
< while ((p = strsep(&cp, ",")) && *p != '\0') {
< if (strncmp(p, "addr=", 5) == 0)
< test_addr = xstrdup(p + 5);
< else if (strncmp(p, "host=", 5) == 0)
< test_host = xstrdup(p + 5);
< else if (strncmp(p, "user=", 5) == 0)
< test_user = xstrdup(p + 5);
< else {
< fprintf(stderr, "Invalid test "
< "mode specification %s\n", p);
< exit(1);
< }
< }
---
> if (parse_server_match_testspec(&connection_info, optarg) == -1)
> exit(1);
1477c1468
< "command-line", 0, NULL, NULL, NULL, NULL) != 0)
---
> "command-line", 0, NULL, NULL) != 0)
1533,1540c1524,1529
< if (test_flag >= 2 &&
< (test_user != NULL || test_host != NULL || test_addr != NULL)
< && (test_user == NULL || test_host == NULL || test_addr ==
NULL))
< fatal("user, host and addr are all required when testing "
< "Match configs");
< if (test_flag < 2 && (test_user != NULL || test_host != NULL ||
< test_addr != NULL))
< fatal("Config test connection parameter (-C) provided without "
---
> if ((test_flag >= 2) &&
> (server_match_spec_complete(&connection_info) == 0))
> fatal("user, host and addr are all required when testing "
> "Match configs");
>
> if ((test_flag < 2) &&
> (server_match_spec_complete(&connection_info) >= 0))
> fatal("Config test connection parameter (-C) provided without "
1551c1540
< &cfg, NULL, NULL, NULL);
---
> &cfg, NULL);
1713,1715c1702,1703
< if (test_user != NULL && test_addr != NULL && test_host != NULL)
< parse_server_match_config(&options, test_user,
< test_host, test_addr);
---
> if (server_match_spec_complete(&connection_info) == 1)
> parse_server_match_config(&options, &connection_info);
2005c1993
< authenticated:
---
> authenticated:

=============================================================================================
diff -r
/home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd_config.5
src/sshd_config.5
=============================================================================================
677a678
> .Cm Address ,
679c680
< .Cm Address .
---
> .Cm Subsystem .

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


dan at doxpara

May 17, 2012, 7:54 AM

Post #2 of 25 (1757 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Is it a problem that scp and shell are un-chrooted? Or are they handled elsewhere?

Sent from my iPhone

On May 17, 2012, at 7:19 AM, Nicola Muto <nicola.muto [at] cryptolab> wrote:

> Hello everybody,
>
> I'm a C/C++ consultant working for Ericsson.
>
> I changed the OpenSSH-Portable code to add a new criteria
> into the Match sshd_config option read by the sshd server.
>
> The new criteria is "Subsystem"; so a conditional block based
> on subsystem client request can now be added to the sshd_config
> configuration server file to override settings in its global
> section.
>
> Ericsson requested this new Subsystem criteria because it needs
> to "chroot" all the sftp session into a well defined directory
> for all users logging into the server from an sftp client.
>
> For details on the Ericsson needs, please refer to the following
> OpenSSH mailing list thread
>
> http://marc.info/?t=132698105300002&r=1&w=2
>
> A little example of the Match Subsystem usage follows
>
> Match Subsystem sftp
> ChrootDirectory /path/to/sftp/data/jail/on/the/server
> X11Forwarding no
> AllowTcpForwarding no
> ForceCommand internal-sftp
>
> To ensure that the Match Subsystem conditional block will work
> properly, that is, the server "chroots" into the directory
> specified by the ChrootDirectory directive above, you must also
> disable the privilege separation using the config directive
>
> UsePrivilegeSeparation no
>
> into the sshd_config file. At least I think so; I checked that
> the sshd server will skip calling the safely_chroot function if I
> keep the privilege separation enabled and login using a no-root user.
>
> I worked on the portable branch of OpenSSH because this feature must be
> installed on a SLES (SuSE linux Enterprise Server) distribution.
>
> For the code changes I started from the openssh-6.0p1 released on April
> 22, 2012, and I followed changes recently made by Darren Tucker to add
> other two criteria for the Match option; i.e. LocalAddress and LocalPort.
> So, my changes are minimized and will be gladly accepted by the OpenSSH
> community. :-)
> Thank you Darren.
>
> I do not know who should accept and confirm my code changes. Also I don't have
> an account to check-in the changes into the OpenBSD SVC repository
> Hans can you help me?
>
> BRs
> \\nm
>
> The diff code list follows
>
> ===============================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/auth.c src/auth.c
> ===============================================================================
> 546a547
>> ConnectionInfo connection_info;
> 548,549c549,554
> < parse_server_match_config(&options, user,
> < get_canonical_hostname(options.use_dns), get_remote_ipaddr());
> ---
>> connection_info.user = user;
>> connection_info.host = get_canonical_hostname(options.use_dns);
>> connection_info.address = get_remote_ipaddr();
>> connection_info.subsystem = NULL;
>>
>> parse_server_match_config(&options, &connection_info);
>
> =======================================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.c src/servconf.c
> =======================================================================================
> 601,602c601
> < match_cfg_line(char **condition, int line, const char *user, const char *host,
> < const char *address)
> ---
>> match_cfg_line(char **condition, int line, ConnectionInfo *ci)
> 606a606,614
>> const char *user = NULL;
>> const char *host = NULL;
>> const char *address = NULL;
>> const char *subsystem = NULL;
>>
>> /* Here the condition is hold
>> * ci==NULL ==> user==NULL
>> * that is, it is not possible that ci==NULL but user!=NULL
>> */
> 608c616
> < if (user == NULL)
> ---
>> if (ci == NULL)
> 610,611c618,624
> < else
> < debug3("checking match for '%s' user %s host %s addr %s", cp,
> ---
>> else {
>> user = ci->user;
>> host = ci->host;
>> address = ci->address;
>> subsystem = ci->subsystem;
>>
>> debug3("checking match for '%s' user %s host %s addr %s subsystem %s", cp,
> 613c626,627
> < address ? address : "(null)");
> ---
>> address ? address : "(null)", subsystem ? subsystem : "(null)");
>> }
> 622c636,639
> < if (!user) {
> ---
>> if (!user) { /* For clarity: it should be "if (!ci || !user) {" but the two expressions have
>> * exactly the same values under the above condition that
>> * (ci==NULL) ==> (user==NULL).
>> */
> 631a649,652
>> if (!user) { /* Same comment as above */
>> result = 0;
>> continue;
>> }
> 639c660
> < if (!host) {
> ---
>> if (!host) { /* Same comment on the condition as user above */
> 648a670,673
>> if (!address) { /* Same comment on the condition as user above */
>> result = 0;
>> continue;
>> }
> 660a686,695
>> } else if (strcasecmp(attrib, "subsystem") == 0) {
>> if (!subsystem) {
>> result = 0;
>> continue;
>> }
>> if (match_pattern_list(subsystem, arg, len, 0) != 1)
>> result = 0;
>> else
>> debug("subsystem %.100s matched 'Subsystem %.100s' at "
>> "line %d", subsystem, arg, line);
> 666c701,702
> < if (user != NULL)
> ---
>>
>> if (ci != NULL)
> 667a704
>>
> 713,714c750
> < const char *filename, int linenum, int *activep, const char *user,
> < const char *host, const char *address)
> ---
>> const char *filename, int linenum, int *activep, ConnectionInfo *conninfo)
> 745c781
> < if (user == NULL) {
> ---
>> if (conninfo == NULL) {
> 1316c1352,1354
> < value = match_cfg_line(&cp, linenum, user, host, address);
> ---
>>
>> value = match_cfg_line(&cp, linenum, conninfo);
>>
> 1454,1455c1492
> < parse_server_match_config(ServerOptions *options, const char *user,
> < const char *host, const char *address)
> ---
>> parse_server_match_config(ServerOptions *options, ConnectionInfo *conninfo)
> 1460c1497
> < parse_server_config(&mo, "reprocess config", &cfg, user, host, address);
> ---
>> parse_server_config(&mo, "reprocess config", &cfg, conninfo);
> 1463a1501,1536
>> int
>> parse_server_match_testspec(ConnectionInfo *ci, char *spec)
>> {
>> char *p;
>>
>> while ((p = strsep(&spec, ",")) && *p != '\0') {
>> if (strncmp(p, "addr=", 5) == 0)
>> ci->address = xstrdup(p + 5);
>> else if (strncmp(p, "host=", 5) == 0)
>> ci->host = xstrdup(p + 5);
>> else if (strncmp(p, "user=", 5) == 0)
>> ci->user = xstrdup(p + 5);
>> else if (strncmp(p, "subsystem=", 10) == 0)
>> ci->subsystem = xstrdup(p + 10);
>> else {
>> fprintf(stderr, "Invalid test mode specification %s\n", p);
>> return -1;
>> }
>> }
>> return 0;
>> }
>>
>> /*
>> * returns 1 for a complete spec, 0 for partial spec and -1 for an
>> * empty spec.
>> */
>> int
>> server_match_spec_complete(ConnectionInfo *ci)
>> {
>> if (ci->user && ci->host && ci->address)
>> return 1; /* complete */
>> if (!ci->user && !ci->host && !ci->address)
>> return -1; /* empty */
>> return 0; /* partial */
>> }
>>
> 1537c1610
> < const char *user, const char *host, const char *address)
> ---
>> ConnectionInfo *conninfo)
> 1545c1618,1620
> < active = user ? 0 : 1;
> ---
>>
>> active = conninfo ? 0 : 1;
>>
> 1549c1624
> < linenum++, &active, user, host, address) != 0)
> ---
>> linenum++, &active, conninfo) != 0)
>
> =======================================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.h src/servconf.h
> =======================================================================================
> 170a171,178
>> /* Information about the incoming connection as used by Match */
>> typedef struct {
>> const char *user;
>> const char *host; /* possibly resolved hostname */
>> const char *address; /* remote address */
>> const char *subsystem; /* Subsystem service requested by the remote client application */
>> } ConnectionInfo;
>>
> 186a195
>>
> 188c197,198
> < int *, const char *, const char *, const char *);
> ---
>> int *, ConnectionInfo *);
>>
> 191,193c201,207
> < const char *, const char *, const char *);
> < void parse_server_match_config(ServerOptions *, const char *, const char *,
> < const char *);
> ---
>> ConnectionInfo *);
>>
>> void parse_server_match_config(ServerOptions *, ConnectionInfo *);
>>
>> int parse_server_match_testspec(ConnectionInfo *, char *);
>> int server_match_spec_complete(ConnectionInfo *);
>>
>
> =====================================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/session.c src/session.c
> =====================================================================================
> 571d570
> <
> 825a825
>>
> 2089a2090
>> /* Searching for subsystem into the options repository */
> 2091,2105c2092,2120
> < if (strcmp(subsys, options.subsystem_name[i]) == 0) {
> < prog = options.subsystem_command[i];
> < cmd = options.subsystem_args[i];
> < if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
> < s->is_subsystem = SUBSYSTEM_INT_SFTP;
> < debug("subsystem: %s", prog);
> < } else {
> < if (stat(prog, &st) < 0)
> < debug("subsystem: cannot stat %s: %s",
> < prog, strerror(errno));
> < s->is_subsystem = SUBSYSTEM_EXT;
> < debug("subsystem: exec() %s", cmd);
> < }
> < success = do_exec(s, cmd) == 0;
> < break;
> ---
>> if (strcmp(subsys, options.subsystem_name[i]) == 0) break;
>> }
>>
>> if (i < options.num_subsystems) { /* subsystem found */
>> ConnectionInfo connection_info;
>>
>> connection_info.user = NULL;
>> connection_info.host = NULL;
>> connection_info.address = NULL;
>> connection_info.subsystem = subsys;
>>
>> logit("subsystem request: Parsing server match config options: user == '%s', host == '%s', address == '%s', subsystem == '%s'",
>> connection_info.user, connection_info.host,
>> connection_info.address, connection_info.subsystem);
>>
>> parse_server_match_config(&options, &connection_info);
>>
>> prog = options.subsystem_command[i];
>> cmd = options.subsystem_args[i];
>>
>> if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
>> s->is_subsystem = SUBSYSTEM_INT_SFTP;
>> debug("subsystem: %s", prog);
>> } else {
>> if (stat(prog, &st) < 0)
>> debug("subsystem: cannot stat %s: %s",
>> prog, strerror(errno));
>> s->is_subsystem = SUBSYSTEM_EXT;
>> debug("subsystem: exec() %s", cmd);
> 2106a2122,2123
>>
>> success = do_exec(s, cmd) == 0;
>
> ===============================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.8 src/sshd.8
> ===============================================================================
> 111c111
> < that would apply to the specified user, host, and address will be set before
> ---
>> that would apply to the specified user, host, address, and subsystem will be set before
> 116a117
>> .Dq addr ,
> 118,119c119,127
> < .Dq addr .
> < All are required and may be supplied in any order, either with multiple
> ---
>> .Dq subsystem .
>> Only
>> .Dq user ,
>> .Dq host ,
>> and
>> .Dq addr
>> keywords are required;
>> .Dq subsystem
>> is optional. These parameters may be supplied in any order, either with multiple
>
> ===============================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.c src/sshd.c
> ===============================================================================
> 1323d1322
> < char *test_user = NULL, *test_host = NULL, *test_addr = NULL;
> 1325c1324
> < char *line, *p, *cp;
> ---
>> char *line;
> 1330a1330
>> ConnectionInfo connection_info;
> 1359a1360,1362
>> connection_info.address = connection_info.host = connection_info.subsystem =
>> connection_info.user = NULL;
>>
> 1452,1465c1455,1456
> < cp = optarg;
> < while ((p = strsep(&cp, ",")) && *p != '\0') {
> < if (strncmp(p, "addr=", 5) == 0)
> < test_addr = xstrdup(p + 5);
> < else if (strncmp(p, "host=", 5) == 0)
> < test_host = xstrdup(p + 5);
> < else if (strncmp(p, "user=", 5) == 0)
> < test_user = xstrdup(p + 5);
> < else {
> < fprintf(stderr, "Invalid test "
> < "mode specification %s\n", p);
> < exit(1);
> < }
> < }
> ---
>> if (parse_server_match_testspec(&connection_info, optarg) == -1)
>> exit(1);
> 1477c1468
> < "command-line", 0, NULL, NULL, NULL, NULL) != 0)
> ---
>> "command-line", 0, NULL, NULL) != 0)
> 1533,1540c1524,1529
> < if (test_flag >= 2 &&
> < (test_user != NULL || test_host != NULL || test_addr != NULL)
> < && (test_user == NULL || test_host == NULL || test_addr == NULL))
> < fatal("user, host and addr are all required when testing "
> < "Match configs");
> < if (test_flag < 2 && (test_user != NULL || test_host != NULL ||
> < test_addr != NULL))
> < fatal("Config test connection parameter (-C) provided without "
> ---
>> if ((test_flag >= 2) && (server_match_spec_complete(&connection_info) == 0))
>> fatal("user, host and addr are all required when testing "
>> "Match configs");
>>
>> if ((test_flag < 2) && (server_match_spec_complete(&connection_info) >= 0))
>> fatal("Config test connection parameter (-C) provided without "
> 1551c1540
> < &cfg, NULL, NULL, NULL);
> ---
>> &cfg, NULL);
> 1713,1715c1702,1703
> < if (test_user != NULL && test_addr != NULL && test_host != NULL)
> < parse_server_match_config(&options, test_user,
> < test_host, test_addr);
> ---
>> if (server_match_spec_complete(&connection_info) == 1)
>> parse_server_match_config(&options, &connection_info);
> 2005c1993
> < authenticated:
> ---
>> authenticated:
>
> =============================================================================================
> diff -r /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd_config.5 src/sshd_config.5
> =============================================================================================
> 677a678
>> .Cm Address ,
> 679c680
> < .Cm Address .
> ---
>> .Cm Subsystem .
>
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev [at] mindrot
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


dtucker at zip

May 17, 2012, 10:25 PM

Post #3 of 25 (1754 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On Thu, May 17, 2012 at 04:19:36PM +0200, Nicola Muto wrote:
> Hello everybody,
>
> I'm a C/C++ consultant working for Ericsson.
>
> I changed the OpenSSH-Portable code to add a new criteria
> into the Match sshd_config option read by the sshd server.
>
> The new criteria is "Subsystem"; so a conditional block based

The problem with that is that Match is done at connection
establishment time and Subsystem is not a property of the connection,
it's a request type that can be sent zero or more times during the life
of the connection. What happens if I open a sftp subsytem then a normal
shell session or vice versa?

> you must also disable the privilege separation

that's usually a pretty good indication that you're doing something
wrong.

I'd like to study your diff a bit more but it got mangled to the point
that patch denies there's even a diff in there. Could you please resend
(a) using diff -u (unified) format and (b) as an text/plain attachment.

Thanks.

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 21, 2012, 12:12 AM

Post #4 of 25 (1760 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Hi,

> Is it a problem that scp and shell are un-chrooted?
> Or are they handled elsewhere?

I'm not sure what part you are referring to. What we need to be able to do is to place just the SFTP server in a chroot jail. But I'll give you some background so you can understand better what it is we want to do

We have a box/node/machine/server/whatever that runs SLES as Nicola wrote. This server has two SSH servers running; one on port 22 and one on port 4422. When logging in to port 22 credentials are first verified against /etc/passwd and then an LDAP server is checked if /etc/passwd failed, for port 4422 it is only /etc/passwd that is checked and SFTP subsystem is disabled.

If you just do a "normal" ssh login (without specifying any subsystem) you end up in what we call "COM CLI" which can be thought of as a text-based interface to an information model (you can navigate a tree structure, modify attributes and create objects, etc). This information model will also provide a branch showing a virtual filesystem. Thus this is a completely different animal compared to a Bash shell, it is purely a high level management interface for the server.

Now when connecting to port 22 using an SFTP client, this client may only see the same virtual filesystem that is also visible via the information model. Thus the SFTP server must run in a chrooted environment, but SSH (COM CLI) may not since the COM CLI must be able to access the complete system.

Connecting to port 4422 gives you a standard Bash shell.


Did this answer your question, or did I answer some other question which you did not ask? :)


/John

-----Original Message-----
From: Dan Kaminsky [mailto:dan [at] doxpara]
Sent: den 17 maj 2012 16:55
To: Nicola Muto
Cc: <openssh-unix-dev [at] mindrot>; Nicola Muto; John Olsson M; Hans Insulander
Subject: Re: New Subsystem criteria for Match option block in OpenSSH server

Is it a problem that scp and shell are un-chrooted? Or are they handled elsewhere?

Sent from my iPhone

On May 17, 2012, at 7:19 AM, Nicola Muto <nicola.muto [at] cryptolab> wrote:

> Hello everybody,
>
> I'm a C/C++ consultant working for Ericsson.
>
> I changed the OpenSSH-Portable code to add a new criteria into the
> Match sshd_config option read by the sshd server.
>
> The new criteria is "Subsystem"; so a conditional block based on
> subsystem client request can now be added to the sshd_config
> configuration server file to override settings in its global section.
>
> Ericsson requested this new Subsystem criteria because it needs to
> "chroot" all the sftp session into a well defined directory for all
> users logging into the server from an sftp client.
>
> For details on the Ericsson needs, please refer to the following
> OpenSSH mailing list thread
>
> http://marc.info/?t=132698105300002&r=1&w=2
>
> A little example of the Match Subsystem usage follows
>
> Match Subsystem sftp
> ChrootDirectory /path/to/sftp/data/jail/on/the/server
> X11Forwarding no
> AllowTcpForwarding no
> ForceCommand internal-sftp
>
> To ensure that the Match Subsystem conditional block will work
> properly, that is, the server "chroots" into the directory specified
> by the ChrootDirectory directive above, you must also disable the
> privilege separation using the config directive
>
> UsePrivilegeSeparation no
>
> into the sshd_config file. At least I think so; I checked that the
> sshd server will skip calling the safely_chroot function if I keep the
> privilege separation enabled and login using a no-root user.
>
> I worked on the portable branch of OpenSSH because this feature must
> be installed on a SLES (SuSE linux Enterprise Server) distribution.
>
> For the code changes I started from the openssh-6.0p1 released on
> April 22, 2012, and I followed changes recently made by Darren Tucker
> to add other two criteria for the Match option; i.e. LocalAddress and LocalPort.
> So, my changes are minimized and will be gladly accepted by the
> OpenSSH community. :-) Thank you Darren.
>
> I do not know who should accept and confirm my code changes. Also I
> don't have an account to check-in the changes into the OpenBSD SVC
> repository Hans can you help me?
>
> BRs
> \\nm
>
> The diff code list follows
>
> ======================================================================
> ========= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/auth.c
> src/auth.c
> ======================================================================
> =========
> 546a547
>> ConnectionInfo connection_info;
> 548,549c549,554
> < parse_server_match_config(&options, user,
> < get_canonical_hostname(options.use_dns), get_remote_ipaddr());
> ---
>> connection_info.user = user;
>> connection_info.host = get_canonical_hostname(options.use_dns);
>> connection_info.address = get_remote_ipaddr();
>> connection_info.subsystem = NULL;
>>
>> parse_server_match_config(&options, &connection_info);
>
> ======================================================================
> ================= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.c
> src/servconf.c
> ======================================================================
> =================
> 601,602c601
> < match_cfg_line(char **condition, int line, const char *user, const char *host,
> < const char *address)
> ---
>> match_cfg_line(char **condition, int line, ConnectionInfo *ci)
> 606a606,614
>> const char *user = NULL;
>> const char *host = NULL;
>> const char *address = NULL;
>> const char *subsystem = NULL;
>>
>> /* Here the condition is hold
>> * ci==NULL ==> user==NULL
>> * that is, it is not possible that ci==NULL but user!=NULL
>> */
> 608c616
> < if (user == NULL)
> ---
>> if (ci == NULL)
> 610,611c618,624
> < else
> < debug3("checking match for '%s' user %s host %s addr %s", cp,
> ---
>> else {
>> user = ci->user;
>> host = ci->host;
>> address = ci->address;
>> subsystem = ci->subsystem;
>>
>> debug3("checking match for '%s' user %s host %s addr %s
>> subsystem %s", cp,
> 613c626,627
> < address ? address : "(null)");
> ---
>> address ? address : "(null)", subsystem ? subsystem : "(null)");
>> }
> 622c636,639
> < if (!user) {
> ---
>> if (!user) { /* For clarity: it should be "if (!ci || !user) {" but the two expressions have
>> * exactly the same values under the above condition that
>> * (ci==NULL) ==> (user==NULL).
>> */
> 631a649,652
>> if (!user) { /* Same comment as above */
>> result = 0;
>> continue;
>> }
> 639c660
> < if (!host) {
> ---
>> if (!host) { /* Same comment on the condition as user
>> above */
> 648a670,673
>> if (!address) { /* Same comment on the condition as user above */
>> result = 0;
>> continue;
>> }
> 660a686,695
>> } else if (strcasecmp(attrib, "subsystem") == 0) {
>> if (!subsystem) {
>> result = 0;
>> continue;
>> }
>> if (match_pattern_list(subsystem, arg, len, 0) != 1)
>> result = 0;
>> else
>> debug("subsystem %.100s matched 'Subsystem %.100s' at "
>> "line %d", subsystem, arg, line);
> 666c701,702
> < if (user != NULL)
> ---
>>
>> if (ci != NULL)
> 667a704
>>
> 713,714c750
> < const char *filename, int linenum, int *activep, const char *user,
> < const char *host, const char *address)
> ---
>> const char *filename, int linenum, int *activep, ConnectionInfo
>> *conninfo)
> 745c781
> < if (user == NULL) {
> ---
>> if (conninfo == NULL) {
> 1316c1352,1354
> < value = match_cfg_line(&cp, linenum, user, host, address);
> ---
>>
>> value = match_cfg_line(&cp, linenum, conninfo);
>>
> 1454,1455c1492
> < parse_server_match_config(ServerOptions *options, const char *user,
> < const char *host, const char *address)
> ---
>> parse_server_match_config(ServerOptions *options, ConnectionInfo
>> *conninfo)
> 1460c1497
> < parse_server_config(&mo, "reprocess config", &cfg, user, host, address);
> ---
>> parse_server_config(&mo, "reprocess config", &cfg, conninfo);
> 1463a1501,1536
>> int
>> parse_server_match_testspec(ConnectionInfo *ci, char *spec) {
>> char *p;
>>
>> while ((p = strsep(&spec, ",")) && *p != '\0') {
>> if (strncmp(p, "addr=", 5) == 0)
>> ci->address = xstrdup(p + 5);
>> else if (strncmp(p, "host=", 5) == 0)
>> ci->host = xstrdup(p + 5);
>> else if (strncmp(p, "user=", 5) == 0)
>> ci->user = xstrdup(p + 5);
>> else if (strncmp(p, "subsystem=", 10) == 0)
>> ci->subsystem = xstrdup(p + 10);
>> else {
>> fprintf(stderr, "Invalid test mode specification %s\n", p);
>> return -1;
>> }
>> }
>> return 0;
>> }
>>
>> /*
>> * returns 1 for a complete spec, 0 for partial spec and -1 for an
>> * empty spec.
>> */
>> int
>> server_match_spec_complete(ConnectionInfo *ci) {
>> if (ci->user && ci->host && ci->address)
>> return 1; /* complete */
>> if (!ci->user && !ci->host && !ci->address)
>> return -1; /* empty */
>> return 0; /* partial */
>> }
>>
> 1537c1610
> < const char *user, const char *host, const char *address)
> ---
>> ConnectionInfo *conninfo)
> 1545c1618,1620
> < active = user ? 0 : 1;
> ---
>>
>> active = conninfo ? 0 : 1;
>>
> 1549c1624
> < linenum++, &active, user, host, address) != 0)
> ---
>> linenum++, &active, conninfo) != 0)
>
> ======================================================================
> ================= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.h
> src/servconf.h
> ======================================================================
> =================
> 170a171,178
>> /* Information about the incoming connection as used by Match */
>> typedef struct {
>> const char *user;
>> const char *host; /* possibly resolved hostname */
>> const char *address; /* remote address */
>> const char *subsystem; /* Subsystem service requested by the
>> remote client application */ } ConnectionInfo;
>>
> 186a195
>>
> 188c197,198
> < int *, const char *, const char *, const char *);
> ---
>> int *, ConnectionInfo *);
>>
> 191,193c201,207
> < const char *, const char *, const char *);
> < void parse_server_match_config(ServerOptions *, const char *, const char *,
> < const char *);
> ---
>> ConnectionInfo *);
>>
>> void parse_server_match_config(ServerOptions *, ConnectionInfo *);
>>
>> int parse_server_match_testspec(ConnectionInfo *, char *);
>> int server_match_spec_complete(ConnectionInfo *);
>>
>
> ======================================================================
> =============== diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/session.c
> src/session.c
> ======================================================================
> ===============
> 571d570
> <
> 825a825
>>
> 2089a2090
>> /* Searching for subsystem into the options repository */
> 2091,2105c2092,2120
> < if (strcmp(subsys, options.subsystem_name[i]) == 0) {
> < prog = options.subsystem_command[i];
> < cmd = options.subsystem_args[i];
> < if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
> < s->is_subsystem = SUBSYSTEM_INT_SFTP;
> < debug("subsystem: %s", prog);
> < } else {
> < if (stat(prog, &st) < 0)
> < debug("subsystem: cannot stat %s: %s",
> < prog, strerror(errno));
> < s->is_subsystem = SUBSYSTEM_EXT;
> < debug("subsystem: exec() %s", cmd);
> < }
> < success = do_exec(s, cmd) == 0;
> < break;
> ---
>> if (strcmp(subsys, options.subsystem_name[i]) == 0) break;
>> }
>>
>> if (i < options.num_subsystems) { /* subsystem found */
>> ConnectionInfo connection_info;
>>
>> connection_info.user = NULL;
>> connection_info.host = NULL;
>> connection_info.address = NULL;
>> connection_info.subsystem = subsys;
>>
>> logit("subsystem request: Parsing server match config options: user == '%s', host == '%s', address == '%s', subsystem == '%s'",
>> connection_info.user, connection_info.host,
>> connection_info.address, connection_info.subsystem);
>>
>> parse_server_match_config(&options, &connection_info);
>>
>> prog = options.subsystem_command[i];
>> cmd = options.subsystem_args[i];
>>
>> if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
>> s->is_subsystem = SUBSYSTEM_INT_SFTP;
>> debug("subsystem: %s", prog);
>> } else {
>> if (stat(prog, &st) < 0)
>> debug("subsystem: cannot stat %s: %s",
>> prog, strerror(errno));
>> s->is_subsystem = SUBSYSTEM_EXT;
>> debug("subsystem: exec() %s", cmd);
> 2106a2122,2123
>>
>> success = do_exec(s, cmd) == 0;
>
> ======================================================================
> ========= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.8
> src/sshd.8
> ======================================================================
> =========
> 111c111
> < that would apply to the specified user, host, and address will be
> set before
> ---
>> that would apply to the specified user, host, address, and subsystem
>> will be set before
> 116a117
>> .Dq addr ,
> 118,119c119,127
> < .Dq addr .
> < All are required and may be supplied in any order, either with
> multiple
> ---
>> .Dq subsystem .
>> Only
>> .Dq user ,
>> .Dq host ,
>> and
>> .Dq addr
>> keywords are required;
>> .Dq subsystem
>> is optional. These parameters may be supplied in any order, either
>> with multiple
>
> ======================================================================
> ========= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.c
> src/sshd.c
> ======================================================================
> =========
> 1323d1322
> < char *test_user = NULL, *test_host = NULL, *test_addr = NULL;
> 1325c1324
> < char *line, *p, *cp;
> ---
>> char *line;
> 1330a1330
>> ConnectionInfo connection_info;
> 1359a1360,1362
>> connection_info.address = connection_info.host = connection_info.subsystem =
>> connection_info.user = NULL;
>>
> 1452,1465c1455,1456
> < cp = optarg;
> < while ((p = strsep(&cp, ",")) && *p != '\0') {
> < if (strncmp(p, "addr=", 5) == 0)
> < test_addr = xstrdup(p + 5);
> < else if (strncmp(p, "host=", 5) == 0)
> < test_host = xstrdup(p + 5);
> < else if (strncmp(p, "user=", 5) == 0)
> < test_user = xstrdup(p + 5);
> < else {
> < fprintf(stderr, "Invalid test "
> < "mode specification %s\n", p);
> < exit(1);
> < }
> < }
> ---
>> if (parse_server_match_testspec(&connection_info, optarg) == -1)
>> exit(1);
> 1477c1468
> < "command-line", 0, NULL, NULL, NULL, NULL) != 0)
> ---
>> "command-line", 0, NULL, NULL) != 0)
> 1533,1540c1524,1529
> < if (test_flag >= 2 &&
> < (test_user != NULL || test_host != NULL || test_addr != NULL)
> < && (test_user == NULL || test_host == NULL || test_addr == NULL))
> < fatal("user, host and addr are all required when testing "
> < "Match configs");
> < if (test_flag < 2 && (test_user != NULL || test_host != NULL ||
> < test_addr != NULL))
> < fatal("Config test connection parameter (-C) provided without "
> ---
>> if ((test_flag >= 2) && (server_match_spec_complete(&connection_info) == 0))
>> fatal("user, host and addr are all required when testing "
>> "Match configs");
>>
>> if ((test_flag < 2) && (server_match_spec_complete(&connection_info) >= 0))
>> fatal("Config test connection parameter (-C) provided without "
> 1551c1540
> < &cfg, NULL, NULL, NULL);
> ---
>> &cfg, NULL);
> 1713,1715c1702,1703
> < if (test_user != NULL && test_addr != NULL && test_host != NULL)
> < parse_server_match_config(&options, test_user,
> < test_host, test_addr);
> ---
>> if (server_match_spec_complete(&connection_info) == 1)
>> parse_server_match_config(&options, &connection_info);
> 2005c1993
> < authenticated:
> ---
>> authenticated:
>
> ======================================================================
> ======================= diff -r
> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd_config.5
> src/sshd_config.5
> ======================================================================
> =======================
> 677a678
>> .Cm Address ,
> 679c680
> < .Cm Address .
> ---
>> .Cm Subsystem .
>
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev [at] mindrot
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


nicola.muto at cryptolab

May 21, 2012, 1:51 AM

Post #5 of 25 (1752 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Hi

Much more details about what john's saying can be found at this
mailing list thread "http://marc.info/?t=132698105300002&r=1&w=2".

Anyway to better understand the problem I would like to report what
Hans wrote in an internal document about the wanted functionality.

It should be allowed to run, at the same time, a SSH server on
port 22 With two "conflicting" requirements:

1) It should allow the user to run a very high level "common line
interface"
(CLI shell) session simply by logging in with SSH
2) It should allow the user to run a SFTP session that is chrooted such
that the
user can only see the files managed by a "file management" software
component
and not the rest of the filesystem.

These two requirements are currently in conflict with each other, since
the
OpenSSH implementation does not provide that feature out of the box.

Moreover, it was confirmed that the desired behavior cannot be achieved
using
existing functionality. This is also confirmed by the OpenSSH user
community.
Again, refer to the above OpenSSH mailing list thread.

\\nm



On 2012-05-21 07:12, John Olsson M wrote:
> Hi,
>
>> Is it a problem that scp and shell are un-chrooted?
>> Or are they handled elsewhere?
>
> I'm not sure what part you are referring to. What we need to be able
> to do is to place just the SFTP server in a chroot jail. But I'll
> give
> you some background so you can understand better what it is we want
> to
> do
>
> We have a box/node/machine/server/whatever that runs SLES as Nicola
> wrote. This server has two SSH servers running; one on port 22 and
> one
> on port 4422. When logging in to port 22 credentials are first
> verified against /etc/passwd and then an LDAP server is checked if
> /etc/passwd failed, for port 4422 it is only /etc/passwd that is
> checked and SFTP subsystem is disabled.
>
> If you just do a "normal" ssh login (without specifying any
> subsystem) you end up in what we call "COM CLI" which can be thought
> of as a text-based interface to an information model (you can
> navigate
> a tree structure, modify attributes and create objects, etc). This
> information model will also provide a branch showing a virtual
> filesystem. Thus this is a completely different animal compared to a
> Bash shell, it is purely a high level management interface for the
> server.
>
> Now when connecting to port 22 using an SFTP client, this client may
> only see the same virtual filesystem that is also visible via the
> information model. Thus the SFTP server must run in a chrooted
> environment, but SSH (COM CLI) may not since the COM CLI must be able
> to access the complete system.
>
> Connecting to port 4422 gives you a standard Bash shell.
>
>
> Did this answer your question, or did I answer some other question
> which you did not ask? :)
>
>
> /John
>
> -----Original Message-----
> From: Dan Kaminsky [mailto:dan [at] doxpara]
> Sent: den 17 maj 2012 16:55
> To: Nicola Muto
> Cc: <openssh-unix-dev [at] mindrot>; Nicola Muto; John Olsson M; Hans
> Insulander
> Subject: Re: New Subsystem criteria for Match option block in OpenSSH
> server
>
> Is it a problem that scp and shell are un-chrooted? Or are they
> handled elsewhere?
>
> Sent from my iPhone
>
> On May 17, 2012, at 7:19 AM, Nicola Muto <nicola.muto [at] cryptolab>
> wrote:
>
>> Hello everybody,
>>
>> I'm a C/C++ consultant working for Ericsson.
>>
>> I changed the OpenSSH-Portable code to add a new criteria into the
>> Match sshd_config option read by the sshd server.
>>
>> The new criteria is "Subsystem"; so a conditional block based on
>> subsystem client request can now be added to the sshd_config
>> configuration server file to override settings in its global
>> section.
>>
>> Ericsson requested this new Subsystem criteria because it needs to
>> "chroot" all the sftp session into a well defined directory for all
>> users logging into the server from an sftp client.
>>
>> For details on the Ericsson needs, please refer to the following
>> OpenSSH mailing list thread
>>
>> http://marc.info/?t=132698105300002&r=1&w=2
>>
>> A little example of the Match Subsystem usage follows
>>
>> Match Subsystem sftp
>> ChrootDirectory /path/to/sftp/data/jail/on/the/server
>> X11Forwarding no
>> AllowTcpForwarding no
>> ForceCommand internal-sftp
>>
>> To ensure that the Match Subsystem conditional block will work
>> properly, that is, the server "chroots" into the directory specified
>> by the ChrootDirectory directive above, you must also disable the
>> privilege separation using the config directive
>>
>> UsePrivilegeSeparation no
>>
>> into the sshd_config file. At least I think so; I checked that the
>> sshd server will skip calling the safely_chroot function if I keep
>> the
>> privilege separation enabled and login using a no-root user.
>>
>> I worked on the portable branch of OpenSSH because this feature must
>> be installed on a SLES (SuSE linux Enterprise Server) distribution.
>>
>> For the code changes I started from the openssh-6.0p1 released on
>> April 22, 2012, and I followed changes recently made by Darren
>> Tucker
>> to add other two criteria for the Match option; i.e. LocalAddress
>> and LocalPort.
>> So, my changes are minimized and will be gladly accepted by the
>> OpenSSH community. :-) Thank you Darren.
>>
>> I do not know who should accept and confirm my code changes. Also I
>> don't have an account to check-in the changes into the OpenBSD SVC
>> repository Hans can you help me?
>>
>> BRs
>> \\nm
>>
>> The diff code list follows
>>
>>
>> ======================================================================
>> ========= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/auth.c
>> src/auth.c
>>
>> ======================================================================
>> =========
>> 546a547
>>> ConnectionInfo connection_info;
>> 548,549c549,554
>> < parse_server_match_config(&options, user,
>> < get_canonical_hostname(options.use_dns),
>> get_remote_ipaddr());
>> ---
>>> connection_info.user = user;
>>> connection_info.host = get_canonical_hostname(options.use_dns);
>>> connection_info.address = get_remote_ipaddr();
>>> connection_info.subsystem = NULL;
>>>
>>> parse_server_match_config(&options, &connection_info);
>>
>>
>> ======================================================================
>> ================= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.c
>> src/servconf.c
>>
>> ======================================================================
>> =================
>> 601,602c601
>> < match_cfg_line(char **condition, int line, const char *user, const
>> char *host,
>> < const char *address)
>> ---
>>> match_cfg_line(char **condition, int line, ConnectionInfo *ci)
>> 606a606,614
>>> const char *user = NULL;
>>> const char *host = NULL;
>>> const char *address = NULL;
>>> const char *subsystem = NULL;
>>>
>>> /* Here the condition is hold
>>> * ci==NULL ==> user==NULL
>>> * that is, it is not possible that ci==NULL but user!=NULL
>>> */
>> 608c616
>> < if (user == NULL)
>> ---
>>> if (ci == NULL)
>> 610,611c618,624
>> < else
>> < debug3("checking match for '%s' user %s host %s addr %s",
>> cp,
>> ---
>>> else {
>>> user = ci->user;
>>> host = ci->host;
>>> address = ci->address;
>>> subsystem = ci->subsystem;
>>>
>>> debug3("checking match for '%s' user %s host %s addr %s
>>> subsystem %s", cp,
>> 613c626,627
>> < address ? address : "(null)");
>> ---
>>> address ? address : "(null)", subsystem ? subsystem :
>>> "(null)");
>>> }
>> 622c636,639
>> < if (!user) {
>> ---
>>> if (!user) { /* For clarity: it should be "if (!ci ||
>>> !user) {" but the two expressions have
>>> * exactly the same values
>>> under the above condition that
>>> * (ci==NULL) ==>
>>> (user==NULL).
>>> */
>> 631a649,652
>>> if (!user) { /* Same comment as above */
>>> result = 0;
>>> continue;
>>> }
>> 639c660
>> < if (!host) {
>> ---
>>> if (!host) { /* Same comment on the condition as user
>>> above */
>> 648a670,673
>>> if (!address) { /* Same comment on the condition as user
>>> above */
>>> result = 0;
>>> continue;
>>> }
>> 660a686,695
>>> } else if (strcasecmp(attrib, "subsystem") == 0) {
>>> if (!subsystem) {
>>> result = 0;
>>> continue;
>>> }
>>> if (match_pattern_list(subsystem, arg, len, 0) != 1)
>>> result = 0;
>>> else
>>> debug("subsystem %.100s matched 'Subsystem %.100s'
>>> at "
>>> "line %d", subsystem, arg, line);
>> 666c701,702
>> < if (user != NULL)
>> ---
>>>
>>> if (ci != NULL)
>> 667a704
>>>
>> 713,714c750
>> < const char *filename, int linenum, int *activep, const char
>> *user,
>> < const char *host, const char *address)
>> ---
>>> const char *filename, int linenum, int *activep, ConnectionInfo
>>> *conninfo)
>> 745c781
>> < if (user == NULL) {
>> ---
>>> if (conninfo == NULL) {
>> 1316c1352,1354
>> < value = match_cfg_line(&cp, linenum, user, host, address);
>> ---
>>>
>>> value = match_cfg_line(&cp, linenum, conninfo);
>>>
>> 1454,1455c1492
>> < parse_server_match_config(ServerOptions *options, const char
>> *user,
>> < const char *host, const char *address)
>> ---
>>> parse_server_match_config(ServerOptions *options, ConnectionInfo
>>> *conninfo)
>> 1460c1497
>> < parse_server_config(&mo, "reprocess config", &cfg, user, host,
>> address);
>> ---
>>> parse_server_config(&mo, "reprocess config", &cfg, conninfo);
>> 1463a1501,1536
>>> int
>>> parse_server_match_testspec(ConnectionInfo *ci, char *spec) {
>>> char *p;
>>>
>>> while ((p = strsep(&spec, ",")) && *p != '\0') {
>>> if (strncmp(p, "addr=", 5) == 0)
>>> ci->address = xstrdup(p + 5);
>>> else if (strncmp(p, "host=", 5) == 0)
>>> ci->host = xstrdup(p + 5);
>>> else if (strncmp(p, "user=", 5) == 0)
>>> ci->user = xstrdup(p + 5);
>>> else if (strncmp(p, "subsystem=", 10) == 0)
>>> ci->subsystem = xstrdup(p + 10);
>>> else {
>>> fprintf(stderr, "Invalid test mode specification %s\n",
>>> p);
>>> return -1;
>>> }
>>> }
>>> return 0;
>>> }
>>>
>>> /*
>>> * returns 1 for a complete spec, 0 for partial spec and -1 for an
>>> * empty spec.
>>> */
>>> int
>>> server_match_spec_complete(ConnectionInfo *ci) {
>>> if (ci->user && ci->host && ci->address)
>>> return 1; /* complete */
>>> if (!ci->user && !ci->host && !ci->address)
>>> return -1; /* empty */
>>> return 0; /* partial */
>>> }
>>>
>> 1537c1610
>> < const char *user, const char *host, const char *address)
>> ---
>>> ConnectionInfo *conninfo)
>> 1545c1618,1620
>> < active = user ? 0 : 1;
>> ---
>>>
>>> active = conninfo ? 0 : 1;
>>>
>> 1549c1624
>> < linenum++, &active, user, host, address) != 0)
>> ---
>>> linenum++, &active, conninfo) != 0)
>>
>>
>> ======================================================================
>> ================= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/servconf.h
>> src/servconf.h
>>
>> ======================================================================
>> =================
>> 170a171,178
>>> /* Information about the incoming connection as used by Match */
>>> typedef struct {
>>> const char *user;
>>> const char *host; /* possibly resolved hostname */
>>> const char *address; /* remote address */
>>> const char *subsystem; /* Subsystem service requested by the
>>> remote client application */ } ConnectionInfo;
>>>
>> 186a195
>>>
>> 188c197,198
>> < int *, const char *, const char *, const char *);
>> ---
>>> int *, ConnectionInfo *);
>>>
>> 191,193c201,207
>> < const char *, const char *, const char *);
>> < void parse_server_match_config(ServerOptions *, const char *,
>> const char *,
>> < const char *);
>> ---
>>> ConnectionInfo *);
>>>
>>> void parse_server_match_config(ServerOptions *, ConnectionInfo
>>> *);
>>>
>>> int parse_server_match_testspec(ConnectionInfo *, char *);
>>> int server_match_spec_complete(ConnectionInfo *);
>>>
>>
>>
>> ======================================================================
>> =============== diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/session.c
>> src/session.c
>>
>> ======================================================================
>> ===============
>> 571d570
>> <
>> 825a825
>>>
>> 2089a2090
>>> /* Searching for subsystem into the options repository */
>> 2091,2105c2092,2120
>> < if (strcmp(subsys, options.subsystem_name[i]) == 0) {
>> < prog = options.subsystem_command[i];
>> < cmd = options.subsystem_args[i];
>> < if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
>> < s->is_subsystem = SUBSYSTEM_INT_SFTP;
>> < debug("subsystem: %s", prog);
>> < } else {
>> < if (stat(prog, &st) < 0)
>> < debug("subsystem: cannot stat %s: %s",
>> < prog, strerror(errno));
>> < s->is_subsystem = SUBSYSTEM_EXT;
>> < debug("subsystem: exec() %s", cmd);
>> < }
>> < success = do_exec(s, cmd) == 0;
>> < break;
>> ---
>>> if (strcmp(subsys, options.subsystem_name[i]) == 0)
>>> break;
>>> }
>>>
>>> if (i < options.num_subsystems) { /* subsystem found */
>>> ConnectionInfo connection_info;
>>>
>>> connection_info.user = NULL;
>>> connection_info.host = NULL;
>>> connection_info.address = NULL;
>>> connection_info.subsystem = subsys;
>>>
>>> logit("subsystem request: Parsing server match config
>>> options: user == '%s', host == '%s', address == '%s', subsystem ==
>>> '%s'",
>>> connection_info.user, connection_info.host,
>>> connection_info.address, connection_info.subsystem);
>>>
>>> parse_server_match_config(&options, &connection_info);
>>>
>>> prog = options.subsystem_command[i];
>>> cmd = options.subsystem_args[i];
>>>
>>> if (strcmp(INTERNAL_SFTP_NAME, prog) == 0) {
>>> s->is_subsystem = SUBSYSTEM_INT_SFTP;
>>> debug("subsystem: %s", prog);
>>> } else {
>>> if (stat(prog, &st) < 0)
>>> debug("subsystem: cannot stat %s: %s",
>>> prog, strerror(errno));
>>> s->is_subsystem = SUBSYSTEM_EXT;
>>> debug("subsystem: exec() %s", cmd);
>> 2106a2122,2123
>>>
>>> success = do_exec(s, cmd) == 0;
>>
>>
>> ======================================================================
>> ========= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.8
>> src/sshd.8
>>
>> ======================================================================
>> =========
>> 111c111
>> < that would apply to the specified user, host, and address will be
>> set before
>> ---
>>> that would apply to the specified user, host, address, and
>>> subsystem
>>> will be set before
>> 116a117
>>> .Dq addr ,
>> 118,119c119,127
>> < .Dq addr .
>> < All are required and may be supplied in any order, either with
>> multiple
>> ---
>>> .Dq subsystem .
>>> Only
>>> .Dq user ,
>>> .Dq host ,
>>> and
>>> .Dq addr
>>> keywords are required;
>>> .Dq subsystem
>>> is optional. These parameters may be supplied in any order, either
>>> with multiple
>>
>>
>> ======================================================================
>> ========= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd.c
>> src/sshd.c
>>
>> ======================================================================
>> =========
>> 1323d1322
>> < char *test_user = NULL, *test_host = NULL, *test_addr = NULL;
>> 1325c1324
>> < char *line, *p, *cp;
>> ---
>>> char *line;
>> 1330a1330
>>> ConnectionInfo connection_info;
>> 1359a1360,1362
>>> connection_info.address = connection_info.host =
>>> connection_info.subsystem =
>>> connection_info.user = NULL;
>>>
>> 1452,1465c1455,1456
>> < cp = optarg;
>> < while ((p = strsep(&cp, ",")) && *p != '\0') {
>> < if (strncmp(p, "addr=", 5) == 0)
>> < test_addr = xstrdup(p + 5);
>> < else if (strncmp(p, "host=", 5) == 0)
>> < test_host = xstrdup(p + 5);
>> < else if (strncmp(p, "user=", 5) == 0)
>> < test_user = xstrdup(p + 5);
>> < else {
>> < fprintf(stderr, "Invalid test "
>> < "mode specification %s\n", p);
>> < exit(1);
>> < }
>> < }
>> ---
>>> if (parse_server_match_testspec(&connection_info,
>>> optarg) == -1)
>>> exit(1);
>> 1477c1468
>> < "command-line", 0, NULL, NULL, NULL, NULL) != 0)
>> ---
>>> "command-line", 0, NULL, NULL) != 0)
>> 1533,1540c1524,1529
>> < if (test_flag >= 2 &&
>> < (test_user != NULL || test_host != NULL || test_addr !=
>> NULL)
>> < && (test_user == NULL || test_host == NULL || test_addr ==
>> NULL))
>> < fatal("user, host and addr are all required when testing "
>> < "Match configs");
>> < if (test_flag < 2 && (test_user != NULL || test_host != NULL ||
>> < test_addr != NULL))
>> < fatal("Config test connection parameter (-C) provided
>> without "
>> ---
>>> if ((test_flag >= 2) &&
>>> (server_match_spec_complete(&connection_info) == 0))
>>> fatal("user, host and addr are all required when testing
>>> "
>>> "Match configs");
>>>
>>> if ((test_flag < 2) &&
>>> (server_match_spec_complete(&connection_info) >= 0))
>>> fatal("Config test connection parameter (-C) provided
>>> without "
>> 1551c1540
>> < &cfg, NULL, NULL, NULL);
>> ---
>>> &cfg, NULL);
>> 1713,1715c1702,1703
>> < if (test_user != NULL && test_addr != NULL && test_host !=
>> NULL)
>> < parse_server_match_config(&options, test_user,
>> < test_host, test_addr);
>> ---
>>> if (server_match_spec_complete(&connection_info) == 1)
>>> parse_server_match_config(&options, &connection_info);
>> 2005c1993
>> < authenticated:
>> ---
>>> authenticated:
>>
>>
>> ======================================================================
>> ======================= diff -r
>> /home/qnicmut/Projects/OpenSSH-Portable/openssh-6.0p1/sshd_config.5
>> src/sshd_config.5
>>
>> ======================================================================
>> =======================
>> 677a678
>>> .Cm Address ,
>> 679c680
>> < .Cm Address .
>> ---
>>> .Cm Subsystem .
>>
>> _______________________________________________
>> openssh-unix-dev mailing list
>> openssh-unix-dev [at] mindrot
>> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev [at] mindrot
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


nicola.muto at cryptolab

May 21, 2012, 7:15 AM

Post #6 of 25 (1754 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> The problem with that is that Match is done at connection
> establishment time and Subsystem is not a property of the connection,
> it's a request type that can be sent zero or more times during the
> life
> of the connection.

So, is it better not to use the ConnectionInfo structure to handle the
subsystem request?
If you like it, I can rearrange the code and remove the subsystem field
from the structure in the file servconf.h.


> What happens if I open a sftp subsytem then a normal
> shell session or vice versa?

All the sftp sessions should be chrooted and all shell session should
not,
in any order they are opened.


> that's usually a pretty good indication that you're doing something
> wrong.

I do not know well how the sshd server works internally and what
are its execution code flows, so I added a lot of traces to understand
better what's happening around the privilege separation concept.
These traces are in the file sshd-traces.txt attached to this email.
Well, with the privilege separation active the main process forks and
drops
privileges definitively before I can receive the subsystem request. So,
as you
can see at line 2.11 in the traces, when the client send the subsystem
request
the process has a no-root UID and it's too late to perform a 'chroot'
syscall.
Am I right?
I do not like, as you I think, to extend the time-window where the sshd
process
is running with root privilege. But I need some help on how to keep the
privilege
separation active and trigger a Match Subsystem option to "chroot" the
sshd process.


> I'd like to study your diff a bit more but it got mangled to the
> point
> that patch denies there's even a diff in there. Could you please
> resend
> (a) using diff -u (unified) format and (b) as an text/plain
> attachment.

Of course Darren. Please, see attachment part, now the diff should be
in
the format you requested.

\\nm



On 2012-05-18 07:25, Darren Tucker wrote:
> On Thu, May 17, 2012 at 04:19:36PM +0200, Nicola Muto wrote:
>> Hello everybody,
>>
>> I'm a C/C++ consultant working for Ericsson.
>>
>> I changed the OpenSSH-Portable code to add a new criteria
>> into the Match sshd_config option read by the sshd server.
>>
>> The new criteria is "Subsystem"; so a conditional block based
>
> The problem with that is that Match is done at connection
> establishment time and Subsystem is not a property of the connection,
> it's a request type that can be sent zero or more times during the
> life
> of the connection. What happens if I open a sftp subsytem then a
> normal
> shell session or vice versa?
>
>> you must also disable the privilege separation
>
> that's usually a pretty good indication that you're doing something
> wrong.
>
> I'd like to study your diff a bit more but it got mangled to the
> point
> that patch denies there's even a diff in there. Could you please
> resend
> (a) using diff -u (unified) format and (b) as an text/plain
> attachment.
>
> Thanks.
>
> --
> Darren Tucker (dtucker at zip.com.au)
> GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
> Good judgement comes with experience. Unfortunately, the
> experience
> usually comes from bad judgement.
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev [at] mindrot
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Attachments: sshd-traces.txt (17.6 KB)
  diff-out.txt (16.1 KB)


keisial at gmail

May 21, 2012, 10:06 AM

Post #7 of 25 (1753 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On 21/05/12 09:12, John Olsson M wrote:
> I'm not sure what part you are referring to. What we need to be able to do is to place just the SFTP server in a chroot jail. But I'll give you some background so you can understand better what it is we want to do
>
> We have a box/node/machine/server/whatever that runs SLES as Nicola wrote. This server has two SSH servers running; one on port 22 and one on port 4422. When logging in to port 22 credentials are first verified against /etc/passwd and then an LDAP server is checked if /etc/passwd failed, for port 4422 it is only /etc/passwd that is checked and SFTP subsystem is disabled.
>
> If you just do a "normal" ssh login (without specifying any subsystem) you end up in what we call "COM CLI" which can be thought of as a text-based interface to an information model (you can navigate a tree structure, modify attributes and create objects, etc). This information model will also provide a branch showing a virtual filesystem. Thus this is a completely different animal compared to a Bash shell, it is purely a high level management interface for the server.
>
> Now when connecting to port 22 using an SFTP client, this client may only see the same virtual filesystem that is also visible via the information model. Thus the SFTP server must run in a chrooted environment, but SSH (COM CLI) may not since the COM CLI must be able to access the complete system.
> Connecting to port 4422 gives you a standard Bash shell.
>
> Did this answer your question, or did I answer some other question which you did not ask? :)
>
> /John
Dirty workaround: the port 22 shell is is a setuid wrapper which exits
from the chroot, drops permissions and execs the "COM CLI".
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 22, 2012, 3:41 AM

Post #8 of 25 (1750 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> Dirty workaround: the port 22 shell is is a setuid wrapper
> which exits from the chroot, drops permissions and execs the
> "COM CLI".

That was indeed a very dirty solution. :)

Good to know about, but I would very much like to avoid it if possible.


/John
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


dtucker at zip

May 22, 2012, 4:23 AM

Post #9 of 25 (1766 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

OK, so the way this works is that it reparses the config again when it
does the subsystem lookup. I'm not convinced this is a good idea, and
there are a couple of problems with the existing implementation.

On Mon, May 21, 2012 at 04:15:16PM +0200, Nicola Muto wrote:
[...]
> + connection_info.user = NULL;
[...]
> + logit("subsystem request: Parsing server match config options: user == '%s', host == '%s', address == '%s', subsystem == '%s'",
> + connection_info.user, connection_info.host,
> + connection_info.address, connection_info.subsystem);

This will deref null pointers on most platforms. glibc might save you,
but most libcs won't.

> + parse_server_match_config(&options, &connection_info);

Because everything except the subsystem is nulled out a this point,
config lines like "Match User foo subsystem sftp" are syntactically
valid, but don't do what you'd think.

This reparsing could also change the server state in unexpected ways,
for example:

AllowTcpForwarding yes
Match Subsystem sftp
AllowTcpForwarding no

would allow port fowarding until you sent an sftp subsystem request.

> + prog = options.subsystem_command[i]

In order to understand it, I forward-ported the patch to HEAD and
removed all of the unrelated changes. (I haven't tested it, I'm only
including it in case saves someone the work).

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
Attachments: openssh-match-subsystem.patch (6.36 KB)


nicola.muto at cryptolab

May 22, 2012, 9:01 AM

Post #10 of 25 (1752 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> OK, so the way this works is that it reparses the config again when
> it
> does the subsystem lookup.

Yes.


> I'm not convinced this is a good idea, and
> there are a couple of problems with the existing implementation.

Why not? I'd like to understand what kind of problems you have with
the current implementation.
The Match Subsystem will be triggered when the client is sending a
subsystem request and this should be the last config file reparsing.


> [...]
>
> This will deref null pointers on most platforms. glibc might save
> you,
> but most libcs won't.

These are very minor problems and could be solved using the ?:
operator, as usual,
to avoid segmentation faults:

str_ptr ? str_ptr : "(null)"

or, using the GCC compact format

str_ptr ?: "(null)"

For that one could also write a CPP macro.
Personally, I wish that the C library implementations all behave the
same way, at
least in these small cases.


> Because everything except the subsystem is nulled out a this point,
> config lines like "Match User foo subsystem sftp" are syntactically
> valid, but don't do what you'd think.

Yes you are right. That issue was already in my mind.
Can it be solved defining a global instance of the ConnectionInfo
structure
into the sshd.c file and importing it as extern into the other files so
that
user, host, address, subsystem (and also lddress and lport) values can
be
always available?


> This reparsing could also change the server state in unexpected ways,
> for example:
>
> AllowTcpForwarding yes
> Match Subsystem sftp
> AllowTcpForwarding no
>
> would allow port fowarding until you sent an sftp subsystem request.

Sorry Darren, but that's exactly what I expect the ssh server should
do,
reading this config. So I know what I'm doing with this kind of
configuration.

\\nm



On 2012-05-22 13:23, Darren Tucker wrote:
> OK, so the way this works is that it reparses the config again when
> it
> does the subsystem lookup. I'm not convinced this is a good idea,
> and
> there are a couple of problems with the existing implementation.
>
> On Mon, May 21, 2012 at 04:15:16PM +0200, Nicola Muto wrote:
> [...]
>> + connection_info.user = NULL;
> [...]
>> + logit("subsystem request: Parsing server match config options:
>> user == '%s', host == '%s', address == '%s', subsystem == '%s'",
>> + connection_info.user, connection_info.host,
>> + connection_info.address, connection_info.subsystem);
>
> This will deref null pointers on most platforms. glibc might save
> you,
> but most libcs won't.
>
>> + parse_server_match_config(&options, &connection_info);
>
> Because everything except the subsystem is nulled out a this point,
> config lines like "Match User foo subsystem sftp" are syntactically
> valid, but don't do what you'd think.
>
> This reparsing could also change the server state in unexpected ways,
> for example:
>
> AllowTcpForwarding yes
> Match Subsystem sftp
> AllowTcpForwarding no
>
> would allow port fowarding until you sent an sftp subsystem request.
>
>> + prog = options.subsystem_command[i]
>
> In order to understand it, I forward-ported the patch to HEAD and
> removed all of the unrelated changes. (I haven't tested it, I'm only
> including it in case saves someone the work).
>
> --
> Darren Tucker (dtucker at zip.com.au)
> GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
> Good judgement comes with experience. Unfortunately, the
> experience
> usually comes from bad judgement.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


peter at stuge

May 22, 2012, 5:14 PM

Post #11 of 25 (1744 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Nicola Muto wrote:
>> This reparsing could also change the server state in unexpected ways,
>> for example:
>>
>> AllowTcpForwarding yes
>> Match Subsystem sftp
>> AllowTcpForwarding no
>>
>> would allow port fowarding until you sent an sftp subsystem request.
>
> Sorry Darren, but that's exactly what I expect the ssh server should
> do, reading this config. So I know what I'm doing with this kind of
> configuration.

The discussion has nothing to do with you or the needs of Ericsson.
OpenSSH behaving like the above would be absolutely retarded.


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 22, 2012, 8:37 PM

Post #12 of 25 (1752 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> The discussion has nothing to do with you or the needs of Ericsson.
> OpenSSH behaving like the above would be absolutely retarded.

I agree!


>> would allow port fowarding until you sent an sftp subsystem request.

My interpretation (as an end user) of the construct shown below is that AllowTcpForwarding is not allowed for the SFTP subsystem, that is if you connect to the server using the SFTP subsystem. All other connection requests for other subsystems would still allow port forwarding.

Now I do not now how much sense the above makes.

Perhaps one should define a list of configuration statements that acts as a nop (no operation) when a match agaginst subsystem is done?


/John
________________________________________
From: openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot [openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot] On Behalf Of Peter Stuge [peter [at] stuge]
Sent: Wednesday, May 23, 2012 02:14
To: openssh-unix-dev [at] mindrot
Subject: Re: New Subsystem criteria for Match option block in OpenSSH server

Nicola Muto wrote:
>> This reparsing could also change the server state in unexpected ways,
>> for example:
>>
>> AllowTcpForwarding yes
>> Match Subsystem sftp
>> AllowTcpForwarding no
>>
>> would allow port fowarding until you sent an sftp subsystem request.
>
> Sorry Darren, but that's exactly what I expect the ssh server should
> do, reading this config. So I know what I'm doing with this kind of
> configuration.

The discussion has nothing to do with you or the needs of Ericsson.
OpenSSH behaving like the above would be absolutely retarded.


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


dtucker at zip

May 22, 2012, 9:05 PM

Post #13 of 25 (1738 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On Wed, May 23, 2012 at 05:37:16AM +0200, John Olsson M wrote:
[...]
> My interpretation (as an end user) of the construct shown below is
> that AllowTcpForwarding is not allowed for the SFTP subsystem, that is if
> you connect to the server using the SFTP subsystem. All other connection
> requests for other subsystems would still allow port forwarding.
>
> Now I do not now how much sense the above makes.

It doesn't make sense. subsystem requests, port forwarding requests
(and shell session requests for that matter) are all different request
types that may be sent in a single SSH connection. There's no such
thing "connecting to the server using the sftp subsystem"; you connect
to the SSH server, then request zero or more of session, subsystem or
port forwarding requests an any order you please.

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


dtucker at zip

May 22, 2012, 10:54 PM

Post #14 of 25 (1744 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On Tue, May 22, 2012 at 06:01:17PM +0200, Nicola Muto wrote:
> Darren Tucker write:
> >I'm not convinced this is a good idea
>
> Why not? I'd like to understand what kind of problems you have with
> the current implementation.

I have two sets of problems with the patch. The lesser of the two were
the implementation things I mentioned in my previous email.

The larger and thornier set is whether or not this is a sensible thing
to do at all.
1) right now, the Match rules do not change the behaviour after the
username is supplied (very early in the logon process). The proposed
patch allows a whole bunch of things to change at run time in possibly
unanticipated ways.
2) it doesn't (and can't work) with ChrootDirectory+PrivilegeSeparation,
which negates a whole lot of the safety features in sshd.

#2 has obvious security implications, but #1 can too. For example: do
you allow the user to write to the chroot directory itself?

You shouldn't (and there's a reason why there's a check for this), but
if you do, having ChrootDirectory on the list makes the exploit easy:
in a single SSH session, open one shell session where you hardlink a
setuid binary into the chroot, then use sftp to upload, say, a backdoored
libc into the chroot (and thereby activating ChrootDirectory), then a
second shell session to execute the setuid binary which will load the
backdoored libc from the chroot and do something nasty.

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3ou do 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


nicola.muto at cryptolab

May 23, 2012, 1:11 AM

Post #15 of 25 (1744 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Sorry guys, there was a misunderstanding due to my wrong words.

Instead, what I wanted to say is that a system administrator that is
configuring the ssh server with the following config lines, for
example,

...
AllowTcpForwarding yes
...
Match Subsystem sftp
AllowTcpForwarding no

"should know what he is doing". That is, the AllowTcpForwarding option
put at
global level is active until a client sent an sftp subsystem request.
This is what comes out by reading the above configuration file; I
think.

Sorry again, I'm disappointed.

\\nm


On 2012-05-23 00:14, Peter Stuge wrote:
> Nicola Muto wrote:
>>> This reparsing could also change the server state in unexpected
>>> ways,
>>> for example:
>>>
>>> AllowTcpForwarding yes
>>> Match Subsystem sftp
>>> AllowTcpForwarding no
>>>
>>> would allow port fowarding until you sent an sftp subsystem
>>> request.
>>
>> Sorry Darren, but that's exactly what I expect the ssh server should
>> do, reading this config. So I know what I'm doing with this kind of
>> configuration.
>
> The discussion has nothing to do with you or the needs of Ericsson.
> OpenSSH behaving like the above would be absolutely retarded.
>
>
> //Peter
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev [at] mindrot
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


peter at stuge

May 23, 2012, 2:12 AM

Post #16 of 25 (1741 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Nicola Muto wrote:
> Sorry guys, there was a misunderstanding due to my wrong words.

Actually I think I understood.


> a system administrator that is configuring the ssh server with the
> following config lines, for example,
>
> ...
> AllowTcpForwarding yes
> ...
> Match Subsystem sftp
> AllowTcpForwarding no
>
> "should know what he is doing".

My point is that this is extremely unintuitive and if the admin also
knows roughly how the SSH protocol works then it is directly
confusing. I am strongly opposed to introducing this kind of
indeterministic and inconsequent behavior into any program, and
into sshd in particular.


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


nicola.muto at cryptolab

May 23, 2012, 6:31 AM

Post #17 of 25 (1725 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Ok Darren, you confirmed my doubts about adding Match-Subsystem
option to sshd, most of all for the ChrootDirectory+PrivilegeSeparation
problem.

Now I have a question. What's that sounds bad, the implementation of
the
patch or the Match-Subsystem idea itself?
In other words. Is it possible to solve all of these issues providing
another
implementation? Am I doing something wrong or forgetting something?

If so, a new implementation should be not so simple and have heavy
impacts
on the source. Moreover, I think that the Peter's issue can't be solved
by whatever implementation is proposed. Am I right?

If not then the Match-Subsystem solution itself sounds not a good idea.

Please, let me know what you think about. Thank you in advance.

> #2 has obvious security implications, but #1 can too. For example:
> do
> you allow the user to write to the chroot directory itself?

No user can write to the chroot directory.

\\nm



On 2012-05-23 05:54, Darren Tucker wrote:
> On Tue, May 22, 2012 at 06:01:17PM +0200, Nicola Muto wrote:
>> Darren Tucker write:
>> >I'm not convinced this is a good idea
>>
>> Why not? I'd like to understand what kind of problems you have with
>> the current implementation.
>
> I have two sets of problems with the patch. The lesser of the two
> were
> the implementation things I mentioned in my previous email.
>
> The larger and thornier set is whether or not this is a sensible
> thing
> to do at all.
> 1) right now, the Match rules do not change the behaviour after the
> username is supplied (very early in the logon process). The
> proposed
> patch allows a whole bunch of things to change at run time in
> possibly
> unanticipated ways.
> 2) it doesn't (and can't work) with
> ChrootDirectory+PrivilegeSeparation,
> which negates a whole lot of the safety features in sshd.
>
> #2 has obvious security implications, but #1 can too. For example:
> do
> you allow the user to write to the chroot directory itself?
>
> You shouldn't (and there's a reason why there's a check for this),
> but
> if you do, having ChrootDirectory on the list makes the exploit easy:
> in a single SSH session, open one shell session where you hardlink a
> setuid binary into the chroot, then use sftp to upload, say, a
> backdoored
> libc into the chroot (and thereby activating ChrootDirectory), then a
> second shell session to execute the setuid binary which will load the
> backdoored libc from the chroot and do something nasty.
>
> --
> Darren Tucker (dtucker at zip.com.au)
> GPG key 8FF4FA69 / D9A3ou do 86E9 7EEE AF4B B2D4 37C9 C982 80C7
> 8FF4 FA69
> Good judgement comes with experience. Unfortunately, the
> experience
> usually comes from bad judgement.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


keisial at gmail

May 23, 2012, 7:57 AM

Post #18 of 25 (1723 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On 23/05/12 15:31, Nicola Muto wrote:
> Ok Darren, you confirmed my doubts about adding Match-Subsystem
> option to sshd, most of all for the ChrootDirectory+PrivilegeSeparation
> problem.
>
> Now I have a question. What's that sounds bad, the implementation of the
> patch or the Match-Subsystem idea itself?
> In other words. Is it possible to solve all of these issues providing
> another
> implementation? Am I doing something wrong or forgetting something?
>
> If so, a new implementation should be not so simple and have heavy
> impacts
> on the source. Moreover, I think that the Peter's issue can't be solved
> by whatever implementation is proposed. Am I right?
>
> If not then the Match-Subsystem solution itself sounds not a good idea.
>
> Please, let me know what you think about. Thank you in advance.
After reading Peter and Darren replies, I agree with them that a
Subsystem match to set forwarding makes no sense.

OTOH, I think you should be setting them globally.

Where you requested this setup:
> Match Subsystem sftp
> ChrootDirectory /path/to/sftp/data/jail/on/the/server
> X11Forwarding no
> AllowTcpForwarding no
> ForceCommand internal-sftp


What you need is probably this:
> X11Forwarding no
> AllowTcpForwarding no
> Match Subsystem sftp
> ChrootDirectory /path/to/sftp/data/jail/on/the/server
> ForceCommand internal-sftp

Chroot and executed command *are* a property of the subsystem. It's a
bit weird to have different chroots, but that's what you require.
No special reason to use ForceCommand instead of "Subsystem sftp
internal-sftp", though.

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


mouring at eviladmin

May 23, 2012, 8:05 AM

Post #19 of 25 (1728 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On May 23, 2012, at 8:31 AM, Nicola Muto wrote:

> Ok Darren, you confirmed my doubts about adding Match-Subsystem
> option to sshd, most of all for the ChrootDirectory+PrivilegeSeparation
> problem.
>
> Now I have a question. What's that sounds bad, the implementation of the
> patch or the Match-Subsystem idea itself?
> In other words. Is it possible to solve all of these issues providing another
> implementation? Am I doing something wrong or forgetting something?
>
> If so, a new implementation should be not so simple and have heavy impacts
> on the source. Moreover, I think that the Peter's issue can't be solved
> by whatever implementation is proposed. Am I right?

I'm not sure any patch would be acceptable. The heart of the issue is simple:

Setup a ControlMaster, start a shell connection to the server, then start
a sftp connection reusing that control. If you set anything like "don't allow
forward, etc" now forward is disabled for the shell and for sftp. This isn't
logical behavior that someone would expect unless they understood the
ssh protocol.

So unless you limit the protocol so you can only setup a single session type
(subsystem, shell, etc) per connection then I can't see how this is a sane
feature for admins to wrap their heads around.

- Ben
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 23, 2012, 11:57 PM

Post #20 of 25 (1725 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

>> a system administrator that is configuring the ssh server
>> with the following config lines, for example,
>>
>> ...
>> AllowTcpForwarding yes
>> ...
>> Match Subsystem sftp
>> AllowTcpForwarding no
>>
>> "should know what he is doing".
>
> My point is that this is extremely unintuitive and if the
> admin also knows roughly how the SSH protocol works then
> it is directly confusing. I am strongly opposed to
> introducing this kind of indeterministic and inconsequent
> behavior into any program, and into sshd in particular.

Don't you have the same problem with fopr instance other match statements?

What happens if you have

...
AllowTcpForwarding yes
...
Match User foo
AllowTcpForwarding no

Wouldn't this cause the same behavior? That once user foo logs on TCP forwarding is denied? What is it that makes the subsystem so magical that it suddenly affects the whole server?


/John

-----Original Message-----
From: openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot [mailto:openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot] On Behalf Of Peter Stuge
Sent: den 23 maj 2012 11:12
To: openssh-unix-dev [at] mindrot
Subject: Re: New Subsystem criteria for Match option block in OpenSSH server

Nicola Muto wrote:
> Sorry guys, there was a misunderstanding due to my wrong words.

Actually I think I understood.


> a system administrator that is configuring the ssh server with the
> following config lines, for example,
>
> ...
> AllowTcpForwarding yes
> ...
> Match Subsystem sftp
> AllowTcpForwarding no
>
> "should know what he is doing".

My point is that this is extremely unintuitive and if the admin also knows roughly how the SSH protocol works then it is directly confusing. I am strongly opposed to introducing this kind of indeterministic and inconsequent behavior into any program, and into sshd in particular.


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 24, 2012, 12:02 AM

Post #21 of 25 (1733 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> #2 has obvious security implications, but #1 can too.
> For example: do you allow the user to write to the
> chroot directory itself?

If you mean chroot directory == "/" then root owns that directory ofcourse. You have the same problem as you describe below with the existing chroot directive, right?

If not, please explain why chroot:ing per subsystem creates a security hole.


> ChrootDirectory), then a second shell session to execute the setuid
> binary which will load the backdoored libc from the chroot and do something
> nasty.

It is here I get lost. The chroot is configured per subsystem. So if you have the following hypothetical construct in SSHD config

Match Subsystem SFTP
ChrootDirectory /foo/bar

You will only end up in the /foo/bar (as the new root) when you connect to the SFTP subsystem.

Thus you can't suddenly choose if you want to have a plain vanilla SSH to be chrooted or not. The idea was that the construct affects the whole subsystem, so how can you get a chroot:ed shell and do something nasty? Or what is it that I am missing? I want to understand. :)


/John

-----Original Message-----
From: Darren Tucker [mailto:dtucker [at] zip]
Sent: den 23 maj 2012 07:54
To: Nicola Muto
Cc: dtucker [at] zip; Nicola Muto; openssh-unix-dev [at] mindrot; John Olsson M; Hans Insulander
Subject: Re: New Subsystem criteria for Match option block in OpenSSH server

On Tue, May 22, 2012 at 06:01:17PM +0200, Nicola Muto wrote:
> Darren Tucker write:
> >I'm not convinced this is a good idea
>
> Why not? I'd like to understand what kind of problems you have with
> the current implementation.

I have two sets of problems with the patch. The lesser of the two were the implementation things I mentioned in my previous email.

The larger and thornier set is whether or not this is a sensible thing to do at all.
1) right now, the Match rules do not change the behaviour after the
username is supplied (very early in the logon process). The proposed
patch allows a whole bunch of things to change at run time in possibly
unanticipated ways.
2) it doesn't (and can't work) with ChrootDirectory+PrivilegeSeparation,
which negates a whole lot of the safety features in sshd.

#2 has obvious security implications, but #1 can too. For example: do you allow the user to write to the chroot directory itself?

You shouldn't (and there's a reason why there's a check for this), but if you do, having ChrootDirectory on the list makes the exploit easy:
in a single SSH session, open one shell session where you hardlink a setuid binary into the chroot, then use sftp to upload, say, a backdoored libc into the chroot (and thereby activating ChrootDirectory), then a second shell session to execute the setuid binary which will load the backdoored libc from the chroot and do something nasty.

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3ou do 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 24, 2012, 12:29 AM

Post #22 of 25 (1725 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

> Setup a ControlMaster, start a shell connection to the server,
> then start a sftp connection reusing that control. If you set
> anything like "don't allow forward, etc" now forward is disabled
> for the shell and for sftp. This isn't logical behavior that
> someone would expect unless they understood the ssh protocol.

I agree. :(

Would it be sane to add additional restrictions on which keywords can follow a "Match Subsystem" construct? I think so and it would be intuitive to do so as well.

I guess the following set of keywords should not be allowed when matching on subsystem

AllowAgentForwarding
AllowTcpForwarding
GatewayPorts
MaxSessions
PermitOpen
X11DisplayOffset
X11Forwarding
X11UseLocalHost

which still leavs the following keywords to be allowed

Banner
ChrootDirectory
ForceCommand
GSSAPIAuthentication
HostbasedAuthentication
KbdInteractiveAuthentication
KerberosAuthentication
KerberosUseKuserok
MaxAuthTries
PubkeyAuthentication
AuthorizedKeysCommand
AuthorizedKeysCommandRunAs
PasswordAuthentication
PermitEmptyPasswords
PermitRootLogin
RhostsRSAAuthentication
RSAAuthentication


Any more that are not sane to allow when matching on subsystem?


/John

-----Original Message-----
From: openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot [mailto:openssh-unix-dev-bounces+john.m.olsson=ericsson.com [at] mindrot] On Behalf Of Ben Lindstrom
Sent: den 23 maj 2012 17:06
To: Nicola Muto
Cc: openssh-unix-dev [at] mindrot openssh
Subject: Re: New Subsystem criteria for Match option block in OpenSSH server


On May 23, 2012, at 8:31 AM, Nicola Muto wrote:

> Ok Darren, you confirmed my doubts about adding Match-Subsystem option
> to sshd, most of all for the ChrootDirectory+PrivilegeSeparation
> problem.
>
> Now I have a question. What's that sounds bad, the implementation of
> the patch or the Match-Subsystem idea itself?
> In other words. Is it possible to solve all of these issues providing
> another implementation? Am I doing something wrong or forgetting something?
>
> If so, a new implementation should be not so simple and have heavy
> impacts on the source. Moreover, I think that the Peter's issue can't
> be solved by whatever implementation is proposed. Am I right?

I'm not sure any patch would be acceptable. The heart of the issue is simple:

Setup a ControlMaster, start a shell connection to the server, then start a sftp connection reusing that control. If you set anything like "don't allow
forward, etc" now forward is disabled for the shell and for sftp. This isn't
logical behavior that someone would expect unless they understood the ssh protocol.

So unless you limit the protocol so you can only setup a single session type (subsystem, shell, etc) per connection then I can't see how this is a sane feature for admins to wrap their heads around.

- Ben
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


peter at stuge

May 24, 2012, 1:13 AM

Post #23 of 25 (1731 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

John Olsson M wrote:
> Don't you have the same problem with fopr instance other match statements?

None of the currently supported ones.


> What happens if you have
>
> ...
> AllowTcpForwarding yes
> ...
> Match User foo
> AllowTcpForwarding no
>
> Wouldn't this cause the same behavior?

It doesn't. Please read the SSH architecture RFC 4251. It's short.

http://ftp.sunet.se/pub/Internet-documents/rfc/rfc4251.txt

TCP forwarding is implemented through an SSH channel. SSH channels
can only ever be created after authentication, so it makes sense to
be able to limit what type of channels are allowed given the
information available to sshd after authentication.


> That once user foo logs on TCP forwarding is denied?

No, TCP forwarding channels are denied in all foo's sessions.


> What is it that makes the subsystem so magical that it suddenly
> affects the whole server?

The subsystem is only one of any number of channels that may be
opened in a single SSH session. Please see RFC 4251.


John Olsson M wrote:
> Would it be sane to add additional restrictions on which keywords
> can follow a "Match Subsystem" construct? I think so and it would
> be intuitive to do so as well.

Sure, not all keywords make sense.


> I guess the following set of keywords should not be allowed when
> matching on subsystem

Matching on subsystem should only be able to change things affecting
that single subsystem. Since a subsystem per definition has no
definition; it is simply an authenticated and encrypted 8-bit-clean
bidirectional channel, it's not really possible for sshd to know what
the subsystem will do. Indeed there exists only one subsystem which
sshd even knows about, by default; sftp. For sftp it does indeed make
sense to consider a chroot, as well as the other options that
sftp-server accepts on the command line. See sftp-server(8).


> which still leavs the following keywords to be allowed
>
> Banner

The banner is no longer relevant at the time of subsystems.


> ChrootDirectory
> ForceCommand

These two are the only ones that possibly have meaning when matching
on subsystem.


> GSSAPIAuthentication
> HostbasedAuthentication
> KbdInteractiveAuthentication
> KerberosAuthentication
> KerberosUseKuserok
> MaxAuthTries
> PubkeyAuthentication
> AuthorizedKeysCommand
> AuthorizedKeysCommandRunAs
> PasswordAuthentication
> PermitEmptyPasswords
> PermitRootLogin
> RhostsRSAAuthentication
> RSAAuthentication

The above are all about authentication which is no longer relevant
at the time of subsystems. Please read RFC 4251 so that you see how
the SSH protocol actually works and by extension how you can find
solutions that will both work for you and benefit others. Thanks!


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


mouring at eviladmin

May 24, 2012, 6:22 AM

Post #24 of 25 (1712 views)
Permalink
Re: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

On May 24, 2012, at 1:57 AM, John Olsson M wrote:

[..]
> Don't you have the same problem with fopr instance other match statements?
>
> What happens if you have
>
> ...
> AllowTcpForwarding yes
> ...
> Match User foo
> AllowTcpForwarding no
>
> Wouldn't this cause the same behavior? That once user foo logs on TCP forwarding is denied? What is it that makes the subsystem so magical that it suddenly affects the whole server?
>

Simple answer no.

Because you can't forward TCP sessions pre-authentication. And once you have authorized yourself the logic is simple, "Everyone, but foo can forward TCP connections." Same is true for matching on addresses, groups, and hosts as all of those are static through the session and are known up front.

- Ben
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


john.m.olsson at ericsson

May 24, 2012, 6:46 AM

Post #25 of 25 (1713 views)
Permalink
RE: New Subsystem criteria for Match option block in OpenSSH server [In reply to]

Thank you very much Peter for taking your time to explain this in such detail instead of just refering to the relevant RFCs. :)

What you write is pretty much what I was eventually able to piece together based on the ongoing discussions and reading of the SSHD Config manual page. But nopw I do understnad much deeper the implications on what it is we request and I hope IU can give relevant comments and suggestions on a way forward. I would very much like this to be implemented and salvaged in a sane way for all stakeholders. :)


>> ChrootDirectory
>> ForceCommand
>
> These two are the only ones that possibly have meaning when
> matching on subsystem.

So if we for a minute assume that we restrict the number of keywords that are allowed to be used to the two ones above when matching on subsystem, would that be an acceptable way forward?

What implications would this have on the existing source code?

Is it feasible to do with a reasonable (for whatever "reasonable" is) amount of work?

Or is your message that we should start looking for Plan B?


/John
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev [at] mindrot
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

OpenSSH dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.