Parser errors on encrypted net-message when decryption key is provided

[akiver]

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 because it's trying to read out of the bit reader boundaries.

To Reproduce

This zip archive contains 2 affected demos.

Code:

Parsing one of the attached demos using the snippet from the last release 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:

+++ 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.

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!

[markus-wa]

thanks for the report @akiver - I'll try to get to this soon :tm:

[markus-wa]

I've started investigating this - looks like https://github.com/saul/demofile/ has the same issue (but that's not to say it's not something to be fixed!)

@akiver do you know any parser that can extract this data currently?

[markus-wa]

So I'm pretty sure this is caused by either the decryption key being read wrong or the decryption algorithm having a bug.

Therefore I don't think it's sensible to just ignore those messages - users could be very confused if they pass the decryption key but all or some messages are not being decrypted.

We would need to at the very least raise a ParserWarn event - probably even an error.

basically:

• if data is corrupt, raise ParserWarn
• if decryption algorithm is broken, fix the bug
• if decryption key is wrong, raise an error

now we just need to figure out what exactly is happening

[markus-wa]

I've slightly changed the title - while there is a panic internally we recover() from it and the library "just" returns an error.

That's only relevant in as far as that we never want to panic from library code, only from user code.

[saul]

Is it possible that these demos are simply corrupt? A very insignificant change to an encrypted data blob (even just 1 bit) will avalanche into a big change in the decrypted data. Unfortunately demos have no checksums - so an error in any of these could cause corruption:

• Cosmic ray hitting the RAM where the demo is buffered before being written to disk
• Network corruption when uploading/downloading from the game server -> Valve CDN/Valve CDN -> your machine
• Disk corruption on the game server/Valve CDN/your machine

[markus-wa]

that's a very valid point @saul

one thing I noticed however is that if one decryption fails, all of them fail - but the rest of the demo is generally still valid. which would likely point to the key or algorithm being the issue.

@akiver a few questions (if you know the answers):

• where are these demos from?
• how often does this happen with MM demos?
• is there any pattern you've seen where it happens?

[saul]

Playing these matches in the official game client shows that we're calculating the same decryption key. If you place both the .dem and the .dem.info in the csgo/replays folder then play them from the main menu, once the demo starts you can see that cl_decryptdata_key_pub is the same as the key extracted by extractPublicEncryptionKey.

Note that the game doesn't crash like our parsers do when it encounters bad encrypted data. I'm pretty sure the engine just skips over these messages, as when you play the demo back you can't see any chat messages.

Also I tested my hypothesis that maybe the key had a bit flipped, but I tried all 64 combinations of the key but none of them decrypted correctly. So I'm pretty sure the encryption key is plainly wrong.

[akiver]

Thank you guys!

@akiver do you know any parser that can extract this data currently?

Nop

I don't remember when/how I downloaded these demos, but it's either from the game or from a NodeJS process using boiler-writter. In both cases, it was with a share code.

Today I downloaded (with a script + boiler-witter, not from the game) and parsed 210 recent MM demos and also 118 demos from the 2019 Berlin major. I never encountered the issue, so maybe at some point, the .dem or .info has been corrupted. Since I switch between OSs and computers I usually transfer demos with an external HDD or a Dropbox-like service, maybe that's the reason. What's strange is that the .info files don't seem corrupted since others data are readable.

@markus-wa about raising an error https://github.com/markus-wa/demoinfocs-golang/issues/326#issuecomment-1085818860, I'm a bit mitigated because this code part could change without notice since it's supposed to private 😄

[markus-wa]

so, I've investigated some more, and what jumps out at me is that the two broken demos @akiver linked have the following keys when translated to HEX (which is what we use for ICE)

F5B4050C8CAF5800 - 9517933397249562624 as uint64
84167DFF22E31800 - 17704781586558310400 as uint64

note both of the hex versions ending with 00

for reference, one demo that works had the key 07CA349329F7C253, not ending in 00

[saul]

Nice catch with the 00 suffix on the hex versions of the key. Off the back of that I've just tried all key combinations of F5B4050C8CAF5800 + n where n is in the range [0..256), but the parser still can't decrypt the encrypted messages.

