helium / miner

Miner for the helium blockchain
Apache License 2.0
609 stars 266 forks source link

Miner allows PUSH_DATA packets from multiple lora gateways #590

Closed PaulVMo closed 3 years ago

PaulVMo commented 3 years ago

Currently, all hotspots on the Helium network are comprised of one miner and one lora gateway in a one-to-one mapping. However, the Lora server implementation in the Helium miner allows PUSH_DATA packets from multiple Lora gateways and will process them as PoC or data packets. I believe this could be potentially used to game PoC by having multiple gateways or multiple middlemen sending packets to a single miner.

PULL_RESPs (transmitting a packet) on the other hand are sent to a single gateway. The gateway to send packets to is selected by looking at the head of the gateway list which end up being the first gateway that sent a pull data request since the miner was up. I believe similar logic should be used to filter out PUSH_DATA packets sent by gateways.

ke6jjj commented 3 years ago

I would argue that this is a feature that should be retained. It has legitimate uses that we may wish to promote for more advanced setups. For examples, I would suggest that you speak with Para1, aka. Carniverous19, author of HIPs 15 & 17, who has just a setup which uses this feature and it is quite noteworthy and commendable. Just because something could be used by a "gaming cluster" does not implicate it as a flaw.

PaulVMo commented 3 years ago

@ke6jjj All of the hotspots currently available on the market consist of one lora packet forwarder and one miner. There is no reason to have more than one lora gateway / packet forwarder per miner. This was not the intent. In fact, other parts of the code actively prevent multiple lora gateways. This is a bug and should be closed.

ke6jjj commented 3 years ago

There are legitimate reasons to have more than one gateway per miner and while your proposal insinuates that there are illegitimate reasons to do so, you have not actually articulated one. I will articulate a legitimate use:

Having multiple, focused, reception-only gateways, each of which is attached to an advanced antenna such as a Yagi-Uda, allows a miner operator to provide excellent device service for an area. This is the primary goal of the network and should not be hindered. Your proposal suggests that leaving the code "as is" leaves it vulnerable to exploitation in an unsavory manner. But this is easily disproven.

The code, as it stands, does not serve the interests of a gaming cluster in the manner you suggest. This is because the miner on a stock hotspot is not accessible to external gateways without advanced modification. The miner process binds its listening activity to localhost, and is thus completely deaf to packets sent from external gateways. To enable multiple gateway support one would need to modify the miner configuration at startup.

But this kind of access is exactly the kind that would also allow a user to completely subvert the very change you are proposing here. Anyone in a position to "use" this "bug" is also necessarily in a position to revert it.

Thus, the only people you will be harming are those who are legitimately trying to craft advanced reception setups. Your proposal would be nothing but a speed bump to someone who wishes to defeat it.

If you can craft a scenario where an illegitimate use is enhanced by this feature, it would be great to hear it.

maco2035 commented 3 years ago

@PaulVMo I would ask you to read page 5 of this document. This explains the network flow. I think if you remove PUSH_DATA there will be issues. As @ke6jjj said, to use this you have a trivial way to get around with this, and people who are doing it already can do it. Thus patching this will introduce problems, and make the codebase harder to maintain.

LoRa gateway to network server interface definition.pdf

Carniverous19 commented 3 years ago

So two different things.

First the miner will accept data from multiple packet forwarders, I believe it will accept all types of messages, PUSH_DATA, PULL_DATA, and the various ACK's. For PUSH_DATA this will work fine but getting PULL_DATA from multiple gateways could mess up the functionality of the miner / hotspots. I believe the PULL_RESP, which is a command from miner to gateway about packets that should be transmitted, will be sent to the IP/port of the last PULL_DATA received. This means the gateway the packet will be transmitted out of will change. What could cause more trouble is the 32-bit 1uS clock is unlikely to be in sync between the multiple gateways pointed at a miner. This may cause transmit commands to be dropped with an error of "too early" or "too late" and even if transmitted may be transmitted at the wrong time. This could cause device JOINs to fail.

To get around these problems I use the middleman software that specifically transmits out of a single gateway regardless of which one received the payload and (attempts to) synchronize the 1uS clocks between multiple gateways.

The second question is how to enforce single-gateway to single-miner if that is desired . You could drop all UDP packets not from the IP address of the first valid semtech packet received. This could work but may run into issues if the IP of the gateway changes (say it reboots and gets a new DHCP lease). You could have a config file for the miner where you have to specify a gateway IP, this could run into the same issue. Lastly you could only accept payloads from the first MAC address (from semtech protocol, not the network card) it receives a valid payload from. The MAC address of the gateway would be the same even if the IP changes. Now the MAC address is specified in the packet_forwarder config file so nothing is stopping someone from assigning the same MAC address to multiple gateways bypassing this filter.

