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@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next