pppd: relax and simplify permission check by LGA1150 · Pull Request #592 · ppp-project/ppp

LGA1150 · 2026-05-27T10:37:39Z

The additional check added in v2.5.3 is intended to mitigate symlink attacks, but it checks the permissions of every component of a path and is too strict, breaking existing configurations. Relax and simplify the check by only checking the opened file descriptor of a path, so the path is resolved only once.

Related issue: #591

jkroonza

Using FILE* where not needed. I don't like the approach, especially for the fexec side, will see if I can find cycles on this.

LGA1150 · 2026-05-28T06:54:25Z

I've refactored ppp_check_access and check_access. They now check the file descriptor instead of file name.

jkroonza · 2026-05-28T10:01:59Z

@LGA1150 I'm going to unsubscribe myself for the moment, please do @jkroonza me once you believe you're done.

Thank you for moving this in a better direction. I see you're still pushing quite a lot so please @jkroonza when you're looking for specific input or believe you're done. I don't have commit rights but I'll happily help review.

You'll note the original patch introduced a few extra char* variables to hold the absolute filename and to eliminate symlink attacks (which with your patch is no longer needed at all).

In order to simplify things long-term (not for the patch itself), you may want to eliminate those again as well. Eg, filename = path_upapfile. There should no longer be a reason to copy the pointer at all, just use path_upapfile as is, for example.

LGA1150 · 2026-05-29T01:30:04Z

@jkroonza The filename local variable was not introduced in said commit, so the simplification is unrelated to this pull request.

jkroonza

None of my comments are blockers @paulusmack I think this is good but can be improved - I'm happy to perform the cleanups in a follow-up commit which I think would be in order anyway to find and sort out a few cases which really aught to be const char* where char* is currently used.

jkroonza · 2026-05-29T07:52:33Z


 err:
-    free(rpath);
    return 0;


Side note: Since we no longer cleanup anything this goto can go away, but I'm not hard objecting to the idiom used here.

jkroonza · 2026-05-29T07:56:52Z

 static int  set_noauth_addr (char **);
 static int  set_permitted_number (char **);
-static void check_access (FILE *, char *);
+static void check_access (int, const char *);


I know this was still in place after previous patch set, but since this is now already checked by ppp_check_access I think this function can go away entirely. I think you modified this now based on my previous review noting that ppp_check_access was then taking a FILE*.

I think in order to not just fix the patch but actually make progress too regarding simplification, kill this function entirely too please unless I'm being stupid and it does actually check something which I'm missing, but as far as I can tell it doesn't actually enforce anything other than warning about the file being group/other writeable - something which is now enforced already by ppp_check_access.

Actually it warns about the file being group/other readable so that is different to ppp_check_access. But this function could easily go away, making a nice simplification, with an extra parameter to ppp_check_access telling it to check group/other read permissions.

jkroonza · 2026-05-29T08:01:43Z

     */
-    if (!ppp_check_access(path_upapfile, &filename, 0, 0))
-	    return UPAP_AUTHNAK;
+    filename = path_upapfile;


I get that this is a straight up revert, so if you don't mind, please fix in this commit - so either you can clean up now or I'll post a follow-up clean up patch where I can find this idiom.

Cases like this is not ideal either so a follow up commit trying to trace and fix all of them is possibly better:

2546 have_eaptls_secret_client() 2550 char *filename; 2567 filename = PPP_PATH_EAPTLSCLIFILE;

Plainly we're assigning const char* there to char* which can lead to hard to troubleshoot issues.

A subsequent clean-up patch could constify all path variables as long as parameters.

jkroonza · 2026-05-29T08:13:32Z

