[bugfix] Fix parsing of large secrets (gopasspw#2654)

The AKV secret type does rely on the bufio.Scanner and that uses
a 64k limit by default. This change extends the limit to always
match the supplied input data. This might exceed the available
memory but not corrupt the output data.

If we want to support payloads larger than the available memory
we must make large changes to the parser. But since the issue
is quite severe and we don't advertise to support huge playloads
this seems to be a good compromise.

Fixes gopasspw#2594

Signed-off-by: Dominik Schulz <[email protected]>

internal/action/binary.go

@@ -46,24 +46,33 @@ func (s *Action) Cat(c *cli.Context) error {
 			return exit.Error(exit.IO, err, "Failed to copy after %d bytes: %s", written, err)
 		}
 
-		return s.Store.Set(
+		sec, err := secFromBytes(name, "STDIN", content.Bytes())
+		if err != nil {
+			return exit.Error(exit.IO, err, "Failed to parse secret from STDIN: %v", err)
+		}
+		if err = s.Store.Set(
 			ctxutil.WithCommitMessage(ctx, "Read secret from STDIN"),
 			name,
-			secFromBytes(name, "STDIN", content.Bytes()),
-		)
+			sec,
+		); err != nil {
+			return exit.Error(exit.Unknown, err, "Failed to write secret from STDIN: %v", err)
+		}
+
+		return nil
 	}
 
 	buf, err := s.binaryGet(ctx, name)
 	if err != nil {
 		return exit.Error(exit.Decrypt, err, "failed to read secret: %s", err)
 	}
+	debug.Log("read %d decoded bytes from secret %s", len(buf), name)
 
 	fmt.Fprint(stdout, string(buf))
 
 	return nil
 }
 
-func secFromBytes(dst, src string, in []byte) gopass.Secret {
+func secFromBytes(dst, src string, in []byte) (gopass.Secret, error) {
 	debug.Log("Read %d bytes from %s to %s", len(in), src, dst)
 
 	sec := secrets.NewAKV()
@@ -74,9 +83,32 @@ func secFromBytes(dst, src string, in []byte) gopass.Secret {
 		debug.Log("Failed to set Content-Transfer-Encoding: %q", err)
 	}
 
-	_, _ = sec.Write([]byte(base64.StdEncoding.EncodeToString(in)))
+	var written int
+	encoder := base64.NewEncoder(base64.StdEncoding, sec)
+	n, err := encoder.Write(in)
+	if err != nil {
+		debug.Log("Failed to write to base64 encoder: %v", err)
+
+		return sec, err
+	}
+	written += n
+
+	if err := encoder.Close(); err != nil {
+		debug.Log("Failed to finalize base64 payload: %v", err)
 
-	return sec
+		return sec, err
+	}
+	n, err = sec.Write([]byte("\n"))
+	if err != nil {
+		debug.Log("Failed to write to secret: %v", err)
+
+		return sec, err
+	}
+	written += n
+
+	debug.Log("Wrote %d bytes of Base64 encoded bytes to secret", written)
+
+	return sec, nil
 }
 
 // BinaryCopy copies either from the filesystem to the store or from the store.
@@ -153,8 +185,12 @@ func (s *Action) binaryCopyFromFileToStore(ctx context.Context, from, to string,
 		return fmt.Errorf("failed to read file from %q: %w", from, err)
 	}
 
+	sec, err := secFromBytes(to, from, buf)
+	if err != nil {
+		return fmt.Errorf("failed to parse secret from input: %w", err)
+	}
 	if err := s.Store.Set(
-		ctxutil.WithCommitMessage(ctx, fmt.Sprintf("Copied data from %s to %s", from, to)), to, secFromBytes(to, from, buf)); err != nil {
+		ctxutil.WithCommitMessage(ctx, fmt.Sprintf("Copied data from %s to %s", from, to)), to, sec); err != nil {
 		return fmt.Errorf("failed to save buffer to store: %w", err)
 	}
 
@@ -249,15 +285,24 @@ func (s *Action) binaryGet(ctx context.Context, name string) ([]byte, error) {
 	}
 
 	if !isBase64Encoded(sec) {
+		debug.Log("handling non-base64 secret")
+
 		// need to use sec.Bytes() otherwise the first line is missing.
 		return sec.Bytes(), nil
 	}
 
-	buf, err := base64.StdEncoding.DecodeString(sec.Body())
+	debug.Log("decoding Base64 encoded secret")
+	body := sec.Body()
+	buf, err := base64.StdEncoding.DecodeString(body)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encode to base64: %w", err)
 	}
 
