pppd: avoid TOCTOU in protected file access#592
Conversation
jkroonza
left a comment
jkroonza
left a comment
There was a problem hiding this comment.
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.
|
I've refactored |
LGA1150
force-pushed
the
fd
branch
2 times, most recently
from
6a29b48 to
faaf527
Compare
|
@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. |
|
@jkroonza The |
LGA1150
force-pushed
the
fd
branch
2 times, most recently
from
fafcfba to
49a02b2
Compare
The proper way to fix TOCTOU is to avoid multiple evaluations of a path. Eliminate the check-then-open antipattern when opening files. Instead, open files first and validate the resulting file descriptor, and run scripts through fexecve(). This fixes the TOCTOU window, and avoids overly strict directory ownership checks that could break existing configurations. Signed-off-by: Qingfang Deng <dqfext@gmail.com>
jkroonza
left a comment
jkroonza
left a comment
There was a problem hiding this comment.
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.
|
|
||
| err: | ||
| free(rpath); | ||
| return 0; |
There was a problem hiding this comment.
Side note: Since we no longer cleanup anything this goto can go away, but I'm not hard objecting to the idiom used here.
| 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 *); |
There was a problem hiding this comment.
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.
| */ | ||
| if (!ppp_check_access(path_upapfile, &filename, 0, 0)) | ||
| return UPAP_AUTHNAK; | ||
| filename = path_upapfile; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A subsequent clean-up patch could constify all path variables as long as parameters.
| check_access(f, filename); | ||
| int fd = fileno(f); | ||
|
|
||
| if (!ppp_check_access(fd, filename, 0)) { |
There was a problem hiding this comment.
Once check_access() goes away I think fileno() inline here is fine avoiding the extra variable.
2 participants
Note: this is an AI-assisted commit, which I have not tested.
The proper way to fix TOCTOU is to avoid multiple evaluations of a path. Eliminate the check-then-open antipattern when opening files. Instead, open files first and validate the resulting file descriptor, and run scripts through fexecve().
This fixes the TOCTOU window, and avoids overly strict directory ownership checks that could break existing configurations.
Related issue: #591