Split SocketConnection into subclasses

[hackathi]

Packet sockets may be bound to an interface, restricting the recieved packets to come from a specific interface. Additionally, recieving on raw sockets can be filtered by protocol (like it's done already for
raw-l3). This commit makes this functionality available to the user.

TODO:

• Update documentation consistently
• Update changelog
• Write unit-test for raw socket receiving (optimal but would be really nice to have)
• Update examples folder (optimal, could be done in another PR)

[aedrax]

looks good to me, simple enough changes too. Don't see any reason this can't be merged in

[SR4ven]

Thanks for the PR @mistressofjellyfish!

Some things we should discuss in the following comments.
Most important might be that boofuzz currently doesn't support receiving data on raw sockets, which makes these changes somewhat obsolete.

[hackathi]

@SR4ven I still have to fix a number of style issues and some tests that rely on SocketConnection being SocketConnection.

While I was at it, I implemented some receiving support into the RawL2/RawL3 classes, which unfortunately introduced some breakage with a semi-private API, as all of the Raw classes now accept a parameter to set their message/frame size. On the raw level, it only makes sense to receive a whole packet anyways, so recv() on those connections returns exactly one of those. That implies however that the MAX_PAYLOADS dict is now removed.

However, I see that as a good thing, because the values there were wrong for anything but bog-standard ethernet in any case as it ignored the existence of Jumboframes or anything other than 802.3, really. Wikpedia has a nice overview.

I'll rebase and squash the commits once I sorted the test and style issues. In the meantime, please review.

[SR4ven]

Great work @mistressofjellyfish, I really like the new style!

As for the code style, I recommend this checklist (running tox and black) and PyCharm for static code analysis :)

Now that we have all these socket_connection files, how about moving them into a separate folder called something like connections? This would keep the base folder and import lists a little cleaner.

As your branch isn't up to date with upstream/master any more and you can't easily merge the new diffs, we need to be really care full not to revert any of the recent changes. I'll take a closer look at this on the final review turn.

About squashing the commits, I can do that in the progress of merging, feel free to create new commits for changes. Makes it easier to review new diffs.

[hackathi]

Okay, so I finally got around to fix almost all of the remaining issues and made every test working again. Yay!

I've not gone forward in moving all the connection classes to their own respective folder. As this would -- and should -- move the ITargetConnection and SerialConnection classes aswell, I feel that it would be more appropriate to do that in another PR.

I see what I can do rebasing my PR.

[hackathi]

Seems like the CI likes to run again now, but I think I introduced some mess into the history (dunno what exactly happened here -- sorry). Regardless, that's fixable by carefully squashing commits together, so it shouldn't be too much of a problem. I'll leave it as-is right now unless you want me to do some cleanup @SR4ven.

[SR4ven]