The problem is none of these are really sufficient to prevent someone who wants to connect multiple gateways to a miner from doing so. The middleman software already bypasses all of these ways of enforcing single gateway to single miner. So if someone wants to connect multiple gateways to a miner there is no stopping that as long as the Semtech packet forwarder is supported.

Since any method of enforcement is easily bypassed by publicly available software and those interfacing directly with the miner software have some level of sophistication, maybe its best to assume those interfacing with the miner will point to a single gateway? Or maybe this can be addressed in documentation just saying "miner is meant to be connected to a single gateway. Pointing multiple gateways at a miner may cause undesirable behavior..."

Note: I see multiple responses after I started drafting, but yea, at the end of the day its a lost cause to prevent multiple gateways from sending data to the miner if they want to.

ke6jjj commented 3 years ago

I chatted with @maco2035 on Discord and he and I agree he misunderstood the proposal a little. When he wrote this,

PaulVMo I would ask you to read page 5 of this document. This explains the network flow. I think if you remove PUSH_DATA there will be issues.

He assumed that you were proposing to eliminate PUSH DATA in general. This of course was not what you were proposing @PaulVMo, so I hope we can ignore that comment for now.

PaulVMo commented 3 years ago

Far enough that it is easily worked around if someone what. I was merely pointing it out something that I believed was possibly an unintended design. As I said, other aspects of the code are currently designed to work one gateway to one miner.

@Carniverous19 the PULL_RESP is always sent to the FIRST gateway that send a PULL_DATA to the miner, not the last one. Which is what inspired me to think that the intent was to limit it to a single gateway. If the IP address of the lora gateway changes, the miner needs to be restarted anyway otherwise transmissions will never work.

BTW, we are talking about IPs changing and multiple lora gateways, none of which are currently supported by Helium. The only definition of hotspots are devices with the packet forwarder and miner running on the same equipment - one to one. I understand people have leveraged this to make more "sophisticated" setups, but until DIY is officially supported, I believe the Helium team ought to be locking things down as much as possible as people will only find more creative ways to game PoC.

I realize it can be bypassed if someone knows what they are doing. So, I'm not going to dying on my sword on this one and will leave it at this.

Carniverous19 commented 3 years ago

Interesting, I didnt realize they locked in the PULL_RESP address to the first PULL_DATA packet they get. I think the last would be better in case the devices are split, I believe that's how middleman handles it but its been a while.

It will also be interesting to see what the gateway-rs interface is for semtech packet forwarders and how this changes (if at all). In general I think any use of the semtech packet forwarder will be fairly insecure as the packet forwarder is not really meant for production anyway as Semtech says themselves:

The protocol between the gateway and the server is purposefully very basic and for demonstration purpose only, or for use on private and reliable networks.

Of course pretty much everybody uses this as production ready software even outside of Helium so it is what it is.

ke6jjj commented 3 years ago

@PaulVMo I do agree with you on one point:

The gateway to send packets to is selected by looking at the head of the gateway list which end up being the first gateway that sent a pull data request since the miner was up.

This is clearly a bug as it causes arbitrary transmit behavior.

PaulVMo commented 3 years ago

@ke6jjj

This is clearly a bug as it causes arbitrary transmit behavior.

Again, depends on your perspective. The current Hotspot definition is one lora packet forward connected to one miner on the same host. Thus, imagining this from the Helium team's perspective, I would not be concerned with supporting more than one lora gateway in the miner.

I would assume that fully supporting multiple lora gateways was future work (you can see some comments around it in the code) that depends on permissionless DIY. Given that dependency, there is no urgency in fixing the miner to allow more than one. And, in the meantime, might not be a bad idea to lock it down more - although as stated above, it is easily worked around with the middleman code...

madninja commented 3 years ago

Having multiple packet forwarders pointed at the same miner is not correct behavior and (while possible) will cause the miner to report mixed witnesses which will cause most of them to get invalidated.

I'm going to close this issue since the plan is to move towards light gateways and validators which should avoid having to have a miner per gateway.

lthiery commented 3 years ago

It will also be interesting to see what the gateway-rs interface is for semtech packet forwarders and how this changes (if at all).

@Carniverous19 FWIW, gateway-rs depends on this library for the Semtech GWMP over UDP. The example here may be helpful in seeing the interface, but essentially, multiple clients are supported as long as their MAC addresses are distinct. Further discussion on whether that's a good thing should probably happen over on light gateway.