Skip to content
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

Security concern with the recently introduced CSV download #500

Closed
Szasza opened this issue Mar 30, 2024 · 3 comments
Closed

Security concern with the recently introduced CSV download #500

Szasza opened this issue Mar 30, 2024 · 3 comments

Comments

@Szasza
Copy link
Contributor

Szasza commented Mar 30, 2024

In 18f7508#diff-953601e25cfe2a31ac1b2602fcab0be206d5c7bb860165d66679f455ead64fcfR335 a direct download from GitHub has been introduced.

Unfortunately the download happens before the code is having a chance to use the map file sitting with the code The only ways to avoid this from happening are:

  • using offline mode, which is not desirable in all cases, or
  • passing in a map as the function parameter, which is not possible as this parameter is not exposed to the CLI

This means that if a malicious actor gains access to the GitHub repo, a manipulated CSV file will automatically be downloaded for all parsedmarc installations.

It would be ideal to use the local file as a first option. Alternatively, it would be great to be able to turn off the automatic download without rendering the whole parsedmarc run offline, or maybe being able to provide the download URL as a config option, so the file can be refreshed in a more controlled manner.

@seanthegeek if you are ok with it, I am happy to rework the reverse DNS map usage slightly to address the above.

@seanthegeek
Copy link
Contributor

seanthegeek commented Mar 31, 2024

I'm thinking we can add three options to the config file:

  • always_use_local_files - Disables the download of the reverse DNS map
  • local_reverse_dns_map_path - Overrides the default local file path to use for the reverse DNS map
  • reverse_dns_map_url - Overrides the default download URL for the reverse DNS map

That would allow users to pick exactly how they would like parsedmarc to load the map.

I built the current behavior to encourage others to contribute to the map, and to avoid users needing to upgrade the parsedmarc package just to get an updated map, or riequire me to make new releases just for updated maps for anything other than correcting errors (like the last few releases 😅).

seanthegeek added a commit that referenced this issue Apr 1, 2024
Add the following general configuration options:

- `always_use_local_files` - Disables the download of the reverse DNS map
- `local_reverse_dns_map_path` - Overrides the default local file path to use for the reverse DNS map
seanthegeek added a commit that referenced this issue Apr 2, 2024
- Actually save `source_type` and `source_name` to Elasticsearch and OpenSearch
- Reverse-lookup cache improvements (PR #501 closes issue #498)
- Update the included `dbip-country-lite.mmdb` to the 2024-03 version
- Update `base_reverse_dns_map.csv`
- Add new general config options (closes issue #500)
  - `always_use_local_files` - Disables the download of the reverse DNS map
  - `local_reverse_dns_map_path` - Overrides the default local file path to use for the reverse DNS map
  - `reverse_dns_map_url` - Overrides the default download URL for the reverse DNS map
@kitzler-walli
Copy link

My instance still tries to download the csv from github even though I set this in my ini file:

always_use_local_files = True
local_reverse_dns_map_path = /opt/parsedmarc/base_reverse_dns_map.csv

@Szasza
Copy link
Contributor Author

Szasza commented Apr 29, 2024

@kitzler-walli is it possible that you use a parsedmarc version earlier than 8.11.0, or not using the correct config file when invoking parsedmarc? As per https://github.com/domainaware/parsedmarc/blob/master/parsedmarc/utils.py#L340-L341 if always_use_local_files is set.

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

No branches or pull requests

3 participants