Stricter checks on scripts.

[jkroonza]

Perform symbolic link path resolution before performing checks (avoid symlink change attacks).

Ensure the path leading to the executable is root:* owned, and not writable by anyone else.

Ensure the executable itself is:

Owned by root.
Executable for root.
Not writable by group or other.

Doesn't look like we need to check FACLs (the bits for additional users are mixed into group permission bits according to my testing).

[paulusmack]

This looks pretty nice. I'd like to extend this to do similar checks (though without requiring execute permission) for options files from /etc/ppp/peers invoked using the call option, and the secrets files (pap-secrets, chap-secrets, etc.).

I wonder if we should be doing feature checks for realpath and fstatat in the configure script, and what we do if they are not available.

I'd word some of the error messages a little differently: instead of saying, for example, "Can't execute %s: Not root owned." I'd say something more explicit such as "Refusing to execute %s because it is not owned by root".

[jkroonza]

I suspect we can create a stub realpath() using readlink() repeatedly until we have something that's NOT a symbolic link.

fstatat is only used here to allow use of flags=AT_SYMLINK_NOFOLLOW, since we resolve the symlinks first, not having AT_SYMLINK_NOFOLLOW would constitute another ToCToU issue, however, that's probably the best we can do if we don't have fstatat available for use.

Regarding other checks, sure, I could implement those too. It does actually make sense to ensure that options and secrets files too are similarly restricted in terms of access, so perhaps just split this out into a separate function:

int ppp_check_access(const char* path, char** use_path, int executable) { ...}

So that's it becomes part of the ppp API too such that plugins can utilize it too?

Call with something like:

const char * ip_up_path = "/etc/ppp/ip-up";
char* real_ip_up_path;

...
if (!ppp_check_access(ip_up_path, &real_ip_up_path, 1)) {
   failure handling here;
}
... success path
free(real_ip_up_path);
}

Alternatively, the caller needs to supply a storage buffer for real_ip_up_path and provide that?

The required handling is then external to ppp_check_access?

In failure case we could rig use_path to point to a constant error string such that logging happens by the caller?

[paulusmack]

I suspect we can create a stub realpath() using readlink() repeatedly until we have something that's NOT a symbolic link.

I wasn't saying we had to have a feature check, I was just raising the question. It seems like realpath() has been in POSIX etc. since at least 2004, so we're probably OK to assume it's available. It's certainly in glibc and musl.

fstatat is only used here to allow use of flags=AT_SYMLINK_NOFOLLOW, since we resolve the symlinks first, not having AT_SYMLINK_NOFOLLOW would constitute another ToCToU issue, however, that's probably the best we can do if we don't have fstatat available for use.

Looks like fstatat() dates back to 2008, so it's probably OK to assume it too. Or we could use lstat, which goes back to at least 2004.

Regarding other checks, sure, I could implement those too. It does actually make sense to ensure that options and secrets files too are similarly restricted in terms of access, so perhaps just split this out into a separate function:

int ppp_check_access(const char* path, char** use_path, int executable) { ...}

So that's it becomes part of the ppp API too such that plugins can utilize it too?

Sounds great.

Call with something like:

const char * ip_up_path = "/etc/ppp/ip-up";
char* real_ip_up_path;

...
if (!ppp_check_access(ip_up_path, &real_ip_up_path, 1)) {
   failure handling here;
}
... success path
free(real_ip_up_path);
}

Alternatively, the caller needs to supply a storage buffer for real_ip_up_path and provide that?

Either seems fine.

The required handling is then external to ppp_check_access?

Sorry, what do you mean by "required handling"?

In failure case we could rig use_path to point to a constant error string such that logging happens by the caller?

Probably cleaner to have an extra parameter to pass back an error string, and set it to NULL if there is no error.

[jkroonza]

I suspect we can create a stub realpath() using readlink() repeatedly until we have something that's NOT a symbolic link.

I wasn't saying we had to have a feature check, I was just raising the question. It seems like realpath() has been in POSIX etc. since at least 2004, so we're probably OK to assume it's available. It's certainly in glibc and musl.

Ok, so just use it, and if there is a reason later add a featurecheck and provide a stub realpath() for the case where it's missing. Happy.

fstatat is only used here to allow use of flags=AT_SYMLINK_NOFOLLOW, since we resolve the symlinks first, not having AT_SYMLINK_NOFOLLOW would constitute another ToCToU issue, however, that's probably the best we can do if we don't have fstatat available for use.

Looks like fstatat() dates back to 2008, so it's probably OK to assume it too. Or we could use lstat, which goes back to at least 2004.

Same strategy then, or switch to lstat then ...

Regarding other checks, sure, I could implement those too. It does actually make sense to ensure that options and secrets files too are similarly restricted in terms of access, so perhaps just split this out into a separate function:

int ppp_check_access(const char* path, char** use_path, int executable) { ...}

So that's it becomes part of the ppp API too such that plugins can utilize it too?

Sounds great.

Call with something like:

const char * ip_up_path = "/etc/ppp/ip-up";
char* real_ip_up_path;

...
if (!ppp_check_access(ip_up_path, &real_ip_up_path, 1)) {
   failure handling here;
}
... success path
free(real_ip_up_path);
}

Alternatively, the caller needs to supply a storage buffer for real_ip_up_path and provide that?

Either seems fine.

I think a supplied buffer is safest.

The required handling is then external to ppp_check_access?

Sorry, what do you mean by "required handling"?

Using the file. Ie, actually executing the script. So the "API" only does the checks, and provides the correct filename to use, and then the caller needs to do the required to actually use (ie, read passwords or execute the script) from the file.

In failure case we could rig use_path to point to a constant error string such that logging happens by the caller?

Probably cleaner to have an extra parameter to pass back an error string, and set it to NULL if there is no error.

Agreed.

[Neustradamus]

@jkroonza: Thanks for your PR :)
There are a lot of needed changes to be perfect?

[jkroonza]

@jkroonza: Thanks for your PR :) There are a lot of needed changes to be perfect?

No, but I'm catching up with $$$ work after a short holiday :).

[Neustradamus]

@jkroonza: Have you looked for your good draft PR?

[jkroonza]

Wil revisit next week. I think this can be made ready for a 2.5.3 release.

[Neustradamus]

@jkroonza: I hope that you will have the time for this one ^^

[jkroonza]

really trying ...

@jkroonza: I hope that you will have the time for this one ^^

You and me both :).

[paulusmack]

@jkroonza do you mind if I take what you have done so far and rework/extend it?

[jkroonza]

@jkroonza do you mind if I take what you have done so far and rework/extend it?

I do not. I was planning for this next week, but end-of-year stuff keeps piling up at rates faster than I'm able to process it, so please, run with this. Happy to review for you if you're interested - that should be faster than writing the code.

[Neustradamus]

@jkroonza: Hope to have a complete solution for your good work ^^