Skip to content

Commit e1ea40e

Browse files
authored
Merge pull request #281 from ProtonMail/fix/clearsign-check-hash
This PR fixes clearsign to only allow hash functions that are allowed for the particular signing algorithm.
2 parents a994e32 + 9cd4c3a commit e1ea40e

File tree

2 files changed

+144
-16
lines changed

2 files changed

+144
-16
lines changed

openpgp/clearsign/clearsign.go

Lines changed: 86 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func Decode(data []byte) (b *Block, rest []byte) {
206206
type dashEscaper struct {
207207
buffered *bufio.Writer
208208
hashers []hash.Hash // one per key in privateKeys
209-
hashType crypto.Hash
209+
hashTypes []crypto.Hash
210210
toHash io.Writer // writes to all the hashes in hashers
211211
salts [][]byte // salts for the signatures if v6
212212
armorHeader map[string]string // Armor headers
@@ -328,7 +328,7 @@ func (d *dashEscaper) Close() (err error) {
328328
sig.Version = k.Version
329329
sig.SigType = packet.SigTypeText
330330
sig.PubKeyAlgo = k.PubKeyAlgo
331-
sig.Hash = d.hashType
331+
sig.Hash = d.hashTypes[i]
332332
sig.CreationTime = t
333333
sig.IssuerKeyId = &k.KeyId
334334
sig.IssuerFingerprint = k.Fingerprint
@@ -390,19 +390,22 @@ func EncodeMultiWithHeader(w io.Writer, privateKeys []*packet.PrivateKey, config
390390
}
391391

392392
hashType := config.Hash()
393-
name := nameOfHash(hashType)
394-
if len(name) == 0 {
395-
return nil, errors.UnsupportedError("unknown hash type: " + strconv.Itoa(int(hashType)))
396-
}
397393

398-
if !hashType.Available() {
399-
return nil, errors.UnsupportedError("unsupported hash type: " + strconv.Itoa(int(hashType)))
400-
}
401394
var hashers []hash.Hash
395+
var hashTypes []crypto.Hash
402396
var ws []io.Writer
403397
var salts [][]byte
404398
for _, sk := range privateKeys {
405-
h := hashType.New()
399+
acceptedHashes := acceptableHashesToWrite(&sk.PublicKey)
400+
// acceptedHashes contains at least one hash
401+
selectedHashType := acceptedHashes[0]
402+
for _, acceptedHash := range acceptedHashes {
403+
if hashType == acceptedHash {
404+
selectedHashType = hashType
405+
break
406+
}
407+
}
408+
h := selectedHashType.New()
406409
if sk.Version == 6 {
407410
// generate salt
408411
var salt []byte
@@ -416,6 +419,7 @@ func EncodeMultiWithHeader(w io.Writer, privateKeys []*packet.PrivateKey, config
416419
salts = append(salts, salt)
417420
}
418421
hashers = append(hashers, h)
422+
hashTypes = append(hashTypes, selectedHashType)
419423
ws = append(ws, h)
420424
}
421425
toHash := io.MultiWriter(ws...)
@@ -432,11 +436,8 @@ func EncodeMultiWithHeader(w io.Writer, privateKeys []*packet.PrivateKey, config
432436
nonV6 := len(salts) < len(hashers)
433437
// Crypto refresh: Headers SHOULD NOT be emitted
434438
if nonV6 { // Emit header if non v6 signatures are present for compatibility
435-
if _, err = buffered.WriteString(fmt.Sprintf("%s: %s", hashHeader, name)); err != nil {
436-
return
437-
}
438-
if err = buffered.WriteByte(lf); err != nil {
439-
return
439+
if err := writeHashHeader(buffered, hashTypes); err != nil {
440+
return nil, err
440441
}
441442
}
442443
if err = buffered.WriteByte(lf); err != nil {
@@ -446,7 +447,7 @@ func EncodeMultiWithHeader(w io.Writer, privateKeys []*packet.PrivateKey, config
446447
plaintext = &dashEscaper{
447448
buffered: buffered,
448449
hashers: hashers,
449-
hashType: hashType,
450+
hashTypes: hashTypes,
450451
toHash: toHash,
451452
salts: salts,
452453
armorHeader: headers,
@@ -470,6 +471,40 @@ func (b *Block) VerifySignature(keyring openpgp.KeyRing, config *packet.Config)
470471
return
471472
}
472473

474+
// writeHashHeader writes the legacy cleartext hash header to buffered.
475+
func writeHashHeader(buffered *bufio.Writer, hashTypes []crypto.Hash) error {
476+
seen := make(map[string]bool, len(hashTypes))
477+
if _, err := buffered.WriteString(fmt.Sprintf("%s: ", hashHeader)); err != nil {
478+
return err
479+
}
480+
481+
for index, sigHashType := range hashTypes {
482+
first := index == 0
483+
name := nameOfHash(sigHashType)
484+
if len(name) == 0 {
485+
return errors.UnsupportedError("unknown hash type: " + strconv.Itoa(int(sigHashType)))
486+
}
487+
488+
switch {
489+
case !seen[name] && first:
490+
if _, err := buffered.WriteString(name); err != nil {
491+
return err
492+
}
493+
case !seen[name]:
494+
if _, err := buffered.WriteString(fmt.Sprintf(",%s", name)); err != nil {
495+
return err
496+
}
497+
}
498+
seen[name] = true
499+
}
500+
501+
if err := buffered.WriteByte(lf); err != nil {
502+
return err
503+
}
504+
505+
return nil
506+
}
507+
473508
// nameOfHash returns the OpenPGP name for the given hash, or the empty string
474509
// if the name isn't known. See RFC 4880, section 9.4.
475510
func nameOfHash(h crypto.Hash) string {
@@ -489,3 +524,38 @@ func nameOfHash(h crypto.Hash) string {
489524
}
490525
return ""
491526
}
527+
528+
func acceptableHashesToWrite(singingKey *packet.PublicKey) []crypto.Hash {
529+
switch singingKey.PubKeyAlgo {
530+
case packet.PubKeyAlgoEd448:
531+
return []crypto.Hash{
532+
crypto.SHA512,
533+
crypto.SHA3_512,
534+
}
535+
case packet.PubKeyAlgoECDSA, packet.PubKeyAlgoEdDSA:
536+
if curve, err := singingKey.Curve(); err == nil {
537+
if curve == packet.Curve448 ||
538+
curve == packet.CurveNistP521 ||
539+
curve == packet.CurveBrainpoolP512 {
540+
return []crypto.Hash{
541+
crypto.SHA512,
542+
crypto.SHA3_512,
543+
}
544+
} else if curve == packet.CurveBrainpoolP384 ||
545+
curve == packet.CurveNistP384 {
546+
return []crypto.Hash{
547+
crypto.SHA384,
548+
crypto.SHA512,
549+
crypto.SHA3_512,
550+
}
551+
}
552+
}
553+
}
554+
return []crypto.Hash{
555+
crypto.SHA256,
556+
crypto.SHA384,
557+
crypto.SHA512,
558+
crypto.SHA3_256,
559+
crypto.SHA3_512,
560+
}
561+
}

openpgp/clearsign/clearsign_test.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package clearsign
66

77
import (
8+
"bufio"
89
"bytes"
910
"crypto"
1011
"fmt"
@@ -171,6 +172,63 @@ func TestSigningInterop(t *testing.T) {
171172
}
172173
}
173174

175+
func TestHashHeader(t *testing.T) {
176+
tests := []struct {
177+
name string
178+
hashTypes []crypto.Hash
179+
expected string
180+
}{
181+
{
182+
name: "unique hashes",
183+
hashTypes: []crypto.Hash{
184+
crypto.SHA256,
185+
crypto.SHA512,
186+
crypto.SHA3_512,
187+
},
188+
expected: "Hash: SHA256,SHA512,SHA3-512\n",
189+
},
190+
{
191+
name: "with duplicates",
192+
hashTypes: []crypto.Hash{
193+
crypto.SHA256,
194+
crypto.SHA512,
195+
crypto.SHA512,
196+
crypto.SHA3_512,
197+
},
198+
expected: "Hash: SHA256,SHA512,SHA3-512\n",
199+
},
200+
{
201+
name: "with duplicates",
202+
hashTypes: []crypto.Hash{
203+
crypto.SHA256,
204+
crypto.SHA256,
205+
crypto.SHA256,
206+
crypto.SHA256,
207+
},
208+
expected: "Hash: SHA256\n",
209+
},
210+
}
211+
212+
for _, tc := range tests {
213+
t.Run(tc.name, func(t *testing.T) {
214+
var buf bytes.Buffer
215+
writer := bufio.NewWriter(&buf)
216+
if err := writeHashHeader(writer, tc.hashTypes); err != nil {
217+
t.Fatalf("unexpected error: %v", err)
218+
}
219+
220+
if err := writer.Flush(); err != nil {
221+
t.Fatalf("flush failed: %v", err)
222+
}
223+
224+
actual := buf.String()
225+
if actual != tc.expected {
226+
t.Errorf("output mismatch:\nExpected: %q\nActual: %q", tc.expected, actual)
227+
}
228+
})
229+
}
230+
}
231+
174232
func testMultiSign(t *testing.T, v6 bool) {
175233
if testing.Short() {
176234
t.Skip("skipping long test in -short mode")

0 commit comments

Comments
 (0)