+	debug.Log("decoded %d Base64 chars into %d bytes", len(body), len(buf))
+	if len(buf) < 1 {
+		debug.Log("body:\n%v", body)
+	}
+
 	return buf, nil
 }
 

internal/action/binary_test.go

@@ -1,6 +1,7 @@
 package action
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"math/rand"
@@ -40,6 +41,8 @@ func TestBinary(t *testing.T) {
 }
 
 func TestBinaryCat(t *testing.T) {
+	tSize := 1024
+
 	u := gptest.NewUnitTester(t)
 
 	ctx := context.Background()
@@ -60,7 +63,7 @@ func TestBinaryCat(t *testing.T) {
 	ctx = act.cfg.WithConfig(ctx)
 
 	infile := filepath.Join(u.Dir, "input.txt")
-	writeBinfile(t, infile)
+	writeBinfile(t, infile, tSize)
 
 	t.Run("populate store", func(t *testing.T) {
 		assert.NoError(t, act.binaryCopy(ctx, gptest.CliCtx(ctx, t), infile, "bar", true))
@@ -72,7 +75,7 @@ func TestBinaryCat(t *testing.T) {
 
 	stdinfile := filepath.Join(u.Dir, "stdin")
 	t.Run("binary cat baz from stdin", func(t *testing.T) {
-		writeBinfile(t, stdinfile)
+		writeBinfile(t, stdinfile, tSize)
 
 		fd, err := os.Open(stdinfile)
 		assert.NoError(t, err)
@@ -94,6 +97,60 @@ func TestBinaryCat(t *testing.T) {
 	})
 }
 
+func TestBinaryCatSizes(t *testing.T) {
+	u := gptest.NewUnitTester(t)
+
+	ctx := context.Background()
+	ctx = ctxutil.WithAlwaysYes(ctx, true)
+	ctx = ctxutil.WithHidden(ctx, true)
+
+	buf := &bytes.Buffer{}
+	out.Stdout = buf
+	stdout = buf
+	defer func() {
+		out.Stdout = os.Stdout
+		stdout = os.Stdout
+	}()
+
+	act, err := newMock(ctx, u.StoreDir(""))
+	require.NoError(t, err)
+	require.NotNil(t, act)
+	ctx = act.cfg.WithConfig(ctx)
+
+	for tSize := 1024; tSize < bufio.MaxScanTokenSize*2; tSize += 1024 {
+		// cat stdinfile | gopass cat baz
+		stdinfile := filepath.Join(u.Dir, "stdin")
+		writeBinfile(t, stdinfile, tSize)
+
+		fd, err := os.Open(stdinfile)
+		assert.NoError(t, err)
+
+		catFn := func() {
+			binstdin = fd
+			defer func() {
+				binstdin = os.Stdin
+				_ = fd.Close()
+			}()
+
+			assert.NoError(t, act.Cat(gptest.CliCtx(ctx, t, "baz")))
+		}
+		catFn()
+
+		// gopass cat baz and compare output with input, they should match
+		buf, err := os.ReadFile(stdinfile)
+		require.NoError(t, err)
+		sec, err := act.binaryGet(ctx, "baz")
+		require.NoError(t, err)
+
+		if string(buf) != string(sec) {
+			t.Fatalf("Input and output mismatch at tSize %d", tSize)
+
+			break
+		}
+		t.Logf("Input and Output match at tSize %d", tSize)
+	}
+}
+
 func TestBinaryCopy(t *testing.T) {
 	u := gptest.NewUnitTester(t)
 
@@ -125,7 +182,7 @@ func TestBinaryCopy(t *testing.T) {
 	t.Run("copy binary file", func(t *testing.T) {
 		defer buf.Reset()
 
-		writeBinfile(t, infile)
+		writeBinfile(t, infile, 1024)
 		assert.NoError(t, act.binaryCopy(ctx, gptest.CliCtx(ctx, t), infile, "bar", true))
 	})
 
@@ -177,7 +234,7 @@ func TestBinarySum(t *testing.T) {
 	infile := filepath.Join(u.Dir, "input.raw")
 
 	t.Run("populate store", func(t *testing.T) {
-		writeBinfile(t, infile)
+		writeBinfile(t, infile, 1024)
 		assert.NoError(t, act.binaryCopy(ctx, gptest.CliCtx(ctx, t), infile, "bar", true))
 	})
 
@@ -207,15 +264,14 @@ func TestBinaryGet(t *testing.T) {
 	assert.Equal(t, data, out)
 }
 
-func writeBinfile(t *testing.T, fn string) {
+func writeBinfile(t *testing.T, fn string, size int) {
 	t.Helper()
 
 	// tests should be predicable
-	rand.Seed(42)
+	lr := rand.New(rand.NewSource(42))
 
-	size := 1024
 	buf := make([]byte, size)
-	n, err := rand.Read(buf)
+	n, err := lr.Read(buf)
 	assert.NoError(t, err)
 	assert.Equal(t, size, n)
 	assert.NoError(t, os.WriteFile(fn, buf, 0o644))

pkg/gopass/secrets/akv.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/gopasspw/gopass/internal/set"
+	"github.com/gopasspw/gopass/pkg/debug"
 	"golang.org/x/exp/maps"
 )
 
@@ -164,7 +165,7 @@ func (a *AKV) Del(key string) bool {
 
 	delete(a.kvp, key)
 
-	s := bufio.NewScanner(strings.NewReader(a.raw.String()))
+	s := newScanner(strings.NewReader(a.raw.String()), a.raw.Len())
 	a.raw = strings.Builder{}
 	first := true
 	for s.Scan() {
@@ -211,7 +212,7 @@ func (a *AKV) Password() string {
 
 // SetPassword updates the password.
 func (a *AKV) SetPassword(p string) {
-	s := bufio.NewScanner(strings.NewReader(a.raw.String()))
+	s := newScanner(strings.NewReader(a.raw.String()), a.raw.Len())
 	a.raw = strings.Builder{}
 
 	// write the new password
@@ -237,7 +238,9 @@ func (a *AKV) SetPassword(p string) {
 func ParseAKV(in []byte) *AKV {
 	a := NewAKV()
 	a.raw = strings.Builder{}
-	s := bufio.NewScanner(bytes.NewReader(in))
+	s := newScanner(bytes.NewReader(in), len(in))
+
+	debug.Log("Parsing %d bytes of input", len(in))
 
 	first := true
 	for s.Scan() {
@@ -281,7 +284,15 @@ func ParseAKV(in []byte) *AKV {
 func (a *AKV) Body() string {
 	out := strings.Builder{}
 
-	s := bufio.NewScanner(strings.NewReader(a.raw.String()))
+	// make sure we always have a terminating newline so the scanner below
+	// can always find a full line.
+	if !strings.HasSuffix(a.raw.String(), "\n") {
+		a.raw.WriteString("\n")
+	}
+
+	debug.Log("Building body from %d chars", a.raw.Len())
+	s := newScanner(strings.NewReader(a.raw.String()), a.raw.Len())
+
 	first := true
 	for s.Scan() {
 		// skip over the password
@@ -294,15 +305,36 @@ func (a *AKV) Body() string {
 		line := s.Text()
 		// ignore KV pairs
 		if strings.Contains(line, kvSep) {
+			debug.Log("ignoring line: %q", line)
+
 			continue
 		}
+		debug.Log("adding line of %d chars", len(line))
 		out.WriteString(line)
 		out.WriteString("\n")
 	}
 
+	debug.Log("built %d chars body", out.Len())
+
 	return out.String()
 }
 
+// newScanner creates a new line scanner and sets it up to properly handle larger secrets.
+// bufio.Scanner uses bufio.MaxScanTokenSize by default. That is only 64k, too little
+// for larger binary secrets. We double the allocation size to account for Base64 encoding
+// overhead.
+func newScanner(in io.Reader, inSize int) *bufio.Scanner {
+	bufSize := inSize * 2
+
+	s := bufio.NewScanner(in)
+	scanBuf := make([]byte, bufSize)
+	s.Buffer(scanBuf, bufSize)
+
+	debug.Log("Using buffer of len %d and max %d", len(scanBuf), bufSize)
+
+	return s
+}
+
 // Write appends the buffer to the secret's body.
 func (a *AKV) Write(buf []byte) (int, error) {
 	var w io.Writer