Run program - #627
Conversation
2f8e069 to
9fcca84
Compare
This should improve execution for non-root pppd invocation. Signed-off-by: Jaco Kroon <jaco@uls.co.za>
The default reverts to the marginally less strict, and could have some TOCTAU issues in the case where there are symlinks involved, and the containing folder has write for group/other. By default we don't enforce strict. For now. Signed-off-by: Jaco Kroon <jaco@uls.co.za>
Signed-off-by: Jaco Kroon <jaco@uls.co.za>
Signed-off-by: Jaco Kroon <jaco@uls.co.za>
|
@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
left a comment
paulusmack
left a comment
There was a problem hiding this comment.
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.
|
|
||
| if (sbuf.st_uid != 0) { | ||
| error("Can't safely use %v because it is not owned by root", path); | ||
| if (sbuf.st_uid != 0 && sbuf.st_uid == geteuid()) { |
There was a problem hiding this comment.
Shouldn't this be sbuf.st_uid != geteuid() ?
| This option will enforce stricter rules on scripts, specifically they must be | ||
| owned by root (or the effective id), and not be writable by either group or | ||
| other. The option is not privileged. The inverse nostrict\-script\-checks to | ||
| disable the checks however are. This switches from using execve() when |
There was a problem hiding this comment.
disable the checks however is.
| owned by root (or the effective id), and not be writable by either group or | ||
| other. The option is not privileged. The inverse nostrict\-script\-checks to | ||
| disable the checks however are. This switches from using execve() when | ||
| invocing scripts to using execveat() - which means we perform the relevant |
| */ | ||
| void | ||
| ppp_script_setenv(char *var, char *value, int iskey) | ||
| ppp_script_setenv(const char *var, const char *value, int iskey) |
There was a problem hiding this comment.
Roll this commit into the next one? Or do more constifying here rather than in the next one...
Sounds reasonable. |
2 participants
General updates around the run_program subsystem.