Inform the admin symlinks were skipped when adding paths#394
Inform the admin symlinks were skipped when adding paths#394rmetrich wants to merge 2 commits intolinux-application-whitelisting:mainfrom
Conversation
When adding path in the trust database, symbolic links are silently skipped, which makes it difficult for the admin to know that more paths must be added to the trust database. This patch adds information message that the symlink was skipped and warning message if the symlink is dangling or cannot be resolved for some reason. Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
src/cli/file-cli.c
Outdated
| else | ||
| msg(LOG_INFO, "Skipping symbolic link %s: " | ||
| "consider adding target %s/%s", | ||
| fpath, fpath, target); |
There was a problem hiding this comment.
$ ls -l /usr/local/bin/myscript
lrwxrwxrwx. 1 root root 11 Mar 13 15:20 /usr/local/bin/myscript -> ../myscript
$ sudo fapolicyd-cli --file add /usr/local/bin
03/13/26 15:30:12 [ INFO ]: Skipping symbolic link /usr/local/bin/myscript: consider adding target /usr/local/bin/myscript/../myscript
$ ls /usr/local/bin/myscript/../myscript
ls: cannot access '/usr/local/bin/myscript/../myscript': Not a directory
fapth is the full path, not a directory
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to inform administrators about skipped symbolic links when adding paths to the trust database. The approach is to handle FTW_SL in ftw_add_list_append.
My review found a critical issue: the new logic for handling symbolic links is currently unreachable because the nftw() call in file_append() does not use the FTW_PHYS flag. Without this flag, symbolic links are not reported as FTW_SL.
Additionally, there is an issue in how the suggested path for relative symbolic links is constructed, which could lead to incorrect and confusing paths being shown to the user. I've provided a suggestion to fix this using dirname().
Please address these points to ensure the feature works as intended.
src/cli/file-cli.c
Outdated
| else | ||
| msg(LOG_INFO, "Skipping symbolic link %s: " | ||
| "consider adding target %s/%s", | ||
| fpath, fpath, target); |
There was a problem hiding this comment.
The suggested path for relative symbolic links is constructed incorrectly, which can be confusing for the user. It uses the full path to the symlink file as the base for the relative target, which is incorrect. For example, if /path/to/link points to target, this will suggest adding /path/to/link/target instead of the correct /path/to/target.
You should use dirname() to get the directory part of the symlink's path to construct the correct target path. This will require including <libgen.h>.
else {
char *fpath_copy = strdup(fpath);
if (fpath_copy == NULL) {
msg(LOG_ERR, "Out of memory while processing %s", fpath);
break;
}
char *dir = dirname(fpath_copy);
msg(LOG_INFO, "Skipping symbolic link %s: "
"consider adding target %s/%s",
fpath, dir, target);
free(fpath_copy);
}|
Enabling FTW_PHYS to get symlink reports can't be done. That would enable descending into an attacker controlled loop where the symlinks forms a loop that traps naive tree walkers. We can't take that risk. |
src/cli/file-cli.c
Outdated
| msg(LOG_INFO, "Skipping non regular file: %s", fpath); | ||
| } | ||
| break; | ||
| case FTW_SL: |
There was a problem hiding this comment.
This block needs to be in { } in order to correctly set the scope of variables declared bellow.
case FTW_SL: {
// declarations and code
break;
}
See #394 (comment) |
When adding path in the trust database, symbolic links are silently skipped, which makes it difficult for the admin to know that more paths must be added to the trust database.
This patch adds information message that the symlink was skipped and warning message if the symlink is dangling or cannot be resolved for some reason.