CVE-2020-28349: Improper Input Validation (CWE-20)
value: An inaccurate frame deduplication process in ChirpStack Network Server 3.9.0 allows a malicious gateway to perform uplink Denial of Service via malformed frequency attributes in CollectAndCallOnceCollect in internal/uplink/collect.go. NOTE: the vendor's position is that there are no "guarantees that allowing untrusted LoRa gateways to the network should still result in a secure network.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 6.5
Published: 2020-11-09T01:15Z
Repository: https://github.com/brocaar/chirpstack-network-server
Language: Go
Files changed: internal/backend/gateway/mqtt/backend.go, internal/uplink/collect.go
Commit date: 2020-08-31 15:28:32 +0100
Commit message: Improve uplink de-duplication.

This removes the de-duplication at the MQTT backend. Some MQTT brokers
provide shared subscription options in which case this additional
de-duplication is not needed.

For MQTT brokers that do not support shared subscriptions, the
collectAndCallOnce function will still filter out duplicated uplinks.

By adding the TXInfo object (hex encoded) to the de-duplication key,
uplink messages received on multiple channels will be handled
separately as they will result in different de-duplication keys.

Diff: diff --git a/internal/backend/gateway/mqtt/backend.go b/internal/backend/gateway/mqtt/backend.go
index 7436bd2..6e7b328 100644
--- a/internal/backend/gateway/mqtt/backend.go
+++ b/internal/backend/gateway/mqtt/backend.go
@@ -3,11 +3,10 @@ package mqtt
 import (
 	"bytes"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
-	"encoding/hex"
 	"fmt"
 	"io/ioutil"
 	"strings"
 	"sync"
 	"text/template"
@@ -247,24 +246,10 @@ func (b *Backend) rxPacketHandler(c paho.Client, msg paho.Message) {
 	log.WithFields(log.Fields{
 		"uplink_id":  uplinkID,
 		"gateway_id": gatewayID,
 	}).Info("gateway/mqtt: uplink frame received")
 
-	// Since with MQTT all subscribers will receive the uplink messages sent
-	// by all the gateways, the first instance receiving the message must lock it,
-	// so that other instances can ignore the same message (from the same gw).
-	key := fmt.Sprintf("lora:ns:uplink:lock:%s:%d:%d:%d:%s", gatewayID, uplinkFrame.TxInfo.Frequency, uplinkFrame.RxInfo.Board, uplinkFrame.RxInfo.Antenna, hex.EncodeToString(uplinkFrame.PhyPayload))
-	if locked, err := b.isLocked(key); err != nil || locked {
-		if err != nil {
-			log.WithError(err).WithFields(log.Fields{
-				"uplink_id": uplinkID,
-				"key":       key,
-			}).Error("gateway/mqtt: acquire lock error")
-		}
-		return
-	}
-
 	b.rxPacketChan <- uplinkFrame
 }
 
 func (b *Backend) statsPacketHandler(c paho.Client, msg paho.Message) {
 	b.wg.Add(1)
diff --git a/internal/uplink/collect.go b/internal/uplink/collect.go
index 1612109..f70d7d7 100644
--- a/internal/uplink/collect.go
+++ b/internal/uplink/collect.go
@@ -17,12 +17,12 @@ import (
 	"github.com/brocaar/lorawan"
 )
 
 // Templates used for generating Redis keys
 const (
-	CollectKeyTempl     = "lora:ns:rx:collect:%s"
-	CollectLockKeyTempl = "lora:ns:rx:collect:%s:lock"
+	CollectKeyTempl     = "lora:ns:rx:collect:%s:%s"
+	CollectLockKeyTempl = "lora:ns:rx:collect:%s:%s:lock"
 )
 
 // collectAndCallOnce collects the package, sleeps the configured duraction and
 // calls the callback only once with a slice of packets, sorted by signal
 // strength (strongest at index 0). This method exists since multiple gateways
@@ -31,12 +31,18 @@ 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 {
 	phyKey := hex.EncodeToString(rxPacket.PhyPayload)
-	key := fmt.Sprintf(CollectKeyTempl, phyKey)
-	lockKey := fmt.Sprintf(CollectLockKeyTempl, phyKey)
+	txInfoB, err := proto.Marshal(rxPacket.TxInfo)
+	if err != nil {
+		return errors.Wrap(err, "marshal protobuf error")
+	}
+	txInfoHEX := hex.EncodeToString(txInfoB)
+
+	key := fmt.Sprintf(CollectKeyTempl, txInfoHEX, phyKey)
+	lockKey := fmt.Sprintf(CollectLockKeyTempl, txInfoHEX, phyKey)
 
 	// this way we can set a really low DeduplicationDelay for testing, without
 	// the risk that the set already expired in redis on read
 	deduplicationTTL := deduplicationDelay * 2
 	if deduplicationTTL < time.Millisecond*200 {

Vulnerable code: 	"encoding/hex"
	// Since with MQTT all subscribers will receive the uplink messages sent
	// by all the gateways, the first instance receiving the message must lock it,
	// so that other instances can ignore the same message (from the same gw).
	key := fmt.Sprintf("lora:ns:uplink:lock:%s:%d:%d:%d:%s", gatewayID, uplinkFrame.TxInfo.Frequency, uplinkFrame.RxInfo.Board, uplinkFrame.RxInfo.Antenna, hex.EncodeToString(uplinkFrame.PhyPayload))
	if locked, err := b.isLocked(key); err != nil || locked {
		if err != nil {
			log.WithError(err).WithFields(log.Fields{
				"uplink_id": uplinkID,
				"key":       key,
			}).Error("gateway/mqtt: acquire lock error")
		}
		return
	}

	CollectKeyTempl     = "lora:ns:rx:collect:%s"
	CollectLockKeyTempl = "lora:ns:rx:collect:%s:lock"
	key := fmt.Sprintf(CollectKeyTempl, phyKey)
	lockKey := fmt.Sprintf(CollectLockKeyTempl, phyKey)
Fixed code: 	CollectKeyTempl     = "lora:ns:rx:collect:%s:%s"
	CollectLockKeyTempl = "lora:ns:rx:collect:%s:%s:lock"
	txInfoB, err := proto.Marshal(rxPacket.TxInfo)
	if err != nil {
		return errors.Wrap(err, "marshal protobuf error")
	}
	txInfoHEX := hex.EncodeToString(txInfoB)

	key := fmt.Sprintf(CollectKeyTempl, txInfoHEX, phyKey)
	lockKey := fmt.Sprintf(CollectLockKeyTempl, txInfoHEX, phyKey)