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 <marcin@serwin.dev>
Acked-by: Robin Jarry <robin@jarry.cc>

6 files changed, 101 insertions, 64 deletions

diff --git a/lib/crypto/gpg/gpgbin/decrypt.go b/lib/crypto/gpg/gpgbin/decrypt.go
index 86d5575e..65f7a73d 100644
--- a/lib/crypto/gpg/gpgbin/decrypt.go
+++ b/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
 }
diff --git a/lib/crypto/gpg/gpgbin/encrypt.go b/lib/crypto/gpg/gpgbin/encrypt.go
index 712bb327..91e0999a 100644
--- a/lib/crypto/gpg/gpgbin/encrypt.go
+++ b/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
 }
diff --git a/lib/crypto/gpg/gpgbin/gpgbin.go b/lib/crypto/gpg/gpgbin/gpgbin.go
index a7eafacd..b4985328 100644
--- a/lib/crypto/gpg/gpgbin/gpgbin.go
+++ b/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
diff --git a/lib/crypto/gpg/gpgbin/sign.go b/lib/crypto/gpg/gpgbin/sign.go
index 163aedfd..63bbd15c 100644
--- a/lib/crypto/gpg/gpgbin/sign.go
+++ b/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
 }
diff --git a/lib/crypto/gpg/gpgbin/verify.go b/lib/crypto/gpg/gpgbin/verify.go
index 8208dc0d..a3ea4b4a 100644
--- a/lib/crypto/gpg/gpgbin/verify.go
+++ b/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)
 
diff --git a/lib/crypto/gpg/reader_test.go b/lib/crypto/gpg/reader_test.go
index fdb5c452..1ea0ef0f 100644
--- a/lib/crypto/gpg/reader_test.go
+++ b/lib/crypto/gpg/reader_test.go
@@ -52,6 +52,20 @@ func TestReader(t *testing.T) {
 			},
 		},
 		{
+			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) <john.doe@example.org>",
+				DecryptedWithKeyId: 3490876580878068068,
+				Body:               strings.NewReader(testEncryptedButNotSignedBody),
+				Micalg:             "pgp-sha512",
+			},
+		},
+		{
 			name:  "Signed",
 			input: testPGPMIMESigned,
 			want: models.MessageDetails{
@@ -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) <john.doe@example.org>
+
+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 <john.doe@example.org>
+To: John Doe <john.doe@example.org>
+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 <john.doe@example.org>
 To: John Doe <john.doe@example.org>
 Mime-Version: 1.0