-	check_access(f, filename);
+	int fd = fileno(f);
+
+	if (!ppp_check_access(fd, filename, 0)) {


Once check_access() goes away I think fileno() inline here is fine avoiding the extra variable.

paulusmack · 2026-06-02T01:32:05Z

     * and couldn't have been modified by a non-root process.
     */
-    if (!ppp_check_access(prog, &rpath, must_exist, 1))
+    fd = open(prog, O_RDONLY);


O_RDONLY is not really correct here, since the file could have execute permission but not read permission. But since we have euid = 0, we can probably open it anyway. May be worth a comment.

According to fexecve manpage:

The file descriptor fd must be opened read-only (O_RDONLY) or with the O_PATH flag and the caller must have permission to execute the file that it refers to.

However O_PATH isn't portable so we have to use O_RDONLY here.

I just read fexecve manpage of Solaris, and it mentioned the O_EXEC flag:

The file descriptor passed to the fexecve() function need not have been opened with the O_EXEC flag. However, if the file to be executed denies read and write permission for the process preparing to perform the exec, the only way to provide the file descriptor fd to fexecve() is to specify the O_EXEC flag when opening fd.

So maybe we need an #ifdef here.

I think this will do:

Suggested change

fd = open(prog, O_RDONLY);

#define _GNU_SOURCE 1 // may be set in configure.ac

#if defined(O_PATH)

int flags = O_PATH;

#elif defined(O_EXEC)

int flags = O_EXEC;

#else

int flags = O_RDONLY; // require read permission on Linux version < 2.6.39

#endif

fd = open(prog, flags);

paulusmack · 2026-06-02T01:32:44Z

+	char fdpath[32];
+
+	snprintf(fdpath, sizeof(fdpath), "/dev/fd/%d", fd);
+	execve(fdpath, args, script_env);


I'm not sure this trick would work on Solaris. But fortunately Solaris has fexecve().

paulusmack · 2026-06-02T02:12:05Z

As discussed in issue #591, the premise of the commit message is wrong, in that there is no TOCTOU issue. The original series was not about fixing any TOCTOU bug, it was about trying to detect certain potentially dangerous misconfigurations of the filesystem. So the commit message at least would need to be reworded.

In fact this patch is about weakening the checks introduced in the earlier series so that only the file itself is checked, not the path leading to it, because of concerns about breaking existing setups. If that is what we decide to do, then the code changes seem mostly OK, except that it doesn't update the man page (pppd.8). It could be done a bit more cleanly in that we could avoid having the various functions in auth.c that read secrets files each calling both check_access() and ppp_check_access() --- either the call to ppp_check_access() could be put inside check_access(), or we could eliminate check_access() entirely and have ppp_check_access() optionally check group/other read permissions.

The additional check added in v2.5.3 is intended to mitigate symlink attacks, but it checks the permissions of every component of a path and is too strict, breaking existing configurations. Relax and simplify the check by only checking the opened file descriptor of a path, so the path is resolved only once. Signed-off-by: Qingfang Deng <dqfext@gmail.com>

jkroonza suggested changes May 27, 2026

View reviewed changes

LGA1150 force-pushed the fd branch from 2fbc3f0 to c541724 Compare May 28, 2026 06:52

LGA1150 force-pushed the fd branch 2 times, most recently from 6a29b48 to faaf527 Compare May 28, 2026 09:55

LGA1150 requested a review from jkroonza May 29, 2026 01:30

LGA1150 force-pushed the fd branch 3 times, most recently from 49a02b2 to 43ac446 Compare May 29, 2026 01:55

jkroonza approved these changes May 29, 2026

View reviewed changes

jkroonza mentioned this pull request Jun 1, 2026

Regression: ppp_check_access() may break existing configurations #591

Open

paulusmack reviewed Jun 2, 2026

View reviewed changes

LGA1150 force-pushed the fd branch from 43ac446 to 71d3d3a Compare June 12, 2026 02:56

LGA1150 changed the title ~~pppd: avoid TOCTOU in protected file access~~ pppd: relax and simplify permission check Jun 12, 2026

LGA1150 force-pushed the fd branch from 71d3d3a to 6a4944f Compare June 17, 2026 02:12

LGA1150 requested a review from jkroonza June 17, 2026 02:13

-    fd = open(prog, O_RDONLY);
+#define _GNU_SOURCE 1 // may be set in configure.ac
+#if defined(O_PATH)
+    int flags = O_PATH;
+#elif defined(O_EXEC)
+    int flags = O_EXEC;
+#else
+    int flags = O_RDONLY; // require read permission on Linux version < 2.6.39
+#endif
+    fd = open(prog, flags);

Uh oh!

Conversation

LGA1150 commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkroonza left a comment

Choose a reason for hiding this comment

Uh oh!

LGA1150 commented May 28, 2026

Uh oh!

jkroonza commented May 28, 2026

Uh oh!

LGA1150 commented May 29, 2026

Uh oh!

jkroonza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulusmack commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

LGA1150 commented May 27, 2026 •

edited

Loading