Skip to content

Add option to ignore symlinks completely #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blenk92
Copy link
Contributor

@blenk92 blenk92 commented Jul 16, 2021

When analyzing a complete rootfs (which might not be the rootfs of the
analyzing system) symlink within that rootfs might be broken. In
particular absolute symlinks. However, if by chance such a symlink
currently points to a valid binary in your system, this binary pointed
to is analyzed. This commit adds the possibility to ignore symlinks to
files (symlinks to dirs are already ignored by default). This allows to
solve the issue described above, and if the whole rootfs is analyzed
there shouldn't be a loss of information (because all the binaries will
be analyzed anyway). Additionally, this also saves some time when
performing the analysis.

@blenk92 blenk92 changed the title Handle symlinks Add option to ignore symlinks completely Jul 16, 2021
@blenk92 blenk92 force-pushed the handle_symlinks branch 2 times, most recently from 16583e3 to 8748187 Compare July 16, 2021 09:13
for path in filepath_list:
if path.is_dir() and not path.is_symlink():
if recursive:
for f in os.scandir(path):
yield from walk_filepath_list([Path(f)], recursive)
yield from walk_filepath_list([Path(f)], recursive, ignore_symlinks)
else:
yield from (Path(f) for f in os.scandir(path))
Copy link
Owner

@Wenzel Wenzel Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this line will yield any path, and not filter on the symlink as expected.

for example, if you have some symlinks in /usr/lib (/usr/lib/cpp -> /etc/alternatives/cpp),
running checksec.py -i /usr/lib will still yield /usr/lib/cpp because it goes through that yield from

Can you find a way to rework that function ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fix, I duplicated the condition:

def walk_filepath_list(filepath_list: List[Path], recursive: bool = False, ignore_symlinks: bool = False) -> Iterator[Path]:
    for path in filepath_list:
        if path.is_dir() and not path.is_symlink():
            if recursive:
                for f in os.scandir(path):
                    yield from walk_filepath_list([Path(f)], recursive, ignore_symlinks)
            else:
                yield from (Path(f) for f in os.scandir(path) if f.is_file()
                        and (not ignore_symlinks or not f.is_symlink()))
        elif path.is_file() and (not ignore_symlinks or not path.is_symlink()):
            yield path

but i don't like duplication 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sure i can try to come up with something ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hope switching to os.walk() is fine for you, seemed a bit simpler to me ;-)

@blenk92 blenk92 force-pushed the handle_symlinks branch 3 times, most recently from 3e544f8 to 0be298b Compare July 30, 2021 10:00
When analyzing a complete rootfs (which might not be the rootfs of the
analyzing system) symlink within that rootfs might be broken. In
particular absolute symlinks. However, if by chance such a symlink
currently points to a valid binary in your system, this binary pointed
to is analyzed. This commit adds the possibility to ignore symlinks to
files (symlinks to dirs are already ignored by default). This allows to
solve the issue described above, and if the whole rootfs is analyzed
there shouldn't be a loss of information (because all the binaries will
be analyzed anyway). Additionally, this also saves some time when
performing the analysis.

This commit also involves some refactoring of the walk_filepath_list()
function.
@blenk92 blenk92 requested a review from Wenzel August 3, 2021 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants