Repository: markus-wa/demoinfocs-golang (go)

Issue:
Parser errors on encrypted net-message when decryption key is provided
**Describe the bug**

Hello!

Some Valve demos can lead to an error when parsing them with the new `NetMessageDecryptionKey` property added in the last version.
The error occurs in the `CSVCMsg_EncryptedData` [handler](https://github.com/markus-wa/demoinfocs-golang/blob/master/pkg/demoinfocs/net_messages.go#L80) because it's trying to read out of the bit reader boundaries.

**To Reproduce**

This [zip archive](https://mega.nz/file/XAkj0Krb#PxI0ci6TIIUL2HF5txdtOPGPgwp6-FVTpeYok62t7OA) contains 2 affected demos.

Code:

Parsing one of the attached demos using the snippet from the [last release](https://github.com/markus-wa/demoinfocs-golang/releases/tag/v2.13.0) should trigger a panic.

**Expected behavior**

I think errors occurring while reading an encrypted data message should not prevent a demo parsing.
It's supposed to be private and may change in the future.

**Library version**
2.13.0

**Additional context**

I'm currently using my own implementation to handle encrypted messages with `AdditionalNetMessageCreators` but since this project now support it, I would like to use it and remove unnecessary code 😄 

I looked into a possible solution but the `gobitread` package doesn't expose a public function to know how many reading bytes are available which prevent me to ignore a corrupted message easily.

What I tried:

```diff
+++ b/pkg/demoinfocs/net_messages.go
@@ -98,6 +98,10 @@ func (p *parser) handleEncryptedData(msg *msg.CSVCMsg_EncryptedData) {
 	br := bit.NewSmallBitReader(r)
 
 	paddingBytes := br.ReadSingleByte()
+	messageSize := len(b)
+	if 1+int(paddingBytes) > messageSize {
+		return
+	}
 	br.Skip(int(paddingBytes) << 3)
 
 	bBytesWritten := br.ReadBytes(4)
@@ -106,8 +110,16 @@ func (p *parser) handleEncryptedData(msg *msg.CSVCMsg_EncryptedData) {
 	unassert.Same(len(b), 5+int(paddingBytes)+nBytesWritten)
 
 	cmd := br.ReadVarInt32()
+
+	if messageSize-nBytesWritten-int(paddingBytes)-5 != 0 {
+		return
+	}
+
 	size := br.ReadVarInt32()
 
+	if nBytesWritten-2 != int(size) {
+		return
+	}
 	m := p.netMessageForCmd(int(cmd))
 
 	if m == nil {
```

But it's not enough for the demo `match730_003449478367177343081_1946274414_112.dem`, I think it would easier to know how many bytes are available before actually reading it  to prevent an error or maybe you have an other idea?

If that can help, here is the code of my own handler, I'm checking if reading bytes is safe before actually doing it at each step:
Note: `reader` is a GO `bytes.Reader` small wrapper, let me know if you need more details.

```go
parser.RegisterNetMessageHandler(func(m *msg.CSVCMsg_EncryptedData) {
	isPublicKey := m.KeyType == encryptedKeyTypePublic
	if !isPublicKey || analyzer.iceKey == nil {
		return
	}

	messageSize := len(m.Encrypted)
	isMessageMalFormed := messageSize%analyzer.iceKey.BlockSize() != 0
	if isMessageMalFormed {
		return
	}

	plaintext := make([]byte, messageSize)
	analyzer.iceKey.DecryptFullArray(m.Encrypted, plaintext)

	reader := bytesreader.NewBytesReader(plaintext)
	paddingBytes := reader.ReadUInt8()
	if 1+paddingBytes > uint8(messageSize) {
		return
	}
	reader.Skip(int64(paddingBytes))
	if reader.Remaining() <= 4 {
		return
	}
	bytesWritten := reader.ReadInt32BE()
	if reader.Remaining() != int(bytesWritten) {
		return
	}

	cmd := reader.ReadVarInt32()
	if cmd == uint32(msg.SVC_Messages_svc_UserMessage) {
		size := reader.ReadVarInt32()
		if reader.Remaining() != int(size) {
			return
		}
		currentOffset := reader.CurrentOffset()
		userMessage := new(msg.CSVCMsg_UserMessage)
		proto.Unmarshal(plaintext[currentOffset:], userMessage)
		switch userMessage.MsgType {
		case int32(msg.ECstrike15UserMessages_CS_UM_SayText2):
                        //  Chat message here
		}
	}
})
```

Thank you!


Pull request:
raise ParserWarn instead of error on bad net-message decryption key
fixes #326

Patch:
diff --git a/examples/encrypted-net-messages/enc_net_msg.go b/examples/encrypted-net-messages/enc_net_msg.go
index 0b87b52..a04a4ed 100644
--- a/examples/encrypted-net-messages/enc_net_msg.go
+++ b/examples/encrypted-net-messages/enc_net_msg.go
@@ -47,6 +47,10 @@ func main() {
 
 	p := dem.NewParserWithConfig(f, cfg)
 
+	p.RegisterEventHandler(func(warn events.ParserWarn) {
+		log.Println("WARNING:", warn.Message)
+	})
+
 	p.RegisterEventHandler(func(message events.ChatMessage) {
 		log.Println(message)
 	})
diff --git a/pkg/demoinfocs/events/events.go b/pkg/demoinfocs/events/events.go
index a5a3412..3655d37 100644
--- a/pkg/demoinfocs/events/events.go
+++ b/pkg/demoinfocs/events/events.go
@@ -523,9 +523,14 @@ const (
 	WarnTypeTeamSwapPlayerNil          // TODO: figure out why this happens
 	WarnTypeGameEventBeforeDescriptors // may occur in POV demos
 
-	// WarnTypeMissingNetMessageDecryptionKey occurs when encrypted net-messages are encountered and the decryption key is missing
+	// WarnTypeMissingNetMessageDecryptionKey occurs when encrypted net-messages are encountered and the decryption key is missing.
 	// See ParserConfig.NetMessageDecryptionKey
 	WarnTypeMissingNetMessageDecryptionKey
+
+	// WarnTypeCantReadEncryptedNetMessage occurs when an encrypted net-messages can't be decrypted even though the decryption key is set.
+	// May occur because the decryption key used is incorrect.
+	// See ParserConfig.NetMessageDecryptionKey
+	WarnTypeCantReadEncryptedNetMessage
 )
 
 // ParserWarn signals that a non-fatal problem occurred during parsing.
diff --git a/pkg/demoinfocs/net_messages.go b/pkg/demoinfocs/net_messages.go
index 6be432f..c1b0670 100644
--- a/pkg/demoinfocs/net_messages.go
+++ b/pkg/demoinfocs/net_messages.go
@@ -5,7 +5,6 @@ import (
 	"encoding/binary"
 
 	"github.com/gogo/protobuf/proto"
-	"github.com/markus-wa/go-unassert"
 	"github.com/markus-wa/ice-cipher-go/pkg/ice"
 
 	bit "github.com/markus-wa/demoinfocs-golang/v2/internal/bitread"
@@ -97,13 +96,35 @@ func (p *parser) handleEncryptedData(msg *msg.CSVCMsg_EncryptedData) {
 	r := bytes.NewReader(b)
 	br := bit.NewSmallBitReader(r)
 
+	const (
+		byteLenPadding = 1
+		byteLenWritten = 4
+	)
+
 	paddingBytes := br.ReadSingleByte()
+
+	if int(paddingBytes) >= len(b)-byteLenPadding-byteLenWritten {
+		p.eventDispatcher.Dispatch(events.ParserWarn{
+			Message: "encrypted net-message has invalid number of padding bytes",
+			Type:    events.WarnTypeCantReadEncryptedNetMessage,
+		})
+
+		return
+	}
+
 	br.Skip(int(paddingBytes) << 3)
 
 	bBytesWritten := br.ReadBytes(4)
 	nBytesWritten := int(binary.BigEndian.Uint32(bBytesWritten))
 
-	unassert.Same(len(b), 5+int(paddingBytes)+nBytesWritten)
+	if len(b) != byteLenPadding+byteLenWritten+int(paddingBytes)+nBytesWritten {
+		p.eventDispatcher.Dispatch(events.ParserWarn{
+			Message: "encrypted net-message has invalid length",
+			Type:    events.WarnTypeCantReadEncryptedNetMessage,
+		})
+
+		return
+	}
 
 	cmd := br.ReadVarInt32()
 	size := br.ReadVarInt32()


Test patch:
diff --git a/examples/encrypted-net-messages/enc_net_msg_test.go b/examples/encrypted-net-messages/enc_net_msg_test.go
index 80c3562..ae350f5 100644
--- a/examples/encrypted-net-messages/enc_net_msg_test.go
+++ b/examples/encrypted-net-messages/enc_net_msg_test.go
@@ -15,3 +15,14 @@ func TestEncryptedNetMessages(t *testing.T) {
 
 	main()
 }
+
+// Make sure it doesn't error / crash
+func TestEncryptedNetMessages_BadKey(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping test")
+	}
+
+	os.Args = []string{"cmd", "-demo", "../../test/cs-demos/match730_003528806449641685104_1453182610_271.dem", "-info", "../../test/cs-demos/match730_003449478367177343081_1946274414_112.dem.info"}
+
+	main()
+}
diff --git a/pkg/demoinfocs/demoinfocs_test.go b/pkg/demoinfocs/demoinfocs_test.go
index 1375371..fe13fe0 100644
--- a/pkg/demoinfocs/demoinfocs_test.go
+++ b/pkg/demoinfocs/demoinfocs_test.go
@@ -282,6 +282,51 @@ func TestUnexpectedEndOfDemo(t *testing.T) {
 	assert.Equal(t, demoinfocs.ErrUnexpectedEndOfDemo, err, "parsing cancelled but error was not ErrUnexpectedEndOfDemo")
 }
 
+func TestBadNetMessageDecryptionKey(t *testing.T) {
+	t.Parallel()
+
+	if testing.Short() {
+		t.Skip("skipping test due to -short flag")
+	}
+
+	const (
+		demPath  = csDemosPath + "/match730_003528806449641685104_1453182610_271.dem"
+		infoPath = csDemosPath + "/match730_003449478367177343081_1946274414_112.dem.info"
+	)
+
+	infoF, err := os.Open(infoPath)
+	assert.NoError(t, err)
+
+	b, err := ioutil.ReadAll(infoF)
+	assert.NoError(t, err)
+
+	k, err := demoinfocs.MatchInfoDecryptionKey(b)
+	assert.NoError(t, err)
+
+	f, err := os.Open(demPath)
+	assert.NoError(t, err)
+
+	defer f.Close()
+
+	cfg := demoinfocs.DefaultParserConfig
+	cfg.NetMessageDecryptionKey = k
+
+	p := demoinfocs.NewParserWithConfig(f, cfg)
+
+	var cantReadEncNetMsgWarns []events.ParserWarn
+
+	p.RegisterEventHandler(func(warn events.ParserWarn) {
+		if warn.Type == events.WarnTypeCantReadEncryptedNetMessage {
+			cantReadEncNetMsgWarns = append(cantReadEncNetMsgWarns, warn)
+		}
+	})
+
+	err = p.ParseToEnd()
+	assert.NoError(t, err)
+
+	assert.NotEmpty(t, cantReadEncNetMsgWarns)
+}
+
 func TestParseToEnd_Cancel(t *testing.T) {
 	t.Parallel()
 
diff --git a/test/cs-demos b/test/cs-demos
index 6ec2aac..e970099 160000
--- a/test/cs-demos
+++ b/test/cs-demos
@@ -1,1 +1,1 @@
-Subproject commit 6ec2aacebacd9f0f5eee7c4640bed7693b8527bc
+Subproject commit e97009976cfcaee540870293c9b7c2825a2b8557