3426 linux chrome credentials collector #3665
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilija-lazoroski
changed the title
mssalvatore
reviewed
Sep 7, 2023
There was a problem hiding this comment.
If we copied this from somewhere, make sure we include the original author, source repository link, and license information at the top.
mssalvatore
reviewed
Sep 7, 2023
monkey/agent_plugins/credentials_collectors/chrome/src/linux_credentials_database_selector.py
Outdated
Show resolved
Hide resolved
mssalvatore
reviewed
Sep 7, 2023
monkey/agent_plugins/credentials_collectors/chrome/src/linux_credentials_database_selector.py
Outdated
Show resolved
Hide resolved
mssalvatore
reviewed
Sep 7, 2023
cakekoa
force-pushed
the
3426-linux-chrom-credentials-collector
branch
2 times, most recently
from
b195cdc to
f978052
Compare
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
2 times, most recently
from
1f6c3e3 to
238959f
Compare
ilija-lazoroski
changed the base branch from
develop
to
3426-windows-credentials-database-selector
cakekoa
force-pushed
the
3426-windows-credentials-database-selector
branch
2 times, most recently
from
a817ceb to
d6555c1
Compare
cakekoa
force-pushed
the
3426-linux-chrom-credentials-collector
branch
from
17da9d8 to
eb80534
Compare
shreyamalviya
requested changes
Sep 12, 2023
Comment on lines
20
to
26
| ("Brave", ".config/BraveSoftware/Brave-Browser"), | ||
| ("SlimJet", ".config/slimjet"), | ||
| ("Dissenter Browser", ".config/GabAI/Dissenter-Browser"), | ||
| ("Vivaldi", ".config/vivaldi"), | ||
| ("Microsoft Edge (Dev)", ".config/microsoft-edge-dev"), | ||
| ("Microsoft Edge (Beta)", ".config/microsoft-edge-beta"), | ||
| ("Microsoft Edge", ".config/microsoft-edge"), |
There was a problem hiding this comment.
Remove all of these. We're only supporting Chrome and Chromium on Linux.
monkey/agent_plugins/credentials_collectors/chrome/src/linux_credentials_database_selector.py
Show resolved
Hide resolved
monkey/agent_plugins/credentials_collectors/chrome/src/linux_credentials_database_selector.py
Show resolved
Hide resolved
| for browser_name, browser_path in CHROMIUM_BASED_DB_PATHS: | ||
| browser_db_path = home_dir_path / browser_path | ||
| try: | ||
| if self._directory_is_accessible(browser_db_path): |
There was a problem hiding this comment.
Should be the processor's responsibility
Comment on lines
43
to
60
| try: | ||
| sub_profile_dirs = profile_dir_path.iterdir() | ||
| for subdir in sub_profile_dirs: | ||
| if subdir == (profile_dir_path / LOGIN_DATABASE_FILENAME): | ||
| if self._file_is_accessible(subdir): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=subdir, master_key=DEFAULT_MASTER_KEY | ||
| ) | ||
| ) | ||
|
|
||
| login_database_path = profile_dir_path / subdir / LOGIN_DATABASE_FILENAME | ||
| if self._file_is_accessible(login_database_path): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=login_database_path, master_key=DEFAULT_MASTER_KEY | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Flake8 doesn't complain about complexity here?
| ) | ||
|
|
||
| login_database_path = profile_dir_path / subdir / LOGIN_DATABASE_FILENAME | ||
| if self._file_is_accessible(login_database_path): |
There was a problem hiding this comment.
Should be the processor's responsibility
Comment on lines
44
to
60
| sub_profile_dirs = profile_dir_path.iterdir() | ||
| for subdir in sub_profile_dirs: | ||
| if subdir == (profile_dir_path / LOGIN_DATABASE_FILENAME): | ||
| if self._file_is_accessible(subdir): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=subdir, master_key=DEFAULT_MASTER_KEY | ||
| ) | ||
| ) | ||
|
|
||
| login_database_path = profile_dir_path / subdir / LOGIN_DATABASE_FILENAME | ||
| if self._file_is_accessible(login_database_path): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=login_database_path, master_key=DEFAULT_MASTER_KEY | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This seems way more complex than it needs to be.
Just do:
sub_profile_dirs = profile_dir_path.iterdir()
if (profile_dir_path / LOGIN_DATABASE_FILENAME).exists():
login_data_paths.add(...)
for subdir in sub_profile_dirs:
login_database_path = profile_dir_path / subdir / LOGIN_DATABASE_FILENAME
login_data_paths.add(...)| def __call__(self) -> Collection[BrowserCredentialsDatabasePath]: | ||
| return [] | ||
| database_paths: Set[BrowserCredentialsDatabasePath] = set() | ||
| for profile_dir in self._get_browser_database_paths(): |
There was a problem hiding this comment.
The paths being returned from self._get_browser_database_paths() are not profile directory paths. They're browser directory paths for each user on the system. The name profile_dir makes it seem like you're talking about the profiles in the browser.
| ) | ||
| ) | ||
|
|
||
| login_database_path = profile_dir_path / subdir / LOGIN_DATABASE_FILENAME |
There was a problem hiding this comment.
Are all the files inside profile_dir_path only for the browser's user profiles? If no, this could add files to login_data_paths that aren't related to user profiles in any way.
Can't you just check the name of the directories and then add them to the set like we do for Windows? In Windows, we check for directories that start with "Profile".
shreyamalviya
force-pushed
the
3426-windows-credentials-database-selector
branch
from
d6555c1 to
87662ba
Compare
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
from
e25e9c8 to
12d09c3
Compare
shreyamalviya
force-pushed
the
3426-windows-credentials-database-selector
branch
from
87662ba to
9f745a6
Compare
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
from
92694a6 to
b3e8df5
Compare
mssalvatore
requested changes
Sep 12, 2023
Comment on lines
62
to
63
| if home_path not in home_dirs.values(): | ||
| home_dirs[os.getlogin()] = home_path |
There was a problem hiding this comment.
Is this check necessary?
Comment on lines
60
to
63
| if "HOME" in os.environ: | ||
| home_path = Path(os.environ["HOME"]) | ||
| if home_path not in home_dirs.values(): | ||
| home_dirs[os.getlogin()] = home_path |
There was a problem hiding this comment.
Is any of this necessary?
Comment on lines
75
to
91
| if (browser_database_path / LOGIN_DATABASE_FILENAME).exists(): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=(browser_database_path / LOGIN_DATABASE_FILENAME), | ||
| master_key=DEFAULT_MASTER_KEY, | ||
| ) | ||
| ) | ||
|
|
||
| sub_browser_database_paths = browser_database_path.iterdir() | ||
| for subdir in sub_browser_database_paths: | ||
| login_database_path = browser_database_path / subdir / LOGIN_DATABASE_FILENAME | ||
| if login_database_path.exists(): | ||
| login_data_paths.add( | ||
| BrowserCredentialsDatabasePath( | ||
| database_file_path=login_database_path, master_key=DEFAULT_MASTER_KEY | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I think all of these conditions and all of the searching can just be replaced with a glob.
for database_file in browser_database_path.glob(f'**/{LOGIN_DATABASE_FILENAME}'):
login_data_paths.add(...)^ Not tested.
monkey/agent_plugins/credentials_collectors/chrome/src/linux_credentials_database_selector.py
Show resolved
Hide resolved
Base automatically changed from
3426-windows-credentials-database-selector
to
develop
September 12, 2023 17:04
mssalvatore
reviewed
Sep 12, 2023
Comment on lines
59
to
63
| home_dirs = { | ||
| p.pw_name: Path(p.pw_dir) for p in pwd.getpwall() # type: ignore[attr-defined] | ||
| } | ||
|
|
||
| return home_dirs |
There was a problem hiding this comment.
Suggested change
| home_dirs = { | |
| p.pw_name: Path(p.pw_dir) for p in pwd.getpwall() # type: ignore[attr-defined] | |
| } | |
| return home_dirs | |
| return { | |
| p.pw_name: Path(p.pw_dir) for p in pwd.getpwall() # type: ignore[attr-defined] | |
| } |
mssalvatore
reviewed
Sep 12, 2023
| try: | ||
| if browser_db_path.exists(): | ||
| database_paths.append(browser_db_path) | ||
| except Exception as err: |
There was a problem hiding this comment.
What exception can be raised here?
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
from
06512dd to
d48dd12
Compare
mssalvatore
reviewed
Sep 12, 2023
| try: | ||
| login_data_paths = self._get_login_data_paths(browser_database_path) | ||
| database_paths.update(login_data_paths) | ||
| except Exception: |
There was a problem hiding this comment.
What exception can be raised here? Would it be better handled within _get_login_data_paths() so that function doesn't short circuit?
mssalvatore
reviewed
Sep 12, 2023
| if browser_db_path.exists(): | ||
| database_paths.append(browser_db_path) | ||
| except Exception as err: | ||
| logger.exception( |
There was a problem hiding this comment.
This logs at the error level. Does the average user really need an error message?
We only want Chrome and Chromium
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
3 times, most recently
from
8f64dd5 to
7661839
Compare
ilija-lazoroski
force-pushed
the
3426-linux-chrom-credentials-collector
branch
from
7661839 to
3d5f087
Compare
mssalvatore
approved these changes
Sep 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #
put issue number here.Add any further explanations here.
PR Checklist
Testing Checklist