Issue 62 restrict interfaces #77
Conversation
There was a problem hiding this comment.
I spent time with this today and this is largely what we specified. But, I found a few items worth discussing that don't correspond to any existing code line, so I'll share them here.
Most significantly, testing revealed that in single-host mode, the test script sporadically originates packets on a blacklisted interface because the networking/routing stack has decided that this is the best interface to get to the arbitrary/non-existent remote IP from the test script. The packet is passed (or dropped) without diverting it to FakeNet-NG. In a real use case, this would manifest itself as malware samples sporadically not receiving responses. I don't know how to fix this except to limit the use of this setting to MultiHost mode where it can't conflict with this behavior. So I think we have to do that.
I'd also like for us to add these new configuration settings to the test file template.ini to ensure the usual tests run ok with the default settings, too. The default is to turn off blacklisted interfaces, I just want the configuration code to be exercised during testing to detect any issues with that in the future.
Lastly: Should the original packet be written to the pcap when it involves a blacklisted interface? (see the call to self.write_pcap() in handle_pkt() of DiverterBase) From a high level, the purpose of blacklisted interfaces is to allow administrative access to FakeNet-NG hosts, and such traffic is not part of the fake little malware world we're trying to create and study when we run FakeNet-NG. I'm open to a discussion on this. I'd like to try to do what the user would expect, but without adding additional complexity with a configuration setting if possible.
fakenet/diverters/diverterbase.py
Outdated
| pkt.dst_ip in self.blacklist_ifaces)): | ||
| self.logger.debug("Blacklisted Interface. src: %s dst: %s" % | ||
| (pkt.src_ip, pkt.dst_ip)) | ||
| if self.blacklist_ifaces_disp == 'Drop': |
There was a problem hiding this comment.
Use .lower() and compare with lowercase 'drop' to ensure that if the default value from diverterbase.py:1083 is used (either automatically or by the user), it works.
fakenet/diverters/diverterbase.py
Outdated
| @@ -1141,12 +1152,33 @@ def handle_pkt(self, pkt, callbacks3, callbacks4): | |||
|
|
|||
| crit = DivertParms(self, pkt) | |||
|
|
|||
| # check for blacklisted interface and drop if needed | |||
| if (self.blacklist_ifaces and | |||
| (pkt.src_ip in self.blacklist_ifaces or | |||
There was a problem hiding this comment.
It looks like blacklist_ifaces will just be the raw string specified in the config which is a comma-delimited list of IPs. That being so, this line will merely check if the source IP is a substring of the list. On a system 11 interfaces with IPs 1.2.3.1 through 1.2.3.11, if the user specifies to pass packets involving 1.2.3.1, won't this code also inadvertently pass packets involving 1.2.3.10 and 1.2.3.11 because of the substring match? I'll revisit the comma-delimited case separately because I believe it also will pose issues.
There was a problem hiding this comment.
For a concrete example, from typing into the python interpreter:
>>> '1.2.3.1' in '1.2.3.10'
True
fakenet/diverters/diverterbase.py
Outdated
| self.blacklist_ifaces_disp = ( | ||
| self.getconfigval('linuxblacklistinterfacesdisposition', 'drop')) | ||
| self.blacklist_ifaces = ( | ||
| self.getconfigval('linuxblacklistedinterfaces', None)) |
There was a problem hiding this comment.
This takes the raw string that the user typed and stashes it in self.blacklist_ifaces as a str object. If the user specifies a comma-separated list of IP addresses here, then self.blacklist_ifaces will just be that string, e.g. 1.2.3.1, 1.2.3.5. Because the pkt.src_ip in self.blacklist_ifaces check can match in cases where it should not (see below) and must be replaced with a new check, I suggest modifying the code here to split these on commas/whitespace and store them as an array or a set, and then using set intersection (see https://docs.python.org/2/library/sets.html) to test if the set of source/dest IPs overlaps with the set of blacklisted interface IPs.
fakenet/diverters/diverterbase.py
Outdated
| # there needs to be no per-packet output by default. If there is | ||
| # output for each packet, an infinite loop is generated where each | ||
| # packet produces output which produces a packet, etc. | ||
| elif (pid and (pid != self.pid) and crit.first_packet_new_session & |
There was a problem hiding this comment.
Why did you use a boolean & operator instead of the logical and keyword at the tail end of this line?
There was a problem hiding this comment.
This is probably yielding inconsistent results from what you expected because of the varying operator precedence of & versus and, with respect to is not:
| Result | Values | With and |
With & |
|---|---|---|---|
| same | [False, False, False] | False (and) | False (&) |
| DIFF | [True, False, False] | False (and) | True (&) |
| same | [False, True, False] | False (and) | False (&) |
| same | [True, True, False] | True (and) | True (&) |
| same | [False, False, True] | False (and) | False (&) |
| DIFF | [True, False, True] | False (and) | True (&) |
| same | [False, True, True] | False (and) | False (&) |
| same | [True, True, True] | False (and) | False (&) |
fakenet/diverters/diverterbase.py
Outdated
| # output for each packet, an infinite loop is generated where each | ||
| # packet produces output which produces a packet, etc. | ||
| elif (pid and (pid != self.pid) and crit.first_packet_new_session & | ||
| no_further_processing is not True): |
There was a problem hiding this comment.
Or more succinctly, not no_further_processing (even though it reads as a double negative in English).
fakenet/diverters/linux.py
Outdated
| @@ -242,7 +242,7 @@ def handle_nonlocal(self, nfqpkt): | |||
| self.logger.error('Exception: %s' % (traceback.format_exc())) | |||
| raise | |||
|
|
|||
| nfqpkt.accept() | |||
| nfqpkt.accept() if not pkt.drop else nfqpkt.drop() | |||
There was a problem hiding this comment.
In this case I agree with @tankbusta that if / else would be more readable:
if pkt.drop:
nfqpkt.drop()
else:
nfqpkt.accept()
Issue #62 complete.