CI tests only run when there are no merge conflicts.
About the merging, I was going to perform a squash and merge through GitHub, but that comes with the trait. GitHub might change your author email in the commit to the primary one you selected on GitHub (Could end up with something like this 46794237+sr4ven@users.noreply.github.com, see isaacs/github#1368 for more info).
If that's a problem you'd have to do some cleanup/squash work and I'll use standard merge.

About the pending changes, I'm about to push a commit myself, fixing some stuff.

[starek4]

Splitting the SocketConnection class is really a good idea. The code is much more readable now 👍

I am also really glad that we keep its previous interface working and marking it as deprecated so users have time to reimplement their tools.

[SR4ven]

Looking at the tests for raw l2/3 connections, I noticed that they don't really work as intended.
I don't have much time right now, so I only took a short attempt in fixing test_raw_l3. Would be nice if someone else could take a closer look at this.
The main problem seems to be that we receive our own outgoing packets because we only have a protocol filter, but no MAC-filter.

[hackathi]

Looking at the tests for raw l2/3 connections, I noticed that they don't really work as intended. [...]
The main problem seems to be that we receive our own outgoing packets because we only have a protocol filter, but no MAC-filter.

Well, we're using the loopback interface for testing. It's no surprise that we receive the packet we've just sent. That should not happen with other raw sockets, where the packet gets actually sent over the line / air / whatever as sent packets won't get dropped into the kernels receive queue.

[SR4ven]

Right, I completely forgot that we test on loopback... but that problem can be easily solved by simply "assigning" a different MAC address to the MiniTestServer (which works on raw-l2 so it can mimic any MAC).
For this to work we'd also need some filtering in the receive function, so that we only catch packets which come from our target instead of any that match the ether_type.
I quickly put together some code that seems to work well so far. It replaces data = self._sock.recv(self.packet_size) in RawL3SocketConnection.recv

elapsed_time = 0
start_time = time.time()
while elapsed_time < self._recv_timeout:
    self._sock.setsockopt(
        socket.SOL_SOCKET,
        socket.SO_RCVTIMEO,
        base_socket_connection.seconds_to_sockopt_format(self._recv_timeout - elapsed_time),
    )
    received, addr = self._sock.recvfrom(self.packet_size)
    print(addr)
    # See https://docs.python.org/3/library/socket.html at AF_PACKET adress tupel
    # https://docs.huihoo.com/doxygen/linux/kernel/3.7/if__packet_8h.html
    if (
        (list(addr)[2] == 0 and list(addr)[4] == self.l2_dst)  # Unicast from target MAC
        or list(addr)[2] == 1  # Bradcast
        or list(addr)[2] == 2  # Multicast
    ):
        data = received
        break
    elapsed_time = time.time() - start_time

However, writing unit tests and the MAC filter seems to require quite some extra work. We should perhaps make these changes in a different PR to keep things simple.
That would mean reverting 7cddcfd and merging this PR first, if that's fine by you @mistressofjellyfish.

[hackathi]

Hm... I don't think that this is a particularly elegant way to address this. My main concern is that I then have to provide a MAC to filter by. While I don't have a convincing argument on why I would have a scenario where I would want to have a catch-all, I don't have an argument why such a scenario could not exist.

Also, while this is rather trivial to implement for Layer 3 (iff Layer 2 is Ethernet), it's a whole different story on Layer 2, which in the current form should support stuff like 802.11 or 802.5.

So, in the end, I'd much rather write documentation about this issue and encouraging people to either pass a filter function or provide their own subclass of one of the raw classes. With the new design, that is a fairly straight forward to do after all, and we could always just do something like this:

def recv(self):
    elapsed_time = 0
    start_time = time.time()
    while elapsed_time < self._recv_timeout:
        self._sock.setsockopt(
            socket.SOL_SOCKET,
            socket.SO_RCVTIMEO,
            base_socket_connection.seconds_to_sockopt_format(self._recv_timeout - elapsed_time),
        )
        received, addr = self._sock.recvfrom(self.packet_size)
    
        # See https://docs.python.org/3/library/socket.html at AF_PACKET adress tupel
        # https://docs.huihoo.com/doxygen/linux/kernel/3.7/if__packet_8h.html
        if self.accept_packet(addr, received):
            data = received
            break
        elapsed_time = time.time() - start_time

def accept_packet(self, addr, received):
    return True

So anyone that wishes to filter just subclasses the appropriate Raw class and implements the accept_packet function.

With regards to the tests - because we know what is going on, I see no harm in doing a double-receive there, as long as it is documented why.

Edit: If we are not going the minmally invasive route, I think we should merge this PR now and discuss further in a second PR in order to avoid future merge conflicts. After all, this is a major change from an implementation perspective; and other contributors shouldn't base their changes on the SocketConnection class if it is clear that we're going to merge this PR eventually.

[SR4ven]

Hm... I don't think that this is a particularly elegant way to address this. My main concern is that I then have to provide a MAC to filter by. While I don't have a convincing argument on why I would have a scenario where I would want to have a catch-all, I don't have an argument why such a scenario could not exist.

While I agree that a catch-all scenario might come in handy at some point, I think the case where one would like to receive from just the target's MAC seems to be more common.
IMHO we should provide a built in MAC filter, which can be overwritten by a custom function or completely disabled. Though this PR will make it much easier to implement things like a MAC-filter, my guess is that average user can't or doesn't want to deal with such stuff.
After all, boofuzz is supposed to simplify fuzzing :)

Also, while this is rather trivial to implement for Layer 3 (iff Layer 2 is Ethernet), it's a whole different story on Layer 2, which in the current form should support stuff like 802.11 or 802.5.

As I don't know too much about 802.11 or 802.5, I can't tell why a MAC-filter could problematic. Aren't we somewhat bound to 802.3 because of the AF_PACKET socket anyway?
However, no need to commit to a solution yet, but as before, I'd tend to at least implement the option to pass a function to check the packet.

So anyone that wishes to filter just subclasses the appropriate Raw class and implements the accept_packet function.

While this would work just fine, why not use a callback function instead? Boofuzz already offers support for pre/post/restart callbacks etc. so that procedure should be know to users.

With regards to the tests - because we know what is going on, I see no harm in doing a double-receive there, as long as it is documented why.

The problem I see are sporadic failures on busy loopback interfaces. No idea how frequent this could happen, but if it does on CI tests, it would be really annoying as I can't retrigger them. There is an option to retry failed tests (https://github.com/pytest-dev/pytest-rerunfailures), but if possible I'd like to avoid such stuff.
However, fixing the unit-tests is something we need to worry about in another PR.

Edit: If we are not going the minmally invasive route, I think we should merge this PR now and discuss further in a second PR in order to avoid future merge conflicts. After all, this is a major change from an implementation perspective; and other contributors shouldn't base their changes on the SocketConnection class if it is clear that we're going to merge this PR eventually.

I'll revert the changes I made to the unit-tests and review again. I hope that @jtpereyda has the time to lay his eyes on this before we merge, it's a pretty big change after all.

[hackathi]

