Skip to content

Commit

Permalink
Add ignore_frequency_for_deduplication setting as a stopgap for curre…
Browse files Browse the repository at this point in the history 
…nt data loss being experienced

It defaults to false. If set to true, it will ignore the frequency on which the packets come in from the gateways. This allows a ghost packet to be gracefully collected as Just Another Packet to de-duplicate with.

Without this setting a ghost package gets in and overrides an established `dev_addr`. Leading to data being lost until the next JOIN request, with the edge device unaware that it has lost its JOIN status.

We have not been able to trace where the ghost JOINs come from. This stops those from being a problem for now.

- brocaar#557 (comment) is not what we are experiencing, our devices are far apart
- This fixes brocaar#566 for us
  • Loading branch information
@danieroux
danieroux committed Apr 26, 2022
1 parent d2fd1f4 commit 8786343
Show file tree
Hide file tree
Showing 5 changed files with 187 additions and 27 deletions.
6 changes: 6 additions & 0 deletions cmd/chirpstack-network-server/cmd/configfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ net_id="{{ .NetworkServer.NetID }}"
# unable to respond to the device within its receive-window.
deduplication_delay="{{ .NetworkServer.DeduplicationDelay }}"
# For packet de-duplication, ignore what frequency the packet came in on
#
# This is a work around. And a security issue:
# https://github.com/brocaar/chirpstack-network-server/issues/557#issuecomment-968719234
ignore_frequency_for_deduplication="{{ .NetworkServer.IgnoreFrequencyForDeduplication }}"
# Device session expiration.
#
# The TTL value defines the time after which a device-session expires
Expand Down
11 changes: 6 additions & 5 deletions internal/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ type Config struct {
} `mapstructure:"redis"`

