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.

Sign up for GitHub

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

Jump to bottom

Issue 62 restrict interfaces #77

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Issue 62 restrict interfaces #77

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
473e12b
added interface restriction logic for linux
mlhaigh May 1, 2018
4eba1d4
fixed indent error
mlhaigh May 2, 2018
cf35fd0
Testing complete
mlhaigh May 3, 2018
3c3dd34
removed extra logging statement
mlhaigh May 3, 2018
9f498d9
stopped packet details from printing when interface is blacklisted
mlhaigh May 4, 2018
e4d2845
fixed comments and style
mlhaigh May 4, 2018
fd3d9bc
pull request changes for 62 restrict interfaces
mlhaigh Aug 24, 2018
14f931f
restrict interfaces feature only available in multihost mode
mlhaigh Aug 24, 2018
c3198f6
added blacklist interface config to test config template
mlhaigh Aug 24, 2018
4151e8b
cleaned up minor issues with blacklist interfaces feature
mlhaigh Aug 31, 2018
2947385
Merge branch 'master' into issue-62-restrict-interfaces
mlhaigh Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions fakenet/configs/default.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ DebugLevel: Off
# should be applied to all interfaces. Comment out to leave unconfigured.
LinuxRedirectNonlocal: *

# Specify which interfaces Fakenet-NG will ignore. Disposition can be
# set to "Drop" or "Pass". "Drop" will drop the packet and "Pass" will ignore
# it and allow it to pass through to any listening application or Fakenet-NG
# Listener. Enter BlacklistedInterfaces as a list of IP addresses separated by
# comma or space (example 127.0.0.1 192.0.0.1).
LinuxBlacklistInterfaces: No
LinuxBlacklistInterfacesDisposition: Drop
LinuxBlacklistedInterfaces: 127.0.0.1

# Set LinuxFlushIptables to Yes to have the Linux Diverter flush all iptables
# rules before adding its FakeNet-NG-specific rules to iptables. FakeNet-NG
# will restore all old rules when it exits, unless its termination is
Expand Down
34 changes: 33 additions & 1 deletion fakenet/diverters/diverterbase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,17 @@ def parse_diverter_config(self):
self.logger.debug('Blacklisted UDP ports: %s', ', '.join(
[str(p) for p in self.getconfigval('BlackListPortsUDP')]))

# Ignore or drop packets to/from blacklisted interfaces
# Currently Linux-only
self.blacklist_ifaces = None
if self.is_set('linuxblacklistinterfaces'):
self.blacklist_ifaces_disp = (
self.getconfigval('linuxblacklistinterfacesdisposition', 'drop'))
self.blacklist_ifaces = (
self.getconfigval('linuxblacklistedinterfaces', None))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.logger.debug('Blacklisted interfaces: %s. Disposition: %s' %
(self.blacklist_ifaces, self.blacklist_ifaces_disp))

def write_pcap(self, pkt):
"""Writes a packet to the pcap.

Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a concrete example, from typing into the python interpreter:

>>> '1.2.3.1' in '1.2.3.10'
True

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':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.logger.debug("Dropping blacklist interface packet")
pkt.drop = True
else:
self.logger.debug("Ignoring blacklist interface packet")
no_further_processing = True

# fnpacket has parsed all that can be parsed, so
pid, comm = self.get_pid_comm(pkt)
if self.pdebug_level & DGENPKTV:
logline = self.formatPkt(pkt, pid, comm)
self.pdebug(DGENPKTV, logline)
elif pid and (pid != self.pid) and crit.first_packet_new_session:

# check for no_further_processing here in order to filter out
# packets that are being ignored already due to a blacklisted
# interface. If a user is using ssh over a blacklisted interface
# 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 &
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you use a boolean & operator instead of the logical and keyword at the tail end of this line?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (&)

no_further_processing is not True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or more succinctly, not no_further_processing (even though it reads as a double negative in English).

self.logger.info(' pid: %d name: %s' %
(pid, comm if comm else 'Unknown'))

Expand Down
4 changes: 4 additions & 0 deletions fakenet/diverters/fnpacket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ def __init__(self, label, raw):
self._is_ip = False
self._is_icmp = False

# Packet handler logic can check if a drop condition has been met.
# Currenty used for blacklisting interfaces
self.drop = False

# Some packet attributes are cached in duplicate members below for code
# simplicity and uniformity rather than having to query which packet
# headers were or were not parsed.
Expand Down
6 changes: 3 additions & 3 deletions fakenet/diverters/linux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I agree with @tankbusta that if / else would be more readable:

if pkt.drop:
    nfqpkt.drop()
else:
    nfqpkt.accept()


def handle_incoming(self, nfqpkt):
"""Incoming packet hook.
Expand All @@ -266,7 +266,7 @@ def handle_incoming(self, nfqpkt):
self.logger.error('Exception: %s' % (traceback.format_exc()))
raise

nfqpkt.accept()
nfqpkt.accept() if not pkt.drop else nfqpkt.drop()

def handle_outgoing(self, nfqpkt):
"""Outgoing packet hook.
Expand All @@ -293,7 +293,7 @@ def handle_outgoing(self, nfqpkt):
self.logger.error('Exception: %s' % (traceback.format_exc()))
raise

nfqpkt.accept()
nfqpkt.accept() if not pkt.drop else nfqpkt.drop()

def check_log_nonlocal(self, crit, pkt):
"""Conditionally log packets having a foreign destination.
Expand Down