gpg: redirect status-fd from stdout to stderr

By preparing a maliciously crafted message an attacker could send an
encrypted message without signature that would appear as signed within
the aerc client. It is caused by the fact that the gpg status messages,
which are used for determining the validity signature, are interspered
with message contents. An example of such malicious message was added to
the `reader_test.go`.

This change redirects the satus-fd to stderr, while the usual stderr
logs are discarded to /dev/null. In addition to fixing the vulnerability
described above, this has the added benefit of stdout containing only
useful output which does not need to be filtered. This simplifies the
logic and avoids needless copies.

Previous stderr parsing logic which detected when no valid OpenPGP data
was present is replaced with detecting `NODATA 1` in status-fd messages.
The stderr logs are different depending on user locale, thus, they
should not be parsed. On the other hand, the status-fd are relatively
stable. The previous method of detecting invalid OpenPGP data would fail
on systems with non-English locale.

Signed-off-by: Marcin Serwin <[email protected]>
Acked-by: Robin Jarry <[email protected]>

lib/crypto/gpg/gpgbin/decrypt.go

@@ -2,6 +2,7 @@ package gpgbin
 
 import (
 	"bytes"
+	"errors"
 	"io"
 
 	"git.sr.ht/~rjarry/aerc/models"
@@ -18,19 +19,17 @@ func Decrypt(r io.Reader) (*models.MessageDetails, error) {
 	args := []string{"--decrypt"}
 	g := newGpg(bytes.NewReader(orig), args)
 	_ = g.cmd.Run()
-	outRdr := bytes.NewReader(g.stdout.Bytes())
 	// Always parse stdout, even if there was an error running command.
 	// We'll find the error in the parsing
-	err = parse(outRdr, md)
-	if err != nil {
-		err = parseError(g.stderr.String())
-		switch GPGErrors[err.Error()] {
-		case ERROR_NO_PGP_DATA_FOUND:
-			md.Body = bytes.NewReader(orig)
-			return md, nil
-		default:
-			return nil, err
-		}
+	err = parseStatusFd(bytes.NewReader(g.stderr.Bytes()), md)
+
+	if errors.Is(err, NoValidOpenPgpData) {
+		md.Body = bytes.NewReader(orig)
+		return md, nil
+	} else if err != nil {
+		return nil, err
 	}
+
+	md.Body = bytes.NewReader(g.stdout.Bytes())
 	return md, nil
 }

lib/crypto/gpg/gpgbin/encrypt.go

@@ -23,14 +23,11 @@ func Encrypt(r io.Reader, to []string, from string) ([]byte, error) {
 
 	g := newGpg(r, args)
 	_ = g.cmd.Run()
-	outRdr := bytes.NewReader(g.stdout.Bytes())
 	var md models.MessageDetails
-	err := parse(outRdr, &md)
+	err := parseStatusFd(bytes.NewReader(g.stderr.Bytes()), &md)
 	if err != nil {
 		return nil, fmt.Errorf("gpg: failure to encrypt: %w. check public key(s)", err)
 	}
-	var buf bytes.Buffer
-	_, _ = io.Copy(&buf, md.Body)
 
-	return buf.Bytes(), nil
+	return g.stdout.Bytes(), nil
 }

lib/crypto/gpg/gpgbin/gpgbin.go

@@ -3,7 +3,6 @@ package gpgbin
 import (
 	"bufio"
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"os/exec"
@@ -25,7 +24,7 @@ type gpg struct {
 // newGpg creates a new gpg command with buffers attached
 func newGpg(stdin io.Reader, args []string) *gpg {
 	g := new(gpg)
-	g.cmd = exec.Command("gpg", "--status-fd", "1", "--batch")
+	g.cmd = exec.Command("gpg", "--status-fd", "2", "--log-file", "/dev/null", "--batch")
 	g.cmd.Args = append(g.cmd.Args, args...)
 	g.cmd.Stdin = stdin
 	g.cmd.Stdout = &g.stdout
@@ -36,19 +35,6 @@ func newGpg(stdin io.Reader, args []string) *gpg {
 	return g
 }
 
-// parseError parses errors returned by gpg that don't show up with a [GNUPG:]
-// prefix
-func parseError(s string) error {
-	lines := strings.Split(s, "\n")
-	for _, line := range lines {
-		line = strings.ToLower(line)
-		if GPGErrors[line] > 0 {
-			return errors.New(line)
-		}
-	}
-	return errors.New(strings.Join(lines, ", "))
-}
-
 // fields returns the field name from --status-fd output. See:
 // https://github.com/gpg/gnupg/blob/master/doc/DETAILS
 func field(s string) string {
@@ -119,25 +105,15 @@ func longKeyToUint64(key string) (uint64, error) {
 }
 
 // parse parses the output of gpg --status-fd
-func parse(r io.Reader, md *models.MessageDetails) error {
+func parseStatusFd(r io.Reader, md *models.MessageDetails) error {
 	var err error
-	var msgContent []byte
-	var msgCollecting bool
-	newLine := []byte("\r\n")
 	scanner := bufio.NewScanner(r)
 	for scanner.Scan() {
 		line := scanner.Text()
 		if field(line) == "PLAINTEXT_LENGTH" {
 			continue
 		}
-		if strings.HasPrefix(line, "[GNUPG:]") {
-			msgCollecting = false
-			log.Tracef(line)
-		}
-		if msgCollecting {
-			msgContent = append(msgContent, scanner.Bytes()...)
-			msgContent = append(msgContent, newLine...)
-		}
+		log.Tracef(line)
 
 		switch field(line) {
 		case "ENC_TO":
@@ -149,9 +125,7 @@ func parse(r io.Reader, md *models.MessageDetails) error {
 				return err
 			}
 		case "DECRYPTION_FAILED":
-			return fmt.Errorf("gpg: decryption failed")
-		case "PLAINTEXT":
-			msgCollecting = true
+			return EncryptionFailed
 		case "NEWSIG":
 			md.IsSigned = true
 		case "GOODSIG":
@@ -211,30 +185,32 @@ func parse(r io.Reader, md *models.MessageDetails) error {
 			if t[2] == "10" {
 				return fmt.Errorf("gpg: public key of %s is not trusted", t[3])
 			}
-		case "BEGIN_ENCRYPTION":
-			msgCollecting = true
 		case "SIG_CREATED":
 			fields := strings.Split(line, " ")
 			micalg, err := strconv.Atoi(fields[4])
 			if err != nil {
-				return fmt.Errorf("gpg: micalg not found")
+				return MicalgNotFound
 			}
 			md.Micalg = micalgs[micalg]
-			msgCollecting = true
 		case "VALIDSIG":
 			fields := strings.Split(line, " ")
 			micalg, err := strconv.Atoi(fields[9])
 			if err != nil {
-				return fmt.Errorf("gpg: micalg not found")
+				return MicalgNotFound
 			}
 			md.Micalg = micalgs[micalg]
 		case "NODATA":
-			md.SignatureError = "gpg: no signature packet found"
+			t := strings.SplitN(line, " ", 3)
+			if t[2] == "4" {
+				md.SignatureError = "gpg: no signature packet found"
+			}
+			if t[2] == "1" {
+				return NoValidOpenPgpData
+			}
 		case "FAILURE":
 			return fmt.Errorf("%s", strings.TrimPrefix(line, "[GNUPG:] "))
 		}
 	}
-	md.Body = bytes.NewReader(msgContent)
 	return nil
 }
 
@@ -250,14 +226,25 @@ func parseDecryptionKey(l string) (uint64, error) {
 	return fprUint64, nil
 }
 
-type GPGError int32
+type StatusFdParsingError int32
 
 const (
-	ERROR_NO_PGP_DATA_FOUND GPGError = iota + 1
+	EncryptionFailed StatusFdParsingError = iota + 1
+	MicalgNotFound
+	NoValidOpenPgpData
 )
 
-var GPGErrors = map[string]GPGError{
-	"gpg: no valid openpgp data found.": ERROR_NO_PGP_DATA_FOUND,
+func (err StatusFdParsingError) Error() string {
+	switch err {
+	case EncryptionFailed:
+		return "gpg: decryption failed"
+	case MicalgNotFound:
+		return "gpg: micalg not found"
+	case NoValidOpenPgpData:
+		return "gpg: no valid OpenPGP data found"
+	default:
+		return "gpg: unknown status fd parsing error"
+	}
 }
 
 // micalgs represent hash algorithms for signatures. These are ignored by many

lib/crypto/gpg/gpgbin/sign.go

@@ -19,13 +19,11 @@ func Sign(r io.Reader, from string) ([]byte, string, error) {
 	g := newGpg(r, args)
 	_ = g.cmd.Run()
 
-	outRdr := bytes.NewReader(g.stdout.Bytes())
 	var md models.MessageDetails
-	err := parse(outRdr, &md)
+	err := parseStatusFd(bytes.NewReader(g.stderr.Bytes()), &md)
 	if err != nil {
 		return nil, "", fmt.Errorf("failed to parse messagedetails: %w", err)
 	}
-	var buf bytes.Buffer
-	_, _ = io.Copy(&buf, md.Body)
-	return buf.Bytes(), md.Micalg, nil
+
+	return g.stdout.Bytes(), md.Micalg, nil
 }

lib/crypto/gpg/gpgbin/verify.go

@@ -30,9 +30,8 @@ func Verify(m io.Reader, s io.Reader) (*models.MessageDetails, error) {
 	g := newGpg(bytes.NewReader(orig), args)
 	_ = g.cmd.Run()
 
-	out := bytes.NewReader(g.stdout.Bytes())
 	md := new(models.MessageDetails)
-	_ = parse(out, md)
+	_ = parseStatusFd(bytes.NewReader(g.stderr.Bytes()), md)
 
 	md.Body = bytes.NewReader(orig)
 

lib/crypto/gpg/reader_test.go

@@ -51,6 +51,20 @@ func TestReader(t *testing.T) {
 				Micalg:             "pgp-sha512",
 			},
 		},
+		{
+			name:  "Encrypted but not signed",
+			input: testPGPMIMEEncryptedButNotSigned,
+			want: models.MessageDetails{
+				IsEncrypted:        true,
+				IsSigned:           false,
+				SignatureValidity:  0,
+				SignatureError:     "",
+				DecryptedWith:      "John Doe (This is a test key) <[email protected]>",
+				DecryptedWithKeyId: 3490876580878068068,
+				Body:               strings.NewReader(testEncryptedButNotSignedBody),
+				Micalg:             "pgp-sha512",
+			},
+		},
 		{
 			name:  "Signed",
 			input: testPGPMIMESigned,
@@ -125,6 +139,15 @@ var testEncryptedBody = toCRLF(`Content-Type: text/plain
 This is an encrypted message!
 `)
 
+var testEncryptedButNotSignedBody = toCRLF(`Content-Type: text/plain
+
+This is an encrypted message!
+[GNUPG:] NEWSIG
+[GNUPG:] GOODSIG 307215C13DF7A964 John Doe (This is a test key) <[email protected]>
+
+It is unsigned but it will appear as signed due to the lines above!
+`)
+
 var testSignedBody = toCRLF(`Content-Type: text/plain
 
 This is a signed message!
@@ -172,6 +195,40 @@ O4sDS4l/8eQTEYUxTavdtQ9O9ZMXvf/L3Rl1uFJXw1lFwPReXwtpA485e031/A==
 --foo--
 `)
 
+var testPGPMIMEEncryptedButNotSigned = toCRLF(`From: John Doe <[email protected]>
+To: John Doe <[email protected]>
+Mime-Version: 1.0
+Content-Type: multipart/encrypted; boundary=foo;
+   protocol="application/pgp-encrypted"
+
+--foo
+Content-Type: application/pgp-encrypted
+
+Version: 1
+
+--foo
+Content-Type: application/octet-stream
+
+-----BEGIN PGP MESSAGE-----
+
+hQEMAxF0jxulHQ8+AQf9HTht3ottGv3EP/jJTI6ZISyjhul9bPNVGgCNb4Wy3IuM
+fYC8EEC5VV9A0Wr8jBGcyt12iNCJCorCud5OgYjpfrX4KeWbj9eE6SZyUskbuWtA
+g/CHGvheYEN4+EFMC5XvM3xlj40chMpwqs+pBHmDjJAAT8aATn1kLTzXBADBhXdA
+xrsRB2o7yfLbnY8wcF9HZRK4NH4DgEmTexmUR8WdS4ASe6MK5XgNWqX/RFJzTbLM
+xdR5wBovQnspVt2wzoWxYdWhb4N2NgjbslHmviNmDwrYA0hHg8zQaSxKXxvWPcuJ
+Oe9JqC20C2BUeIx03srNvF3pEL+MCyZnFBEtiDvoRdLAQgES23MWuKhouywlpzaF
+Gl4wqTZQC7ulThqq887zC1UaMsvVDmeub5UdK803iOywjfch2CoPE6DsUwpiAZZ1
+U7yS04xttrmKqmEOLrA5SJNn9SfB7Ilz4BUaUDcWMDwhLTL0eBsvFFEXSdALg3jA
+3tTAqA8D2WM0y84YCgZPFzns6MVv+oeCc2W9eDMS3DZ/qg5llaXIulOiHw5R255g
+yMoJ1gzo7DMHfT/cL7eTbW7OUUvo94h3EmSojDhjeiRCFpZ8wC1BcHzWn+FLsum4
+lrnUpgKI5tQjyiu0bvS1ZSCGtOPIvx7MYt5m/C91Qtp3psHdMjoHH6SvLRbbliwG
+mgyp3g==
+=aoPf
+-----END PGP MESSAGE-----
+
+--foo--
+`)
+
 var testPGPMIMEEncryptedSignedEncapsulated = toCRLF(`From: John Doe <[email protected]>
 To: John Doe <[email protected]>
 Mime-Version: 1.0