Ethernet denial of service

[HeroicKatora]

Bug report

A malicious sender over an ethernet interface can cause a denial of service by making every call to poll return an error before process_egress could have been called. This will cause the interface to never send any packets that are pending on any socket.

The same happens inwittingly when the ingress call can constantly keep the buffer of a socket filled, for example by saturating line rate of a fast enough nic with a packet generator.

Reproduction steps

1. Create a phy::Device that has an infinite stream of packets and somehow signals when a packet would have been sent (for example by panicking).
2. Wrap that device into an interface, setup routing to be valid for layers eth, ip, udp
3. Enqueue a packet to transmit on a UdpSocket within a SocketSet
4. loop infinitely with iface.poll(&mut socketset)
   3.1 Each loop, you can dequeue packets received, or not, doesn't matter
5. Observe that the send code is never triggered

Instead of having an infinite stream of packets read each time poll is called, you can alternatively implement the phy such that exactly one malformed packet is ready each time. This has the same effect on the poll-loop.

I have code ready for this, should I link it or not?

Fixes

There are several possible ways to fix this:

• Collect the result of both ingress and egress, for example by removing the ? from the their initial call point and instead

  let ingress = self.socket_ingress(sockets, timestamp);
  let egress = self.socket_egress(sockets, timestamp);
  let processed_any = ingress? | egress?;
  

  This has the risk of losing the egress error information if ingress errored as well.
• Store a 'spin'-bool within EthernetInterface that determines whether to ingress or egress first. Flip it everytime poll is invoked.

Both of the above are still unsatisfying, as each poll call will at most egress a single packet before evaluating the Err from the next socket_ingress call and returning. Note that process_ingress will loop while there are RxToken returned from the phy while proces_egress will try to send a single packet from each socket within the set. This imbalance is quite confusing.

Workarounds

Nothing quick and fast here, you'd need to poll without receiving packets so that it can not error. That possible leaves a wrapping phy implementation that allows you to turn off receive. Something like this. Then you can disable rx for the phy, call poll once without transmit being potentially interrupted by receive, and reenable rx.

[whitequark]

• Store a 'spin'-bool within EthernetInterface that determines whether to ingress or egress first. Flip it everytime poll is invoked.

You can't really do this as the TCP state machine currently assumes ingress first.

Note that process_ingress will loop while there are RxToken returned from the phy while proces_egress will try to send a single packet from each socket within the set. This imbalance is quite confusing.

The imbalance is an attempt at fairness between several sockets. Admittedly not a very good one, but sending all available packets from the first socket in the set generally has worse results.

[HeroicKatora]

The imbalance is an attempt at fairness between several sockets. Admittedly not a very good one, but sending all available packets from the first socket in the set generally has worse results.

Imbalance between several sockets still sounds better than imbalance between incoming, malicious traffic and outgoing packets. But I think this has the same root case: The interface offers of poll (or its implementation) offers no way to continue after the first error. And I'd argue the current state is also an imbalance, worse since it is effectively a 100%-0% imbalance towards the first erroring code path instead of 'only' a slow down.

Here's another idea: Send single packets from all socket in round-robin fashion until none has any more packets to emit or some number of packets have been sent in total (fixed, configurable, ..). Thus, poll will terminate and give all sockets a chance to ready more packets in their code paths and at the same time it is possible to egress multiple packets (i.e. an answer for each packet that was received) and thirdly sockets can send different numbers of packets each while maintaining a balance (due to RR).

This is however not quite sufficient alone, as really there must be a way not to error out with ? immediately, but to fairly give other equal traffic routes (in and out) a chance to continue despite this.

[whitequark]

What about changing the poll signature so that the errors are reported via a callback? This means any errors wouldn't interrupt the inner loop in the poll.

[Dirbaio]

To me this looks like this is 2 problems that could be tackled separately:

1. Ingress being given preference over egress for CPU time.
2. Errors from sockets interrupt the polling loop (on both ingress and egress).

My proposal to solve 2 is to:

• Log and ignore Malformed errors. Do not report them to the user, do not abort the poll loop. The rationale is these errors aren't really actionable. Their only use is logging them for human debugging, and in that case we can easily log them from inside smoltcp instead of jumping through hoops to report them to user code.
• Change socket process() and dispatch() so they can't return errors at all. Sockets either log and ignore errors, or if they're smoltcp bugs (things that should never happen), they deal with them with assert/panic.

For 1, I'm not sure what'd be the solution.

You can't really do this as the TCP state machine currently assumes ingress first.

@whitequark could you expand on this? To me it seems that handling egress first then ingress would just look to the TCP socket like the ingressed packet arrived "a little bit later", ie it could've happened anyway. I can't see how it can break things. If the socket wants to send something, it'll return PollAt::Now and get polled again.