From 1a2e56937396dfb993eab0a77d7097c2bf15caeb Mon Sep 17 00:00:00 2001 From: marin thiercelin Date: Mon, 10 Jan 2022 14:24:08 +0100 Subject: [PATCH 1/2] Fix parsing issue of AEAD encrypted messages. In pgpMessage.SeparateKeyAndData(), the parsing would ignore AEAD encrypted data packets. Which would result in a split message with a nil data packet. We add support for AEAD encrypted data packets. This also affects `NewPGPSplitMessageFromArmored` and `NewPGPSplitMessage`. --- CHANGELOG.md | 6 ++ crypto/message.go | 123 ++++++++++++++++++++++------------------- crypto/message_test.go | 23 ++++++++ 3 files changed, 96 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9980855..16048ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed +- Fixed bug with `NewPGPSplitMessageFromArmored(armored)` and `PGPMessage.SeparateKeyAndData()`. +Those functions didn't parse AEAD encrypted messages correctly (eg messages encrypted with the latest versions of gnupg.), resulting in a nil `DataPacket`. + ## [2.4.0] 2021-12-21 ### Added diff --git a/crypto/message.go b/crypto/message.go index 5ff3fce..a3f45b7 100644 --- a/crypto/message.go +++ b/crypto/message.go @@ -333,7 +333,6 @@ func (msg *PGPMessage) SeparateKeyAndData(estimatedLength, garbageCollector int) // For info on each, see: https://golang.org/pkg/runtime/#MemStats packets := packet.NewReader(bytes.NewReader(msg.Data)) outSplit = &PGPSplitMessage{} - gcCounter := 0 // Store encrypted key and symmetrically encrypted packet separately var encryptedKey *packet.EncryptedKey @@ -345,68 +344,21 @@ func (msg *PGPMessage) SeparateKeyAndData(estimatedLength, garbageCollector int) } switch p := p.(type) { case *packet.EncryptedKey: + // TODO: add support for multiple keypackets if encryptedKey != nil && encryptedKey.Key != nil { break } encryptedKey = p - case *packet.SymmetricallyEncrypted: - // TODO: add support for multiple keypackets - var b bytes.Buffer - // 2^16 is an estimation of the size difference between input and output, the size difference is most probably - // 16 bytes at a maximum though. - // We need to avoid triggering a grow from the system as this will allocate too much memory causing problems - // in low-memory environments - b.Grow(1<<16 + estimatedLength) - // empty encoded length + start byte - if _, err := b.Write(make([]byte, 6)); err != nil { - return nil, errors.Wrap(err, "gopenpgp: error in writing data packet header") + outSplit.DataPacket, err = readPacketContents(p.Contents, estimatedLength, garbageCollector) + if err != nil { + return nil, err } - - if err := b.WriteByte(byte(1)); err != nil { - return nil, errors.Wrap(err, "gopenpgp: error in writing data packet header") + case *packet.AEADEncrypted: + outSplit.DataPacket, err = readPacketContents(p.Contents, estimatedLength, garbageCollector) + if err != nil { + return nil, err } - - actualLength := 1 - block := make([]byte, 128) - for { - n, err := p.Contents.Read(block) - if goerrors.Is(err, io.EOF) { - break - } - if _, err := b.Write(block[:n]); err != nil { - return nil, errors.Wrap(err, "gopenpgp: error in writing data packet body") - } - actualLength += n - gcCounter += n - if gcCounter > garbageCollector && garbageCollector > 0 { - runtime.GC() - gcCounter = 0 - } - } - - // quick encoding - symEncryptedData := b.Bytes() - switch { - case actualLength < 192: - symEncryptedData[4] = byte(210) - symEncryptedData[5] = byte(actualLength) - symEncryptedData = symEncryptedData[4:] - case actualLength < 8384: - actualLength -= 192 - symEncryptedData[3] = byte(210) - symEncryptedData[4] = 192 + byte(actualLength>>8) - symEncryptedData[5] = byte(actualLength) - symEncryptedData = symEncryptedData[3:] - default: - symEncryptedData[0] = byte(210) - symEncryptedData[1] = byte(255) - symEncryptedData[2] = byte(actualLength >> 24) - symEncryptedData[3] = byte(actualLength >> 16) - symEncryptedData[4] = byte(actualLength >> 8) - symEncryptedData[5] = byte(actualLength) - } - outSplit.DataPacket = symEncryptedData } } if encryptedKey == nil { @@ -422,6 +374,65 @@ func (msg *PGPMessage) SeparateKeyAndData(estimatedLength, garbageCollector int) return outSplit, nil } +func readPacketContents(contents io.Reader, estimatedLength int, garbageCollector int) ([]byte, error) { + gcCounter := 0 + var b bytes.Buffer + // 2^16 is an estimation of the size difference between input and output, the size difference is most probably + // 16 bytes at a maximum though. + // We need to avoid triggering a grow from the system as this will allocate too much memory causing problems + // in low-memory environments + b.Grow(1<<16 + estimatedLength) + // empty encoded length + start byte + if _, err := b.Write(make([]byte, 6)); err != nil { + return nil, errors.Wrap(err, "gopenpgp: error in writing data packet header") + } + + if err := b.WriteByte(byte(1)); err != nil { + return nil, errors.Wrap(err, "gopenpgp: error in writing data packet header") + } + + actualLength := 1 + block := make([]byte, 128) + for { + n, err := contents.Read(block) + if goerrors.Is(err, io.EOF) { + break + } + if _, err := b.Write(block[:n]); err != nil { + return nil, errors.Wrap(err, "gopenpgp: error in writing data packet body") + } + actualLength += n + gcCounter += n + if gcCounter > garbageCollector && garbageCollector > 0 { + runtime.GC() + gcCounter = 0 + } + } + + // quick encoding + symEncryptedData := b.Bytes() + switch { + case actualLength < 192: + symEncryptedData[4] = byte(210) + symEncryptedData[5] = byte(actualLength) + symEncryptedData = symEncryptedData[4:] + case actualLength < 8384: + actualLength -= 192 + symEncryptedData[3] = byte(210) + symEncryptedData[4] = 192 + byte(actualLength>>8) + symEncryptedData[5] = byte(actualLength) + symEncryptedData = symEncryptedData[3:] + default: + symEncryptedData[0] = byte(210) + symEncryptedData[1] = byte(255) + symEncryptedData[2] = byte(actualLength >> 24) + symEncryptedData[3] = byte(actualLength >> 16) + symEncryptedData[4] = byte(actualLength >> 8) + symEncryptedData[5] = byte(actualLength) + } + return symEncryptedData, nil +} + // GetBinary returns the unarmored binary content of the signature as a []byte. func (sig *PGPSignature) GetBinary() []byte { return sig.Data diff --git a/crypto/message_test.go b/crypto/message_test.go index 64e9f38..0fa363c 100644 --- a/crypto/message_test.go +++ b/crypto/message_test.go @@ -439,3 +439,26 @@ func TestMessageGetArmoredWithEmptyHeaders(t *testing.T) { assert.NotContains(t, armored, "Version") assert.NotContains(t, armored, "Comment") } + +func TestPGPSplitMessageFromArmoredWithAEAD(t *testing.T) { + var message = `-----BEGIN PGP MESSAGE----- + +hF4DJDxTg/yg6TkSAQdA3Ogzuxwz7IdSRCh81gdYuB0bKqkYDs7EksOkYJ7eUnMw +FsRNg+X3KbCj9j747An4J7V8trghOIN00dlpuR77wELS79XHoP55qmyVyPzmTXdx +1F8BCQIQyGCAxAA1ppydoBVp7ithTEl2bU72tbOsLCFY8TBamG6t3jfqJpO2lz+G +M0xNgvwIDrAQsN35VGw72I/FvWJ0VG3rpBKgFp5nPK0NblRomXTRRfoNgSoVUcxU +vA== +=YNf2 +-----END PGP MESSAGE----- +` + split, err := NewPGPSplitMessageFromArmored(message) + if err != nil { + t.Errorf("Couldn't parse split message: %v", err) + } + if split.KeyPacket == nil { + t.Error("Key packet was nil") + } + if split.DataPacket == nil { + t.Error("Data packet was nil") + } +} From a514e451c4d37f137a9270125b8febe74e282800 Mon Sep 17 00:00:00 2001 From: wussler Date: Mon, 10 Jan 2022 14:42:46 +0100 Subject: [PATCH 2/2] Fix typo in CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16048ad..608a1bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed bug with `NewPGPSplitMessageFromArmored(armored)` and `PGPMessage.SeparateKeyAndData()`. -Those functions didn't parse AEAD encrypted messages correctly (eg messages encrypted with the latest versions of gnupg.), resulting in a nil `DataPacket`. +Those functions didn't parse AEAD encrypted messages correctly (eg messages encrypted with the latest versions of gnupg), resulting in a nil `DataPacket`. ## [2.4.0] 2021-12-21