NetworkServer struct {
NetID lorawan.NetID
NetIDString string `mapstructure:"net_id"`
DeduplicationDelay time.Duration `mapstructure:"deduplication_delay"`
DeviceSessionTTL time.Duration `mapstructure:"device_session_ttl"`
GetDownlinkDataDelay time.Duration `mapstructure:"get_downlink_data_delay"`
NetID lorawan.NetID
NetIDString string `mapstructure:"net_id"`
DeduplicationDelay time.Duration `mapstructure:"deduplication_delay"`
IgnoreFrequencyForDeduplication bool `mapstructure:"ignore_frequency_for_deduplication"`
DeviceSessionTTL time.Duration `mapstructure:"device_session_ttl"`
GetDownlinkDataDelay time.Duration `mapstructure:"get_downlink_data_delay"`

Band struct {
Name band.Name `mapstructure:"name"`
Expand Down
41 changes: 37 additions & 4 deletions internal/uplink/collect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ const (
// It is safe to collect the same packet received by the same gateway twice.
// Since the underlying storage type is a set, the result will always be a
// unique set per gateway MAC and packet MIC.
func collectAndCallOnce(rxPacket gw.UplinkFrame, callback func(packet models.RXPacket) error) error {
func collectAndCallOnce(rxPacket gw.UplinkFrame, ignoreFrequencyForDeduplication bool, callback func(packet models.RXPacket) error) error {
phyKey := hex.EncodeToString(rxPacket.PhyPayload)
txInfoB, err := proto.Marshal(rxPacket.TxInfo)

txInfoHEX, err := makeTXInfoHex(rxPacket, ignoreFrequencyForDeduplication)
if err != nil {
return errors.Wrap(err, "marshal protobuf error")
return errors.Wrap(err, "error making txinfoHEX for redis key")
}
txInfoHEX := hex.EncodeToString(txInfoB)

key := storage.GetRedisKey(CollectKeyTempl, txInfoHEX, phyKey)
lockKey := storage.GetRedisKey(CollectLockKeyTempl, txInfoHEX, phyKey)
Expand Down Expand Up @@ -113,6 +113,39 @@ func collectAndCallOnce(rxPacket gw.UplinkFrame, callback func(packet models.RXP
return callback(out)
}

func makeTXInfoHex(rxPacket gw.UplinkFrame, ignoreFrequencyForDeduplication bool) (string, error) {
if ignoreFrequencyForDeduplication {
return deduplicateOnTXInfoClearingFrequency(rxPacket)
} else {
return deduplicateOnTXInfo(rxPacket)
}
}

func deduplicateOnTXInfo(rxPacket gw.UplinkFrame) (string, error) {
return txInfoHex(rxPacket.TxInfo)
}

// "The reason why the de-duplication logic includes the frequency in its key is because
// there was a security issue reported a while ago, which would allow a re-play with a
// better RSSI / SNR to "steal" the downlink path"
// - Orne, https://github.com/brocaar/chirpstack-network-server/issues/557#issuecomment-968719234
//
// To work around current data loss being experience, we take this chance.
func deduplicateOnTXInfoClearingFrequency(rxPacket gw.UplinkFrame) (string, error) {
cloned := proto.Clone(rxPacket.TxInfo).(*gw.UplinkTXInfo)
cloned.Frequency = 0

return txInfoHex(cloned)
}

func txInfoHex(uplinkTXInfo *gw.UplinkTXInfo) (string, error) {
txInfoB, err := proto.Marshal(uplinkTXInfo)
if err != nil {
return "", errors.Wrap(err, "marshal protobuf error")
}
return hex.EncodeToString(txInfoB), nil
}

func collectAndCallOncePut(key string, ttl time.Duration, rxPacket gw.UplinkFrame) error {
b, err := proto.Marshal(&rxPacket)
if err != nil {
Expand Down
146 changes: 130 additions & 16 deletions internal/uplink/collect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package uplink

import (
"context"
"encoding/hex"
"github.com/golang/protobuf/proto"
"github.com/stretchr/testify/assert"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -35,6 +38,39 @@ func (ts *CollectTestSuite) SetupSuite() {
assert.NoError(Setup(conf))
}

func TestGetKeyWithFrequencySetToZero(t *testing.T) {
originalFreq := uint32(868100000)

uplinkFrame := gw.UplinkFrame{
TxInfo: &gw.UplinkTXInfo{
Frequency: originalFreq,
Modulation: common.Modulation_LORA,
ModulationInfo: &gw.UplinkTXInfo_LoraModulationInfo{
LoraModulationInfo: &gw.LoRaModulationInfo{
Bandwidth: 125,
SpreadingFactor: 7,
CodeRate: "3/4",
},
},
},
}

zeroFreqKey, err := deduplicateOnTXInfoClearingFrequency(uplinkFrame)

// Original frame freq remains unchanged
assert.Equal(t, uplinkFrame.TxInfo.Frequency, originalFreq)

assert.NoError(t, err)
assert.NotNil(t, zeroFreqKey)

txInfob, err := hex.DecodeString(zeroFreqKey)
assert.NoError(t, err)

var txInfo gw.UplinkTXInfo
proto.Unmarshal(txInfob, &txInfo)
assert.Equal(t, uint32(0), txInfo.Frequency)
}

func (ts *CollectTestSuite) TestHandleRejectedUplinkFrameSet() {
testController := test.NewNetworkControllerClient()
assert := require.New(ts.T())
Expand Down Expand Up @@ -82,11 +118,17 @@ func (ts *CollectTestSuite) TestHandleRejectedUplinkFrameSet() {
}

func (ts *CollectTestSuite) TestDeduplication() {
type Gateways struct {
eui64 lorawan.EUI64
frequencyUsed uint32
}

testTable := []struct {
Name string
PHYPayload lorawan.PHYPayload
Gateways []lorawan.EUI64
Count int
Name string
PHYPayload lorawan.PHYPayload
Gateways []Gateways
Count int
IgnoreFreqForDeDup bool
}{
{
"single item expected",
Expand All @@ -98,10 +140,14 @@ func (ts *CollectTestSuite) TestDeduplication() {
MIC: [4]byte{1, 2, 3, 4},
MACPayload: &lorawan.MACPayload{},
},
[]lorawan.EUI64{
{1, 1, 1, 1, 1, 1, 1, 1},
[]Gateways{
{
eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
frequencyUsed: 868100000,
},
},
1,
false,
}, {
"two items expected",
lorawan.PHYPayload{
Expand All @@ -112,11 +158,18 @@ func (ts *CollectTestSuite) TestDeduplication() {
MIC: [4]byte{2, 2, 3, 4},
MACPayload: &lorawan.MACPayload{},
},
[]lorawan.EUI64{
{2, 1, 1, 1, 1, 1, 1, 1},
{2, 2, 2, 2, 2, 2, 2, 2},
[]Gateways{
{
eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
frequencyUsed: 868100000,
},
{
eui64: lorawan.EUI64{2, 2, 2, 2, 2, 2, 2, 2},
frequencyUsed: 868100000,
},
},
2,
false,
}, {
"two items expected (three collected)",
lorawan.PHYPayload{
Expand All @@ -127,13 +180,70 @@ func (ts *CollectTestSuite) TestDeduplication() {
MIC: [4]byte{3, 2, 3, 4},
MACPayload: &lorawan.MACPayload{},
},
[]lorawan.EUI64{
{3, 1, 1, 1, 1, 1, 1, 1},
{3, 2, 2, 2, 2, 2, 2, 2},
{3, 2, 2, 2, 2, 2, 2, 2},
[]Gateways{
{
eui64: lorawan.EUI64{3, 1, 1, 1, 1, 1, 1, 1},
frequencyUsed: 868100000,
},
{
eui64: lorawan.EUI64{3, 2, 2, 2, 2, 2, 2, 2},
frequencyUsed: 868100000,
},
{
eui64: lorawan.EUI64{3, 2, 2, 2, 2, 2, 2, 2},
frequencyUsed: 868100000,
},
},
2,
false,
},
{
"two items expected (two collected from same gateway, different frequencies, but ignoring that in dedup. Called once only.)",
lorawan.PHYPayload{
MHDR: lorawan.MHDR{
MType: lorawan.UnconfirmedDataUp,
Major: lorawan.LoRaWANR1,
},
MIC: [4]byte{3, 2, 3, 4},
MACPayload: &lorawan.MACPayload{},
},
[]Gateways{
{
eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
frequencyUsed: 868100000,
},
{
eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
frequencyUsed: 868300000,
},
},
2,
true,
},
// https://github.com/brocaar/chirpstack-network-server/issues/557#issuecomment-968719234
//{
// "The unexpected/expected case. This will call *twice*, and fail",
// lorawan.PHYPayload{
// MHDR: lorawan.MHDR{
// MType: lorawan.UnconfirmedDataUp,
// Major: lorawan.LoRaWANR1,
// },
// MIC: [4]byte{3, 2, 3, 4},
// MACPayload: &lorawan.MACPayload{},
// },
// []Gateways{
// {
// eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
// frequencyUsed: 868100000,
// },
// {
// eui64: lorawan.EUI64{1, 1, 1, 1, 1, 1, 1, 1},
// frequencyUsed: 868300000,
// },
// },
// 1,
// false,
//},
}

for _, tst := range testTable {
Expand All @@ -152,7 +262,8 @@ func (ts *CollectTestSuite) TestDeduplication() {

var wg sync.WaitGroup
for i := range tst.Gateways {
g := tst.Gateways[i]
g := tst.Gateways[i].eui64
freq := tst.Gateways[i].frequencyUsed
phyB, err := tst.PHYPayload.MarshalBinary()
assert.NoError(err)

Expand All @@ -161,18 +272,21 @@ func (ts *CollectTestSuite) TestDeduplication() {
RxInfo: &gw.UplinkRXInfo{
GatewayId: g[:],
},
TxInfo: &gw.UplinkTXInfo{},
TxInfo: &gw.UplinkTXInfo{
Frequency: freq,
},
PhyPayload: phyB,
}
assert.NoError(helpers.SetUplinkTXInfoDataRate(packet.TxInfo, 0, band.Band()))

go func(packet gw.UplinkFrame) {
assert.NoError(collectAndCallOnce(packet, cb))
assert.NoError(collectAndCallOnce(packet, tst.IgnoreFreqForDeDup, cb))
wg.Done()
}(packet)
}
wg.Wait()

// collectAndCallOnce
assert.Equal(1, called)
assert.Equal(tst.Count, received)
})
Expand Down
10 changes: 8 additions & 2 deletions internal/uplink/uplink.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ import (
)

var (
deduplicationDelay time.Duration
deduplicationDelay time.Duration
ignoreFrequencyForDeduplication bool
)

// Setup configures the package.
Expand All @@ -52,6 +53,11 @@ func Setup(conf config.Config) error {

deduplicationDelay = conf.NetworkServer.DeduplicationDelay

ignoreFrequencyForDeduplication = conf.NetworkServer.IgnoreFrequencyForDeduplication
if ignoreFrequencyForDeduplication {
log.Warn("Ignoring the frequency of the packets when deduplicating: This introduces a security issue that enables a re-play attack - https://github.com/brocaar/chirpstack-network-server/issues/557#issuecomment-968719234")
}

return nil
}

Expand Down Expand Up @@ -157,7 +163,7 @@ func HandleDownlinkTXAcks(wg *sync.WaitGroup) {
}

func collectUplinkFrames(ctx context.Context, uplinkFrame gw.UplinkFrame) error {
return collectAndCallOnce(uplinkFrame, func(rxPacket models.RXPacket) error {
return collectAndCallOnce(uplinkFrame, ignoreFrequencyForDeduplication, func(rxPacket models.RXPacket) error {
err := handleCollectedUplink(ctx, uplinkFrame, rxPacket)
if err != nil {
cause := errors.Cause(err)
Expand Down

0 comments on commit 8786343

Please sign in to comment.