Re: [PATCH] location-importer.in: Conduct sanity checks per DROP list

["Peter Müller" <peter.mueller@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 6996 bytes --]

Hello Michael,

> Hello,
> 
>> On 26 Sep 2022, at 19:26, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>
>> Hello Michael,
>>
>> better late than never: Thanks for your reply and the comments.
>>
>>> Hello Peter,
>>>
>>>> On 18 Aug 2022, at 22:42, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>>>
>>>> Previously, the lack of distinction between different DROP lists caused
>>>> only the last one to be persisted.
>>>>
>>>> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
>>>> ---
>>>> src/scripts/location-importer.in | 58 +++++++++++++++++++++-----------
>>>> 1 file changed, 39 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/src/scripts/location-importer.in b/src/scripts/location-importer.in
>>>> index 8d47497..e4a9ab0 100644
>>>> --- a/src/scripts/location-importer.in
>>>> +++ b/src/scripts/location-importer.in
>>>> @@ -1427,18 +1427,21 @@ class CLI(object):
>>>> 	def _update_overrides_for_spamhaus_drop(self):
>>>> 		downloader = location.importer.Downloader()
>>>>
>>>> -		ip_urls = [
>>>> -					"https://www.spamhaus.org/drop/drop.txt",
>>>> -					"https://www.spamhaus.org/drop/edrop.txt",
>>>> -					"https://www.spamhaus.org/drop/dropv6.txt"
>>>> +		ip_lists = [
>>>> +					("Spamhaus DROP list", "https://www.spamhaus.org/drop/drop.txt"),
>>>> +					("Spamhaus EDROP list", "https://www.spamhaus.org/drop/edrop.txt"),
>>>> +					("Spamhaus DROPv6 list", "https://www.spamhaus.org/drop/dropv6.txt")
>>>> 				]
>>>
>>> Would it not be better to have this as a dict?
>>>
>>> Or if you prefer something to iterate over, a tuple?
>>>
>>> Would it not be better/easier if we would use a handle like “SPAMHAUS-DROP”, “SPAMHAUS-EDROP”, … instead of a string with spaces? It does not matter that much to me, but since this is a machine-readable string, should it not be less human formatted?
>>
>> Hm, good point. I have changed that in the upcoming second version of the patch.
>>
>>>
>>>>
>>>> -		asn_urls = [
>>>> -					"https://www.spamhaus.org/drop/asndrop.txt"
>>>> +		asn_lists = [
>>>> +					("Spamhaus ASN-DROP list", "https://www.spamhaus.org/drop/asndrop.txt")
>>>> 				]
>>>>
>>>> -		for url in ip_urls:
>>>> -			# Fetch IP list
>>>> +		for list in ip_lists:
>>>> +			name = list[0]
>>>> +			url = list[1]
>>>
>>> It would be more Pythonic if you wrote it like this:
>>>
>>> for name, url in ip_lists:
>>> ...
>>>
>>>> +
>>>> +			# Fetch IP list from given URL
>>>> 			f = downloader.retrieve(url)
>>>>
>>>> 			# Split into lines
>>>> @@ -1448,11 +1451,11 @@ class CLI(object):
>>>> 			# downloads.
>>>> 			if len(fcontent) > 10:
>>>> 				self.db.execute("""
>>>> -					DELETE FROM autnum_overrides WHERE source = 'Spamhaus ASN-DROP list';
>>>> -					DELETE FROM network_overrides WHERE source = 'Spamhaus DROP lists';
>>>> -				""")
>>>> +					DELETE FROM network_overrides WHERE source = '%s';
>>>
>>> No need for the ‘’ around the %s.
>>
>> Yes there is, the SQL command results in an exception otherwise.
> 
> Err, you are allowing some SQL command injection here.
> 
> You do not want to use “%s” as a string format parameter for Python strings. It is more of a placeholder for the value that has to be escaped first - SQLite is using ? which is probably a better choice for Python.
> 
> The database binding is doing this automatically, but the arguments need to be passed after the query.
> 
> Under no circumstances whatsoever you want to chance the query dynamically. Never ever.
> 
> I fixed this here: https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=b56bf4bf065130b38968b580e0bea6db809783d8

better late than never: Many thanks for spotting and fixing this before it went into
anything stable or productive! Guess I'm not a good programmer after all...

All the best,
Peter Müller

> 
> Best,
> -Michael
> 
>>
>> Second version of the patch is coming right up.
>>
>> Thanks, and best regards,
>> Peter Müller
>>
>>>
>>>> +				""" % name,
>>>> +				)
>>>> 			else:
>>>> -				log.error("Spamhaus DROP URL %s returned likely bogus file, ignored" % url)
>>>> +				log.error("%s (%s) returned likely bogus file, ignored" % (name, url))
>>>> 				continue
>>>>
>>>> 			# Iterate through every line, filter comments and add remaining networks to
>>>> @@ -1475,8 +1478,8 @@ class CLI(object):
>>>>
>>>> 					# Sanitize parsed networks...
>>>> 					if not self._check_parsed_network(network):
>>>> -						log.warning("Skipping bogus network found in Spamhaus DROP URL %s: %s" % \
>>>> -							(url, network))
>>>> +						log.warning("Skipping bogus network found in %s (%s): %s" % \
>>>> +							(name, url, network))
>>>> 						continue
>>>>
>>>> 					# Conduct SQL statement...
>>>> @@ -1488,14 +1491,31 @@ class CLI(object):
>>>> 						) VALUES (%s, %s, %s)
>>>> 						ON CONFLICT (network) DO UPDATE SET is_drop = True""",
>>>> 						"%s" % network,
>>>> -						"Spamhaus DROP lists",
>>>> +						name,
>>>> 						True
>>>> 					)
>>>>
>>>> -		for url in asn_urls:
>>>> +		for list in asn_lists:
>>>> +			name = list[0]
>>>> +			url = list[1]
>>>> +
>>>
>>> See above.
>>>
>>>> 			# Fetch URL
>>>> 			f = downloader.retrieve(url)
>>>>
>>>> +			# Split into lines
>>>> +			fcontent = f.readlines()
>>>> +
>>>> +			# Conduct a very basic sanity check to rule out CDN issues causing bogus DROP
>>>> +			# downloads.
>>>> +			if len(fcontent) > 10:
>>>> +				self.db.execute("""
>>>> +					DELETE FROM autnum_overrides WHERE source = '%s';
>>>> +				""" % name,
>>>> +				)
>>>> +			else:
>>>> +				log.error("%s (%s) returned likely bogus file, ignored" % (name, url))
>>>> +				continue
>>>
>>> Is there a reason why this DELETE statement is not part of the transaction? If something goes wrong later on, you would have lost the previous data.
>>>
>>> Would it also not be programmatically better to start the import and then count how many lines you are importing?
>>>
>>> The “f.readlines()” call is something that I would consider a bit ugly.
>>>
>>>> +
>>>> 			# Iterate through every line, filter comments and add remaining ASNs to
>>>> 			# the override table in case they are valid...
>>>> 			with self.db.transaction():
>>>> @@ -1518,8 +1538,8 @@ class CLI(object):
>>>>
>>>> 					# Filter invalid ASNs...
>>>> 					if not self._check_parsed_asn(asn):
>>>> -						log.warning("Skipping bogus ASN found in Spamhaus DROP URL %s: %s" % \
>>>> -							(url, asn))
>>>> +						log.warning("Skipping bogus ASN found in %s (%s): %s" % \
>>>> +							(name, url, asn))
>>>> 						continue
>>>>
>>>> 					# Conduct SQL statement...
>>>> @@ -1531,7 +1551,7 @@ class CLI(object):
>>>> 						) VALUES (%s, %s, %s)
>>>> 						ON CONFLICT (number) DO UPDATE SET is_drop = True""",
>>>> 						"%s" % asn,
>>>> -						"Spamhaus ASN-DROP list",
>>>> +						name,
>>>> 						True
>>>> 					)
>>>>
>>>> -- 
>>>> 2.35.3
>