While I agree that a catch-all scenario might come in handy at some point, I think the case where one would like to receive from just the target's MAC seems to be more common.
IMHO we should provide a built in MAC filter, which can be overwritten by a custom function or completely disabled. Though this PR will make it much easier to implement things like a MAC-filter, my guess is that average user can't or doesn't want to deal with such stuff.
After all, boofuzz is supposed to simplify fuzzing :)

I think you know the userbase better than I do; however I'd much rather enable stuff than disable it - it is always much harder to get rid of than adding functionality. Passing a validation function is a practical solution, however I'd still argue the default should be just to accept everything while providing some default filters.

To some degree, we could also just make all filters implement some Interface and have one of the filters passed when instantiating the socket connection, like so:

foo = RawL3SocketConnection(interface="lo", packet_filter=EthernetMacFilter("aa:bb:cc:dd:ee:ff")

And for the default argument, we just use a AcceptAllFilter() or whatever.

As I don't know too much about 802.11 or 802.5, I can't tell why a MAC-filter could problematic. Aren't we somewhat bound to 802.3 because of the AF_PACKET socket anyway?
However, no need to commit to a solution yet, but as before, I'd tend to at least implement the option to pass a function to check the packet.

At least for 802.11, it's an AF_PACKET socket, and it should be for 802.5 as well, as we're just changing the Layer 2 protocol. The Problem whit implementing that "naively" is that the offset where the target and source mac adress resides in these protocols is different than in 802.3 ethernet. In any case, those filters depend on the User knowing which Layer 2 protocol to use.

So anyone that wishes to filter just subclasses the appropriate Raw class and implements the accept_packet function.

While this would work just fine, why not use a callback function instead? Boofuzz already offers support for pre/post/restart callbacks etc. so that procedure should be know to users.

See above. I don't have any hard feelings there, as long as the solution that gets implemented is easily extendable without modifying boofuzz source code.

With regards to the tests - because we know what is going on, I see no harm in doing a double-receive there, as long as it is documented why.

The problem I see are sporadic failures on busy loopback interfaces. No idea how frequent this could happen, but if it does on CI tests, it would be really annoying as I can't retrigger them. There is an option to retry failed tests (https://github.com/pytest-dev/pytest-rerunfailures), but if possible I'd like to avoid such stuff.
However, fixing the unit-tests is something we need to worry about in another PR.

Hm, that's indeed a valid concern I haven't thought about. In any case, introducing more code there won't fix the underlying issue of the loopback interface and still yields code paths that can't be tested. Is it possible to set up a virtual ethernet thingy in the CI? RedHat has a nice description. That would give us a 100% private interface for testing, which actually has two ends so we circumvent this problem entirely. At least for the raw tests, I think that this will be closer to a practical application as well.

[SR4ven]

I think you know the userbase better than I do; however I'd much rather enable stuff than disable it - it is always much harder to get rid of than adding functionality. Passing a validation function is a practical solution, however I'd still argue the default should be just to accept everything while providing some default filters.

Sounds reasonable.
Whether to use an interface for classes or callback is something we can discuss during implementation.

At least for 802.11, it's an AF_PACKET socket, and it should be for 802.5 as well, as we're just changing the Layer 2 protocol. The Problem whit implementing that "naively" is that the offset where the target and source mac adress resides in these protocols is different than in 802.3 ethernet. In any case, those filters depend on the User knowing which Layer 2 protocol to use.

In the example I posted earlier, the source mac was extracted from the address tuple returned by recvfrom(). So as long as this function is aware of the underlying layer, I see no problem here. It even works for layer 2 ethernet.

Hm, that's indeed a valid concern I haven't thought about. In any case, introducing more code there won't fix the underlying issue of the loopback interface and still yields code paths that can't be tested. Is it possible to set up a virtual ethernet thingy in the CI? RedHat has a nice description. That would give us a 100% private interface for testing, which actually has two ends so we circumvent this problem entirely. At least for the raw tests, I think that this will be closer to a practical application as well.

Virtual ethernet would fix that problem, but the issue here might be the implementation. First I'm not sure if the CI env allows it (most likely does) and second this would mean that dev pcs that run local tests would have to provide those interfaces too.
Currently I'd tend to make this one test optional and rely on other tests using mac filters. If a test with a mac filter passes, how likely is it that the receive all fails...
On top all CI tests run in either a dedicated VM or docker container, so technically there shouldn't be much traffic on loopback, it's just that we cannot guarantee it.

[SR4ven]

I have manually tested all connection classes except for receiving with UDP.
Apart for the two comments everything looks good to me.

Regarding merging, do you want to squash the commits to keep a certain history or can I simply squash everything into one commit? Also keep #358 (review) in mind.

[hackathi]

Regarding merging, do you want to squash the commits to keep a certain history or can I simply squash everything into one commit?

Just squash it all together, that is good. Regarding email; it really doesn't matter.

[SR4ven]

Many thanks to @mistressofjellyfish for the great work!

[jtpereyda]

Thanks all! Nice PR @mistressofjellyfish . The big ol' if statements in the previous code were an antipattern IMO.