New Subsystem criteria for Match option block in OpenSSH server

[Nicola Muto]

> 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.