Edit: just tested 84167DFF22E31800 + n on the other demo, but unfortunately also didn't work.

[markus-wa]

that was quick! ahh shame - it might just be a coincidence / red herring then :/

[markus-wa]

I think the takeaway for me from this whole thing is what @saul correctly said

[...] I'm pretty sure the engine just skips over these messages, as when you play the demo back you can't see any chat messages.

[...] So I'm pretty sure the encryption key is plainly wrong.

I think the tricky part is now deciding on how to handle this.

• If we just raise a ParserWarn chances are the consumers will not see this warning and will assume all data was extracted when it really wasn't
• If we return an error, we fail parsing even though the consumer did everything right (it's not the consumers fault that Valve messed up the decryption key)

We could return an error (ErrBadDecryptionKey) at the end after we parsed everything successfully, a bit similar to ErrUnexpectedEndOfDemo which can happen when parsing was successful. but what if we get both ErrUnexpectedEndOfDemo and ErrBadDecryptionKey in the same demo?

Anyone got any thoughts?

edit: maybe we can do something like ParserConfig.IgnoreErrBombsiteIndexNotFound but for ErrBadDecryptionKey - but I'm personally not a fan of that "feature" tbh

[an0nfunc]

Over at csgow.tf we skipped decryption-keys that are not len() 16 outright and do not try to hand them over to the parser. IMHO, especially since this is not that rare, it should not amount to an error halting parsing, maybe a warning is better suited.

Just my 2 cents.

[markus-wa]

@an0nfunc I think that's another issue - see #338 #339 for this - in the next version all keys should be 16 bytes :slightly_smiling_face:

[an0nfunc]

@markus-wa Oh, seems I mixed them up :). Well, point about the warning still stands.

[an0nfunc]

Looking over this, since I do the extraction without using your methods, I think #338 and this are related, are they not? Since filtering for 16 bytes I never got this issue, because I filtered out the "broken" description keys. That is exactly the last byte that is missing in your test-demos.

[markus-wa]

that's what I thought at first, but #339 doesn't fix the two linked demos though :disappointed:

And I think the last byte isn't missing, it just happens to be 00 for both of those, which can be valid

[an0nfunc]

But that does not explain why I never get this error. Even with over 4k MM matches parsed, I never encountered this. But I have plenty of demos where the decryption key is len == 15.

What I want to say is: That may be a viable option to check for a valid decyption key without going into parsing (like IsValidDecryptionKey). That does not replace proper error handling, of course.

Edit: I can provide plenty more demos where this happens if needed. Just looked it up: I have ~200 of 4000 matches where this happens (len of decryptionkey == 15). Edit2: If we could correlate #338 with this, that would help. Like, does #338 only occur if the decryption key is invalid? Edit3: Got some of them. I can provide about 150 more, if they are not dated back more then 30d. Demo1, Demo2, Demo3, Demo4, Demo5

[markus-wa]

@an0nfunc please test your demos with decryption key length 15 against #339 - I'm pretty sure this will fix your problem.

as @akiver said any demos downloaded recently (200 of them) were ok - I think this is just no longer an issue with new demos.

unless someone can provide a sharecode to reproduce this issue here I will focus on #338 / #339 and assume this issue here does no longer occur with recently downloaded .dem / .dem.info files

[an0nfunc]

@markus-wa I tested 5, and #339 does seem to fix it for them. So I guess that was just a coincidence. Well, happy that it works now :)

[markus-wa]

the keys for the demo linked by akiver are 16 bytes, just the last byte is 00 - which is different from length 15 bytes :slightly_smiling_face: - but yes, there's a coincidence in that the last byte was 00 in those I think, which may have lead to the confusion

[markus-wa]

@akiver can you verify that #342 fixes this and everything behaves as expected?

sorry this took so long, I've been a bit busy :sweat:

[akiver]

@akiver can you verify that #342 fixes this and everything behaves as expected?

sorry this took so long, I've been a bit busy 😓

All good! Thank you!

[markus-wa]

I've released v2.13.2 which includes the fix: https://github.com/markus-wa/demoinfocs-golang/releases/tag/v2.13.2

Thanks all!