Run program

[jkroonza]

General updates around the run_program subsystem.

[jkroonza]

@paulusmack my testing on this is much improved. Last remaining challenge is to just set strict-script-checks in the case where the option is available, but to have pppd ignore it in older versions.

As per discussion, the hard-link rather than symlinks sorts my issues for ppp 2.5.3, and the PPP_SCRIPT_INSTANCE is actually quite convenient for the execveat() variation which I'd like to push for to become the default at some point.

This partially reverts both variations, in that we will check the relevant on the final destination of symlink chains (as per execveat()) but not all the symlinks nor full paths leading up to that point. This does leave TOCTAU issues open, but as per our long back-and-forth email discussion, solving that is in a sensible manner without major breakage is non-trivial.

One last commit I may want to tack on here with your blessing is to move the check_access read-check into ppp_check_access as well, and warn from there for the case where !exec, perhaps with a strict-secrets-perms option (and a relevant no negation), which where we will warn for now, possibly even error() but if strict-secrets-perms is set we go for fatal(), and eventually this too should (IMHO) become the default.

[paulusmack]

The logic error needs to be fixed, and preferably the man page update too. The comments about constifying in a separate commit or not you can take or leave.

[paulusmack]

One last commit I may want to tack on here with your blessing is to move the check_access read-check into ppp_check_access as well, and warn from there for the case where !exec, perhaps with a strict-secrets-perms option (and a relevant no negation), which where we will warn for now, possibly even error() but if strict-secrets-perms is set we go for fatal(), and eventually this too should (IMHO) become the default.

Sounds reasonable.

[jkroonza]

I note a few trailing white-space elimination changes slipped in somewhere. Let me know if this is an issue to be cleaned up.

[jkroonza]

Changes:

Rolled the const changes into the strict-script-option commit.

Swapped the PPP_SCRIPT_INSTANCE export and script-script-checks options around such that the documentation in pppd.8 is consistent at all times.

Added strict read checks for secrets file (and an option to switch this off).

[jkroonza]

Well, that's not a secrets file ... going enum route.

Warning - secret file /etc/ppp/peers/ulscore has world and/or group access

[jkroonza]

Sorry for the noise, was pushing to git to deploy for testing. Believe this should cover everything now. The last commit suddenly got big having the need to use an enum now rather than a simple boolean for the type of file. Basic testing pans out. Would appreciate another stupidity eyeball check prior to merge.

[paulusmack]

make check in the pppd directory is failing because some of the unit tests use utils.c and you added a reference to strict_secrets_files in ppp_check_access(). I suggest you add the following to your last commit:

diff --git a/pppd/utils.c b/pppd/utils.c
index 4c4a42b..ee26b37 100644
--- a/pppd/utils.c
+++ b/pppd/utils.c
@@ -93,6 +93,7 @@ ppp_explicit_bzero(void *buf, size_t len)
 #endif
 }
 
+#ifndef UNIT_TEST
 /*
  * Check that a file descriptor is owned by root (Or the effective user), not
  * writable by group or other.
@@ -148,6 +149,7 @@ ppp_check_access(int fd, const char *path, ppp_file_type_t filetype)
  err:
     return 0;
 }
+#endif
 
 /*
  * strlcpy - like strcpy/strncpy, doesn't overflow destination buffer,

[paulusmack]

Or if you don't like the ifdef, move ppp_check_access() to some suitable spot in main.c.

[jkroonza]

Or if you don't like the ifdef, move ppp_check_access() to some suitable spot in main.c.

I don't. But given how this variable really doesn't belong in options.c, how about moving the variable to utils.c?

[paulusmack]

I don't. But given how this variable really doesn't belong in options.c, how about moving the variable to utils.c?

That would work too.

[paulusmack]

I just noticed that the new options don't quite match: we have "strict-secrets-files" (with an 's' after 'secret') but "nostrict-secret-files" without the extra 's'.

[jkroonza]

I just noticed that the new options don't quite match: we have "strict-secrets-files" (with an 's' after 'secret') but "nostrict-secret-files" without the extra 's'.

Fixed. They now all have the s.

[jkroonza]

I don't. But given how this variable really doesn't belong in options.c, how about moving the variable to utils.c?

That would work too.

Done.

@paulusmack I trust this should be everything. I've not re-tested this round now as I believe the changes are rather elementary, make check now passes (on my laptop at least).

[paulusmack]

Found another problem - we may need a 4th value for file type...

[paulusmack]

Ah. I should have noticed this earlier, sorry. This one is meant to be owned by the user running pppd, not root. (It's the file containing your username and password for logging into your ISP, the argument of the +ua option, which is not privileged.)

[jkroonza]

What would you like to call that? PPP_FT_USERFILE?

Think these should probably start getting documented properly in the header file ...

[jkroonza]

I can't find a description of this option in the man page, nor any reference to it. The only place where I'm finding information on this option is in PLUGINS file (and the source code where it's implemented).

120 The pap_passwd_hook is called to determine what username and password
121 pppd should use in authenticating itself to the peer with PAP. The
122 user string will already be initialized, by the `user' option, the
123 `name' option, or from the hostname, but can be changed if necessary.
124 MAXNAMELEN bytes of space are available at *user, and MAXSECRETLEN
125 bytes of space at *passwd. If this hook returns 0, pppd will use the
126 values at *user and *passwd; if it returns -1, pppd will look in the
127 pap-secrets file, or use the value from the +ua or password option, as
128 it would normally.

That's the full extent of the documentation. I think it might be worthwhile to add appropriate details to the man page.

Another simple way might be to simply move the ppp_check_access prior to the seteuid(euid) call just above, but if the access check fails we will need to recover the setuid() call anyway, so perhaps something like:

ufile = fopen(fname, "r");
check_res = ufile && ppp_check_access(fileno(ufile), fname, PPP_FT_SECRET));

if (seteuid(euid) == -1)
   fatal(...);

...

if (!check_res) { /* instead of calling here */

