run_parts updates #707
run_parts updates #707
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lib/run_part.c
Outdated
| @@ -80,7 +80,7 @@ int run_parts (const char *directory, const char *name, const char *action) | |||
| return (1); | |||
| } | |||
|
|
|||
| if (S_ISREG (sb.st_mode) || S_ISLNK (sb.st_mode)) { | |||
There was a problem hiding this comment.
LGTM. At least I couldn't make it evaluate to true with some tests. Even a broken link or a link pointing to itself won't trigger this, because stat(2) will just fail.
For patch 2:
Reviewed-by: Alejandro Colomar <[email protected]>
lib/run_part.c
Outdated
| if (scanlist<=0) { | ||
| (void) close (dfd); |
There was a problem hiding this comment.
Because we are not interested in potential failures from close(2), since the file descriptors are only read from, and some static analyzers flag unused return values from close(2).
There was a problem hiding this comment.
I prefer leaving this unsilenced.
Otherwise, one may change the code in a way that it should then check for the return value but forget to do so or remove the cast, and it would result in a silent bug.
For silencing static analyzers, I prefer to just pass flags to them to silence something.
| if (fd == -1) { | ||
| fprintf(shadow_logfd, "failed to open %s/%s: %s\n", | ||
| directory, namelist[n]->d_name, strerror(errno)); | ||
| for (int k = n; k < scanlist; k++) { |
There was a problem hiding this comment.
What does k mean?
There was a problem hiding this comment.
clang-tidy, which I use in my editor, complained about the loop iterator variable n being modified inside the loop, so I used a different one.
There was a problem hiding this comment.
I think it would be more idiomatic to use i, no? That's the usual loop counter. Or do you have a preference for k for this specific case?
Since stat(2) is used the result can never be a symbolic link.
Skip scripts that are either - not owned be the executing user or root, - not owned by the executing group or root, - world-writable.
Pin the script to execute before gathering its information to avoid any potential time-of-check-time-of-use issues.
| fprintf(shadow_logfd, "waitpid: %s\n", strerror(errno)); | ||
| fprintf(shadow_logfd, "failed to wait for pid %d (%s): %s\n", pid, script_name, strerror(errno)); |
There was a problem hiding this comment.
I'd also change this to a format "wait(...): %s\n" or something like that for consistency.
| @@ -51,47 +52,73 @@ int run_parts(const char *directory, const char *name, const char *action) | |||
| int n; | |||
| int execute_result = 0; | |||
|
|
|||
| int dfd = open(directory, O_PATH | O_DIRECTORY | O_CLOEXEC); | |||
| if (dfd == -1) { | |||
| fprintf(shadow_logfd, "failed open directory %s: %s\n", directory, strerror(errno)); | |||
There was a problem hiding this comment.
"open(%s): %s\n"?
| free(namelist[n]); | ||
| struct stat sb; | ||
|
|
||
| int fd = openat (dfd, namelist[n]->d_name, O_PATH | O_CLOEXEC); |
There was a problem hiding this comment.
This still has a spurious space.
| fprintf(shadow_logfd, "asprintf: %s\n", strerror(errno)); | ||
| for (; n<scanlist; n++) { | ||
| free(namelist[n]); | ||
| struct stat sb; |
There was a problem hiding this comment.
Please keep the 2 spaces before the variable name. That will keep the diff smaller, and also have it easier to distinguish where the type name ends and the variable name starts.
|
|
||
| int fd = openat (dfd, namelist[n]->d_name, O_PATH | O_CLOEXEC); | ||
| if (fd == -1) { | ||
| fprintf(shadow_logfd, "failed to open %s/%s: %s\n", |
There was a problem hiding this comment.
"openat(...): %s\n"?
| for (; n<scanlist; n++) { | ||
| free(namelist[n]); | ||
| if (fstat (fd, &sb) == -1) { | ||
| fprintf(shadow_logfd, "failed to stat %s/%s: %s\n", |
There was a problem hiding this comment.
"fstat(...): %s\n"?
| free(s); | ||
| for (; n<scanlist; n++) { | ||
| free(namelist[n]); | ||
| if (fstat (fd, &sb) == -1) { |
| "%s: did not exit cleanly.\n", | ||
| namelist[n]->d_name); | ||
| for (; n<scanlist; n++) { | ||
| free(namelist[n]); | ||
| "%s/%s: did not exit cleanly.\n", | ||
| directory, namelist[n]->d_name); | ||
| for (int k = n; k < scanlist; k++) { | ||
| free(namelist[k]); |
There was a problem hiding this comment.
I think this is a cosmetic change that would be better in a preparation patch, isn't it? It would make it easier to review.
use shadow_logfd instead of stdout
Similar to execution failure message.
run_part: drop void symlink check
Since stat(2) is used the result can never be a symbolic link.
skip scripts with insecure ownership/permission
Skip scripts that are either
use execveat(2) to avoid toctou issues
Pin the script to execute before gathering its information to avoid any potential time-of-check-time-of-use issues.