Security improvements #1411
Security improvements #1411
Conversation
Updated RNG for FHSS sequence. Added HMAC instead of CRC (optional).
|
This is really cool. The problem that we have is that R9M TX modules are very limited on space! 2.1.0 This PR This PR (removed hash, and copied 3 bytes from UID) Shows that the SHA256 hash implementation uses 772 bytes Changing the PCG RNG to something simpler like the mulberry32 (which is 32-bit logic and no 64-bit) reduces the code from 46212, saving 116 bytes. |
|
Unfortunately mulberry32 suffers from the same flaw as the original RNG, i.e. if you iterate through values for UID[2], calculating the seed as shown below. The sequence generated for the value of
I'll see where I can reduce the size. Jye suggested calculating the hash and passing as a define, which would mean we could leave out the SHA256 code. Just need to solve the problem of manual binding with that implementation. |
|
This does nothing for increasing the security of the protocol. FHSS replay vectorIt doesn't matter what random() implementation you use, because the FHSS sequence is easily sniffed as it is broadcast. There's no need to figure out the original UID, you just need the hop table, and that's public information that is repeated as often as twice a second. Packet CRC brute force vectorFrom there, you don't need to know the UID from the sync packet either. You just need to know the CRC seed to be able to generate valid packets. The CRC is only 14 bits, so with 500 packets using it every second and only one packet being needed to determine the seed via brute force, the attacker doesn't need to know the UID at all. Sync packet replay vectorSince the hash is passed in the sync packet, this is vulnerable to simple replay attacks too, right? Since the value is static, if an attacker wanted to send a sync packet they can just replay one that was already captured. They don't need to to "take control" of the target either, the sync packet is just to get the RX in sync, and prevent collisions, not as a digital signature to confirm the identity of the TX. DoS vectorFinally, does an attacker really need to take control of the target? Without any of this UID nonsense, I can just broadcast following the hop table and knock someone out of the sky. ConclusionThere is no need to add this bloat to the code under the guise of security for show when it adds no practical protection or benefit. To be secure, both ends would need some sort of chaining / morphing keys that adjust the FHSS continuously so that it can't simply be sniffed in under a second and replayed, and the CRC seed would have to keep changing as well. There's no implied security in the protocol, the checks that are there are to aid in collision prevention (everybody gets a different hop sequence) and then preventing RXes from syncing to the wrong TX via the partial UID match in the sync packet. |
src/src/rx_main.cpp
Outdated
| for(int i= 0; i< 3; i++){ | ||
| char str[3]; | ||
|
|
||
| sprintf(str, "%02x", (int)UIDHash[i]); |
There was a problem hiding this comment.
Do NOT use sprintf! It's a real hog! 1788 bytes it uses.
It's just debugging so just use DBG("%x ", (int)UIDHash[i]);
src/src/tx_main.cpp
Outdated
| for(int i= 0; i< 3; i++){ | ||
| char str[3]; | ||
|
|
||
| sprintf(str, "%02x", (int)UIDHash[i]); |
|
I agree with almost everything you have said @CapnBry. I looked at other methods but could not find a solution which didn't compromise the point of ELRS. The current implementation makes it particularly easy to discover the UID, FHSS sequence and the CRC seed quite quickly and without requiring any hardware other than a standard TX module. I can demonstrate this if you like. The intention was to raise the barrier for entry, either in terms of time or hardware required. I didn't realise how close to the limit some of the targets are on space and saw this as an improvement at low cost. I now better understand the bloat issue. I have some improvements which seem to significantly reduce the bloat. With regards to bloat, a few bytes could be saved by reducing the size of the UID to 4 bytes instead of the current 6. The first two bytes are not used at all, and the size of the UID is replicated in the MasterUID and BindingUID. The UID seems to be historically based on the size of a MAC address. |
|
Your changes do nothing to address your complaints though, other that obscuring the UID. The thing is, nobody needs to know the UID, I can take control against your code without ever knowing the UID or using any special hardware using simple replay on the sync packet and the same brute force CRC seed discovery (which can be performed on the device in milliseconds). It therefore fails to raise the barrier of entry at all-- it solely makes the code more complicated with absolutely zero increased difficulty. I'll keep saying this over and over. There is zero benefit and zero done to address security with this. Therefore it doesn't matter how much code space it occupies compared to master, unless it occupies less code space, there's no reason to even consider this at all. I'm trying to comprehend how you believe this code change makes any difference. Sync packet
FHSS hop sequence
CRC Initializer
I agree, it does prevent a sniffer from directly capturing the UID from the air, but the UID is not needed if the value you need to send valid packets is known. There's no point in adding complexity for absolutely no benefit. All the exploits I am listing require no additional hardware, just changes to the existing ELRS code. PracticallyWhat is the ultimate goal of your proposed security? Preventing someone from connecting to your drone and flying it to their house? Nobody is going to be stealing a model by connecting to an idle receiver that has no transmitter on. It is far more practical an "attack" to just disrupt the signal of the target to create a loss of control, a DoS. Taking control is certainly a more interesting attack, but the result is the same to the victim, and if it were for malicious intent (to disrupt a race) you'd be far more successful by creating intermittent loss of signal. This is just following the FHSS (which we know is immediately discoverable on standard hardware) and transmitting over the victim to clobber their LQ at crucial points. |
…ibrary. 2. FHSS sequence reseeds, avoiding the need for a better RNG library. 3. UID reduced from 6 bytes to 4 as the first two bytes weren't used. As this is replicated 3 times, it saves more bytes than the hash introduces.
If you don't know the UID, it is a slower process to discover the hop sequence, particularly if you are using an existing TX to do this. Discovering the sequence through observation is error prone, it's much more accurate to discover the UID.
It may occupy less space now.
Proposed solution sends a 3 byte identifier which isn't used to generate the hop sequence.
Immediately discoverable is a bit of a stretch. Brute forcing it and listening to the sequence will always be possible. Existing implementation makes it easier by giving you 75% of the information you need to generate the sequence.
No arguments here. I looked at adding a HMAC, but this was obviously way to much bloat.
UID not needed, but it sure is handy.
Here is where I'm coming from. I decided to look at how hard it would be to take control of a receiver with not much background in RC links or RF, but a security history. As I was looking at the code I found things, like sending the UID and the fact that the final byte I needed to brute force created half as many sequences as expected, that made the job significantly easier. I wrote the hijack code and successfully used it in testing. This pull request addresses those things that made it easy. It is entirely possible that a different method exists to achieve the same thing, although I struggle to understand how a full FHSS sequence can be discovered 'immediately' by hardware that can only listen on one channel at a time and doesn't know what the next channel will be, nor how long in between channel hops. The practicality of the attack (as in taking control of a drone and flying it away) is affected by other factors like the channel order for the sticks and any auxillary channels. Also need to get the right packet rate and the I think what I've done is an improvement, but I understand your position. Even if these changes are discarded, the reduction of the UID size is probably worth implementing to save a bit more space. Thanks for your time. |
|
The pushed commit still has the fundamental flaw that sending the hash in the clear is no different from stopping an attacker from just replaying the captured value to allow a sync. Not being able to get back to the UID is not worth anything if it does not prevent the same outcome. Making any changes to the FHSS you'd need to first show me how it currently is doing unfair allocation. The only thing the FHSS generator needs to do is use each channel equally across all possible input UIDs, which it does a good enough job currently that no changes are needed to that. Please stop wasting my time with your giant changes that fundamentally do not address anything other than obscuring the UID, which doesn't do anything to increase the security of the system as the UID is not needed to produce a valid OTA stream by an attacker. |
|
I don't know much about RF either but from a security perspective let's not compare a DoS attack and a remote takeover. @redfiveau interesting findings :) |
|
@CapnBry You seem to have a pretty good grasp on this. I'd be curious where you'd start if you wanted to make a version that DOES have some security against the kind of attack @redfiveau mentions. Such a ELRS-secure might be too big for some current hardware. It might have a lower effective data rate. And it would be useful in some use cases. You've expressed well why this commit is NOT the way to make it more secure. Do you have some thoughts on what WOULD be a good way to go to prevent takeover, if the use case made with worthwhile? |
|
Trying to think anyone would be able to bolt on encryption on top of a 5 byte payload packet sent 1000x a second over a lossy connection with very knowable data inside is a very foolish undertaking. You might be able to make it slightly more secure to a person who has no actual motivation to actually do harm, but it would be very hard to secure while working inside those constraints. It is possible but ExpressLRS isn't some amazing magic thing that has intrinsic value: it is just something that speaks CRSF, transmits it via LoRa/FLRC. Adding on security would require handshaking, probably larger packets, slower rates, etc. Once you've thrown out everything from the ELRS OTA that made it ELRS, why are you even bothering to start from ELRS? If someone wants a secure RC link, then start by making a secure RC link from the ground up with that in mind, don't halfass it by trying to retrofit it, adding tons of code and complexity to an existing system. There's also zero value in what you end up with if someone can just RF blast you out of the sky anyway. Someone taking control over your drone? Why would that happen? What is their goal? Would just knocking you out of the sky also achieve their objective? You have to get into some sort of Marvel movie plot if you think an attacker is going to take control of an ELRS model and make it do something uncommanded and the attacker gets away because the RC link was hijacked. The question keeps coming back to: why? Security just to say you have it is dumb. My handset doesn't even have a lock screen that requires 2FA to use it, we should add that because it is more secure and therefore better? |
|
@CapnBry is absolutely correct here. If you want a secure link, you need to start again. Adding anything approaching an actually secure system is counter to the goals of the project, i.e. it wouldn't be Express and it wouldn't be Long Range. My commit made a small improvement to security, but did not make the system secure and attempts I made to introduce anything more were incompatible with existing hardware and probably not super effective. That said, the ability to take control of an incoming drone instead of blasting it out of the sky is valuable in cases where the drone has an explosive payload on it, but as @CapnBry says, this is a movie plot threat. |
Absolutely it becomes a different project. Different goals. Getting into this in more detail, working with some top security experts, I've learned that all the work put into ExpressLRS to make an awesome RC control system is very valuable. The way it handles all the corner case, the Configurator work, etc all applies just as much to an encrypted system. There's no need to duplicate all that work, when the goal is to have a solid, reliable RC link that's also encrypted (particularly the telemetry). From what I've seen, these two ideas have gotten conflated: A) ELRS doesn't need to use encryption to achieve its goals B) An encrypted RC control system doesn't need to do anything that ELRS does, and can't benefit from the thousands of hours of work already put into an open design and open code for a great system. Sentence A is true. Besides the fact that ELRS is great, it's also proven to be really good, really solid. You can rely on the quality, as opposed to guessing that a new system designed completely from scratch might be good. Most of not all of the weird bugs you'd have in a new system have been found and fixed in ELRS. It would be silly to re-create the same bugs by starting over, ignoring everything learned by the ELRS devs and all the work they've done. There's just no reason to re-do the same 1,000 pull requests that have already been done in ELRS. |
|
My two cents to this It is possible to encrypt a small bandwidth radio link. You need:
From a data rate perspective you loose only a packet now and then to transmit key index and seed. This is not perfect security and with enough sniffed data you can decode messages. But when your scope is only to protect for a relatively short time, it is good enough. This is out of scope of the current ExpressLRS project, but can probably added without too much trouble. The question whether this makes sense is a completely different one though. This does not prevent the other side from just putting up a big ass transmitter and blocking your signal. |
|
Talking about explosive payloads and "Marwel movie plots", if you watch this video from Ukrainian armed forces unit that uses FPV drones in combat, but also develops radioelectronic warfare, in this video they somehow manage to force enemy drone to go up full throttle till it runs out of battery. It's too little information in the video, we don't even know if enemy drone uses ELRS 900 or Crossfire. But chances are that exactly this "vulnerability" is exploited in the actual war. https://youtu.be/Y3qdyGH8jdI timestamp 05:50 But yep, this is rather an exception. In 99% cases infantry has a portative (~5-10kg) jammer that just emits 50+ watt of noise to the whole 700-1200mHz frequency range |
|
This kind of vulnerability isn't only a "take over your drone and steal it" one-in-a-million scenario, but serious safety implications where a bad actor could easily crash destroy anyone's drone and cause thousands of dollars of proprety damage or even injury, or worst case scenario, causes every drone in an event or race to go out of control and rocket full throttle in random directions without anyone being able to stop it. This was not imaginable in the good old days, but today the world is full of people trying to shut down everything they don't like. I couldn't imagine not having a password and authentification on my wifi; on a potentially dangerous remotely controlled flying machine it's simply inconceivable. |
Replaces random number generator with a better performing version.
Uses hash of UID to confirm that RX and TX share the same UID instead of the UID itself.