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

[David Lamparter]

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 :)

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


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

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


-David

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