Refactored secure link

[podhrmic]

This PR adds a general template for secure (encrypted/authenticated) pprzlink.

The idea is simple - secure transport doesn't put bytes directly to the link device, but saves them in a local buffer until end_message() is called.

At that point, the buffer is passed to spprz_process_outgoing_packet() which is defined externally (a minimal weak implementation if provided). If spprz_process_outgoing_packet() returns true, the packet is sent.

Similarly for the incoming packets (see spprz_utils.h for detailed description). spprz_utils.c provide just a trivial weak implementation for demonstration purposes, and should not be used for real application.

The advantage is that pprzink doesn't constrain what encryption/authentication schema can be used, as this is all up to the user. So no additional files / libraries/ etc are needed for pprzlink itself.

[podhrmic]

@gautierhattenberger thanks for looking at this.

But in fact it means that we can't use the XBEE api mode (or do logging?) with encryption unless we make secured version of each of these transport.

I am not familiar with XBEE api mode, but you could modify the xbee transport to have calls to process_incoming/outgoing_packets() the same way as secure pprz transport does (and use the simple pass through most of the time. So yes, you would need to update each transport.

I should mention that my encryption algorithm wasn't meant to fit all needs. If you need multiple UAVs, you will be probably happier with XBEE Api (that might offer some sort of encryption of its own). Indeed some multi-party-encryption can be added later.

Here is an other idea (and I'm not sure it will work): what if encryption is not a transport but a device (so transport layer is unmodified) that is initialized with an actual device (uart, udp, sd, ...) and will actually use it in the end after encryption. What do you think ? My thought is that with this all existing layers can be reused. It is not even necessary to put it in pprzlink, but it still might interesting for reusing it elsewhere.

It would require the device to be "packet aware" and thus be able to parse the pprzlink data (and find the start and end of each packet etc). Unfortunately there is no clean solution, because I am essentially encrypting only the payload - but in order to do so, I need to make modifications to the transport layers as I did here (to be able to correctly construct messages).

The other option I explored was to modify the message generators - avoids changing the transport code, but you have to generate different code for secure and standard pprzlink and change the way messages are constructed, so that would be potentially even messier.

So I suggest following:

1. clearly state that secure link has limitations (but can be expanded later) and is not a drop-in-replacement for all current transports
2. add only secure_ppprzlink_transport() and keep the other transports as they are

[gautierhattenberger]

On a second thought, your right, it is not the device that should be chained, but the transport. So something like messages -> crypto transport (but no header/tail) -> regular transport (pprz/xbee/...) -> device To do this, you need to initialized the crypto transport with one of the "normal" transports and pass the encrypted buffer for encapsulation once it is finished.

[podhrmic]

yes that would be ideal.

Thoughts how to implement it without completely changing the current code?

I see one major issue with this general approach:

• the secure link has to be able to understand (at least some) of the messages (mostly for the key exchange). Thus it has to have access to the generated code in similar sort like paparazzi (for example here ).

Which would mean pulling paparazzi code back into pprzlink:-/ Does that make sense?

[gautierhattenberger]

At best, pprzlink should only care about applying the encryption. The key exchange protocol should be handle on paparazzi. Maybe pprzlink can provide a part of the code with help functions and a specification about how to use them. Regarding implementation, I'll have a look in the coming days. Maybe I can make an example how "hook" transport layer to make it generic for testing purpose.

[podhrmic]

At best, pprzlink should only care about applying the encryption. The key exchange protocol should be handle on paparazzi. Maybe pprzlink can provide a part of the code with help functions and a specification about how to use them.

That is how I implemented it originally, but it turned out to be quite messy, because then both paparazzi and pprzlink needed to link against hacl (to have access to encryption functions).

If we want to make only minimal changes to pprzlink, then this PR is IMHO a good try. You only call two external functions that do nothing on default (process_incoming/outgoing_packet()) and the user (or in this case paparazzi) provides a specific implementation with encryption and key exchange. Sure there is a little bit of overhead of adding extra variables to the transport struct, but it is only a few bytes.

These two functions process_incoming/outgoing_packet() can actually be in all transports (we wouldn't need secure_transport), so you get encryption potentially for all cases.

Thoughts?

I know I am kind of rushing on this, but I would like to get at least a minimal example merged soon.

[podhrmic]

I thought about it some more:

1. changing the other transports to support the same encryption as secure pprzlink doesn't make much sense. short and pprzlog transports are for specific applications, ivy transport is for a different use. XBee transport has a different message format, plus my encryption algorithm doesn't currently support multi UAV communication.
2. I can imagine changing pprz_transport in a way that allows optional encryption, so there wouldn't be need for additional spprz_transport which would be pretty slick.

Again, I am not trying to cover all possible use cases - the goal is to implement secure pprzlink that runs over transparent serial port and works with one GCS and one UAV.

[gautierhattenberger]

You can check this branch https://github.com/paparazzi/paparazzi/tree/crypto_transport for an example of chained transports. Some remarks:

• it is not tested, not even compiled
• some parts are probably missing but it is just to explain what I had in mind
• the current location on paparazzi side can be discussed
• it is not yet as generic as expected (selecting the transport on top only from configuration) but can be achieved with minimum effort if we go this way.

[podhrmic]

You just define your own transport in paparazzi, leaving pprzlink as is. I haven't thought of that and I like it.

However, I need to distinguish between standard and encrypted packets (see #68 ), currently the STX byte gets losts if I just call https://github.com/paparazzi/paparazzi/commit/0e552b47cdab6458484aa0049f798df25085849b#diff-c4d92c714a2d634cb479123f03e2d4f3R146

Are you OK to expand pprz_check_and_parse() with something like this:

  switch (t->status) {
    case UNINIT:
      switch (c) {
        case PPRZ_STX:
          t->status++;
          t->packet_encrypted = false;
          break;
        case PPRZ_STX_SECURE:
          t->status++;
          t->packet_encrypted = true;
          break;
        default:
          break;
      }
      break;

That is general enough and doesn't change anything for normal use. EDIT: already implemented here https://github.com/paparazzi/pprzlink/commit/1b5bbcd4b325a155ed281669095d9530663a0bdd

If you agree with this change, I ll go ahead with a new PR.

[podhrmic]

Plus also at that point I ll ditch encryption for pprzlink 1.0 just to keep it simple (so only 2.0 is supported)

[gautierhattenberger]

As I said, implementing in Paparazzi is not a strong choice, it is just that your crypto lib is on this side.

Regarding the packet being encrypted or not, it wasn't very clear maybe with my comments. The crypto transport layer should come with a header as well (initialized here: https://github.com/paparazzi/paparazzi/blob/crypto_transport/sw/airborne/modules/datalink/crypto_haclc_dl.c#L82) and it has to be at least one byte that could hold several informations like:

• is the packet encrypted
• is the packet part of the key exchange protocol (and which step)
• is the packet only signed
• is the packet using a timestamp or not
• ...

The first point is solving the issue of mixing regular and encrypted messages. The second one is also very useful I think as you could integrate the key exchange protocol at the transport level (around here: https://github.com/paparazzi/paparazzi/blob/crypto_transport/sw/airborne/modules/datalink/crypto_haclc_dl.c#L149). So for each new message (encapsulated by PPRZ, XBEE or any future protocol), you first parse/remove crypto header and then you can choose to:

• directly pass the payload part (no encryption)
• decrypt the message and then pass the payload
• send back messages for the key exchange

The benefit is that you don't need to implement the logic outside of your own code, and it can make integration of this transport directly inside pprzllink much easier as it won't require specific code for the user.

[podhrmic]

OK, I think only one extra byte will be enough (encrypted packet/plaintext packet). I still want to leverage using standard pprzlink messages for key exchange etc (see https://github.com/paparazzi/pprzlink/pull/70/commits/eac160d51d0df3bac1e605fe946fe11e2e50a37e#diff-410c505a32fc1a58d7da18d75f47cb9bR1985 ), but that should be pretty straightforward.

[gautierhattenberger]

Of course the messages used for the key exchange are regular messages, you just send them from the transport receive function. Well, I guess it is possible, that would be a first...

[podhrmic]

Closing this as Off-topic, moving the discussion and efforts to https://github.com/paparazzi/paparazzi/pull/2205