[jkroonza]

index be975ed..f017d40 100644
--- a/pppd/auth.c
+++ b/pppd/auth.c
@@ -515,6 +515,7 @@ setupapfile(char **argv)
     uid_t euid;
     char u[MAXNAMELEN], p[MAXSECRETLEN];
     char *fname;
+    int check_res;
 
     lcp_allowoptions[0].neg_upap = 1;
 
@@ -529,6 +530,7 @@ setupapfile(char **argv)
        return 0;
     }
     ufile = fopen(fname, "r");
+    check_res = ufile && ppp_check_access(fileno(ufile), fname, PPP_FT_SECRET);
     if (seteuid(euid) == -1)
        fatal("unable to regain privileges: %m");
     if (ufile == NULL) {
@@ -536,7 +538,7 @@ setupapfile(char **argv)
         free(fname);
        return 0;
     }
-    if (!ppp_check_access(fileno(ufile), fname, PPP_FT_SECRET)) {
+    if (!check_res) {
        fclose(ufile);
        free(fname);
        return 0;

Would this work for you? Ownership will have to be root || user, but if root owned will have to be group/other readable and will thus be caught by strict secrets.

Incidentally, what's the difference between %v and %s in format strings, eg:

--- a/pppd/utils.c
+++ b/pppd/utils.c
@@ -139,10 +139,10 @@ ppp_check_access(int fd, const char *path, ppp_file_type_t filetype)
 
     if ((filetype == PPP_FT_SECRET) && (sbuf.st_mode & (S_IRWXG | S_IRWXO)) != 0) {
        if (strict_secrets_files) {
-           error("Warning - secret file %s has world and/or group access", path);
+           error("Can't use %v: secret file has world and/or group access", path);
            goto err;
        } else {
-           warn("Warning - secret file %s has world and/or group access", path);
+           warn("Warning - secret file %v has world and/or group access", path);
        }
     }
 

Which variation is correct?

[paulusmack]

The difference is that %v does a translation to printable form for control characters (except tab) and characters 0x80 and above, whereas %s doesn't. There's also %q which does a different translation to printable characters; the translation done by %q is reversible, whereas that done by %v isn't; that done by %q may change strings composed entirely of printable ASCII, whereas that done by %v doesn't. As an example of what %v does: if the string is the bytes 0x1b 0x5b 0xab it would come out as ^[[M-+.

I like to use %v if the string in question comes from the user or the peer, in case it's going somewhere where control characters could have some unwanted effect. If the string comes from a privileged option it's not really a concern (which would be the case here) though I still tend to lean towards using %v.

[paulusmack]

I can't find a description of this option in the man page, nor any reference to it. The only place where I'm finding information on this option is in PLUGINS file (and the source code where it's implemented).

I looked into the history of this. In the very earliest version I have, it didn't take an argument and just meant "do PAP". In a slightly later version, but still before I started working on it, it meant "do PAP and get the username and password from this file". In 1993 I checked a version of the man page into CVS which included the +ua option but described it as obsolescent, and in 1996 I took out the description from the man page with the CVS commit message "describe new options, don't describe obsolete ones". So maybe it's finally almost time to actually remove it. :) But I don't think I want to remove it for 2.5.4.

Another simple way might be to simply move the ppp_check_access prior to the seteuid(euid) call just above, but if the access check fails we will need to recover the setuid() call anyway, so perhaps something like:

ufile = fopen(fname, "r");
check_res = ufile && ppp_check_access(fileno(ufile), fname, PPP_FT_SECRET));

if (seteuid(euid) == -1)
   fatal(...);

...

if (!check_res) { /* instead of calling here */

Yes, that should work. Good idea.