[cmaster-next] Snapcraft_v2 branch ready for merge into stable/2.0

[Martin Winter]

David,

On 15 Dec 2016, at 0:41, David Lamparter wrote:

> Hm.  Some bits aren't quite nice yet...
>
>
> +++ b/vtysh/vtysh.h
> +       case OPTION_CONFDIR:
> +         /*
> +          * Overwrite location for vtysh.conf
> +          */
> ...
>
> This is a hard no-go.  vtysh.conf contains authentication-related
> options which can be used together with setting vtysh SGID to 
> quaggavty.
> E.g. you can set it up like this:
>
> ./configure --with-pam
>
> -rwx--s--x. root   quaggavty  /usr/bin/vtysh
> srwxrwx---. quagga quaggavty  /var/run/quagga/zebra.vty
>
> /etc/pam.d/quagga:
> auth   sufficient   pam_group.so group=netadmins
> auth   sufficient   pam_group.so group=sysadmins
> auth   required     pam_deny.so
>
> Now users that are in the "netadmins" or "sysadmins" group can use 
> vtysh
> to access zebra config.
>
> BUT.
>
> vtysh.conf has a "username XXX no-password" option which disables PAM.
>
> And, with your change, the user can supply their own vtysh.conf.  With 
> a
> line "username myuser no-password".  Now they can access zebra without
> being in either of these groups...
>
> ... Congratulations, you created a security issue :)

Crap.

Any suggestion on how to get this done? Location is unknown at compile 
time.

Only thought I have is to only allow the override if run as root?
Any better idea?

> (Bottom line: the file location of vtysh.conf can *never* be a command
> line option if vtysh is installed SGID or SUID.  You could try 
> checking
> if it is SGID/SUID and ignore the option.)
>
> (Note: I even recently updated the documentation on this, see
> doc/vtysh.texi.  It says "No security guarantees are made for this
> configuration", but still that doesn't mean we should break it 
> further.)
>
>
> +++ b/lib/privs.c
> ...
> -  if (zprivs_state.zgid)
> +  /* change gid only if we changed uid - otherwise skip */
> +  if ((zprivs_state.zgid) && (zprivs_state.zsuid != 
> zprivs_state.zuid))
> ...
> -  if (zprivs_state.zgid)
> +  /* change gid only if we changed uid - otherwise skip */
> +  if ((zprivs_state.zgid) && (zprivs_state.zsuid != 
> zprivs_state.zuid))
>
> Both of these won't do; if someone starts a daemon with "-u root", it
> should still apply its group settings.  Need a gid != current gid 
> check.

Ack.

>
> +++ b/ospfd/ospf_main.c
> ...
> +      snprintf(pidfile_temp, sizeof(pidfile_temp), "%s/ospfd-%d.pid", 
> pid_file, instance );
> +      strncpy(pid_file, pidfile_temp, sizeof(pid_file));
>
> strncpy is the wrong function to call (should be strlcpy), but it
> shouldn't even be there - just snprintf to pid_file directly.

Ack.

> Also, normally, socket path options on other daemons use full paths as
> arguments, not directory paths.  Can we rename it to --vty_socket_dir?

No problem.

- Martin


> On Sun, Dec 11, 2016 at 04:29:08AM -0800, Martin Winter wrote:
>> Got all the required changes from Renato and have now a branch
>> with all the Snapcraft parts ready for merge.
>> This includes code to modify the main Quagga and (in the snapcraft
>> subdir) all the needed files to build a snap.
>>
>> Doc / Package files will need one more round to adjust mainly for the
>> name (once we settle on something)
>>
>> Branch is  snapcraft_v2
>>
>> Main changes:
>>
>> - Snap packages are only allowed to write into their own mounted
>> container and the
>>    filenames are not known until the package is installed. There are 
>> now
>> new —vty_socket
>>    cli options to specify the location for the vty socket instead of
>> using the compile-time
>>    path. (Plus —ctl_socket for the extra LDP socket and 
>> —config_dir
>> for vtysh)
>>
>> - Snap packages can’t even read files outside their directories.
>> Getting the homedir
>>    from the password file isn’t possible. Using now HOME env 
>> variable
>> and only fall back
>>    to passed file if it doesn’t exits
>>
>> - Snap packages can’t SETUID or SETGID. They always run under root.
>> There is now a check
>>    for UID and GID and the change only happens if it’s not already
>> running under the
>>    requested User/Group
>>
>> - Martin
>>
>> _______________________________________________
>> cmaster-next mailing list
>> cmaster-next at lists.nox.tf
>> https://lists.nox.tf/listinfo/cmaster-next