domainaware / parsedmarc

A Python package and CLI for parsing aggregate and forensic DMARC reports
https://domainaware.github.io/parsedmarc/
Apache License 2.0
961 stars 209 forks source link

Security concern with the recently introduced CSV download #500

Closed Szasza closed 3 months ago

Szasza commented 3 months ago

In https://github.com/domainaware/parsedmarc/commit/18f7508a1f09db554bc8a31bb26acc79d5c37c3f#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:

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 commented 3 months ago

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

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 😅).

kitzler-walli commented 2 months ago

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 commented 2 months ago

@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.