[Samba] Feedback request on a tentative proposal to enhance smb.conf symlink-related params

[Stilez]

Hsnks Jeremy.  I'm a bit puzzled, because from your reply this doesn't 
actually sound like it would be expensive or complex to do.

> It then does a realpath() and checks if it is underneath
> the share path= parameter, and if "wide links = no" and it
> is not, refuses any further processing of the pathname.

Currently it does one chdir() + realdir() to find where the client is 
actually requesting,  and then (it seems) a quick strcmp() or regex compare 
against the share path string to see if that's a parent.

 If the comparison is made by strcmp(substr()) type functionality then this 
adds one strcmp(substr()) per valid/invalid path in the share params, which 
is minimal processing unless a user specifies very many paths indeed (not 
very likely and if they do then that's down to them). If it's done by regex 
then it means prestoring the regex match string and replacing a pattern 
like "^dir1/" by "^(dir1|dir2|..)/" (and a second regex for invalids). It 
doesn't lead to a large multiplicity of regex, if regex is used. But 
whether strcmp or regex, both seem quite minor/trivial in a computational 
"expense" sense, and fit into the existing approach with almost no change 
to the existing codebase.

Pseudocode for validating a path using the strcmp() method:

 Inputs are the requested path and a presorted array containing all symlink 
param paths for the share (each entry contains a path + a boolean for 
whether it's from a valid/invalid symlinks param.)  Array is presorted to 
put invalid entries first, as they override valid ones.

chdir(requested_path);
strRealpath = realdir();
boolStatus=false;    /* default if no match */
for (i=0; i < count(pathlist); i++) {
  if (pathlist(i)('strPath') == 
substr(strRealpath,0,len(pathlist(i)('path')))) {
    /* we have a match. pathlist() entries can only be valid or invalid
         and invalid are presorted to the start of the list for efficiency
         when config initially read in.  So a match can only mean one of:
         <invalid>, or <valid having not matched any invalid>.
         Therefore the matched item itself gives us our answer. */
    boolStatus = pathlist(i)('valid');
       /* true if matched item came from a "valid symlinks" entry,
	          false if it came from an "invalid symlinks" entry */
    break;
  }
}    /* boolStatus now true/false indicates whether or not permissible */

Although not familiar with the codebase, from yiur description this might 
be all that's needed to validate on this schema. It doesn't seem to need 
more than this code snippet, and doesn't look likely to be expensive.

It is helpful to be able to specify a list of specific dirs that can/can't 
be followed to or links created to, and I'm not seeing expense involved. 
Are you sure it needs much more than this sort of thing?

Stilez




On 1 March 2018 7:15:39 pm Jeremy Allison <jra at samba.org> wrote:

> On Thu, Mar 01, 2018 at 10:48:02AM +0000, Stilez Stilezy wrote:
>> As mentioned in another thread, I notice that the params used to control
>> symlinks feel a bit inefficient and inelegant, and quite limited.  I think
>> there might be a good opportunity to simplify and also make their management in
>> Samba more powerful and adaptable to use-cases.  I'm guessing this list is a
>> good enough starting point to propose a smb.conf param change to this area and
>> see what reactions it gets.
>>  
>>  
>> The issue:
>>  
>> Currently symlink handling uses 3 parameters:
>>  
>>  - "follow symlinks" (are they followed at all?)
>>  - "wide links" (if so, can they be traversed to a location that isn't a child
>> of the share root dir? Effectively "read/traverse but not write" restriction)
>>  - "allow insecure links" (effectively "non-write restriction" removed, user
>> can write wide links to arbitrary dirs. as well as follow them)
>>  
>> Limitations/opacity caused by the current options:
>>  
>> 1. These three params have slightly opaque names ("wide links" doesn't say
>> *what* about wide links is/isn't altered by the option), and only certain
>> combinations make sense. Implication:  smb.conf could combine them to good
>> effect, simplifying the config, and also being clearer about how they relate to
>> each other.
>>  
>> 2. There is no granularity about where symlinks can link to. The only
>> granularity within Samba is to restrict read/traversal/create to either * a
>> descendant of /dir/share, or * a descendant of / (=universal access). There's
>> no other restriction setup possible within smb.conf. An implication is that
>> it's impossible to include a directive that restricts symlink use+creation to
>> specified locations outside the share root, other than by allowing them to have
>> scope over the entire of /. For example, a if the file system separates user
>> home and "generally available files", or has a virtual file system made up of
>> dynamically updated symlinks, the user can't create symlinks unless these are
>> all under the same share root, in which case the file system is forced to allow
>> symlinks to dirs. that might not be desirable. Alternatively if they aren't
>> under a common share root the user can't maintain symlinks as the only
>> directive available gives permission to create links to anywhere. It's very
>> crude.
>>  
>>  
>> Tentative ideas for discussion:
>>  
>> Replace the current symlink handling directives by directives modelled on valid
>> /invalid hosts+users:
>>
>>     valid symlink targets = follow:<mask>, <mask>, <mask> ... [create:] <mask>,
>>     <mask>, <mask>, ...
>>     invalid symlink targets = follow:<mask>, <mask>, <mask> ... [create:]
>>     <mask>, <mask>, <mask>, ...
>>      
>>     <mask> is built as usual from % substitutions,
>>     default for each share is   valid symlink targets = follow:%P, create:%P
>
> This would be too expensive to implement in the server
> and is too much complexity for very little gain.
>
> The reason is as follows. The server doesn't actually
> read symlinks component by component - what it does is
> find the last component in a client requested path, and
> chdir() into that.
>
> It then does a realpath() and checks if it is underneath
> the share path= parameter, and if "wide links = no" and it
> is not, refuses any further processing of the pathname.