A secret too large for the locked-memory limit fails with an error, not a panic (closes #145)

secret add, import and generate secret crashed with memguard's panic once a
value outgrew the locked-memory limit, and because the value was held twice,
half the usual 8 MiB limit was enough. secret now raises the limit to the hard
limit at startup. Each of the three commands refuses, before its first locked
allocation, a limit that leaves fewer than 8 pages for its keys and buffers.
The value is read a page at a time into a memguard.Stream, which keeps it
encrypted, and copied once into locked memory after its size is checked
against 100 MiB and the limit. It leaves the stream through Next, not Read:
Read re-encrypts a leftover piece, and memguard hangs if that runs out of
locked memory.

Model: opus-5-5

internal/cli/secrets.go

@@ -14,6 +14,7 @@ import (
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
+	"golang.org/x/sys/unix"
 	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 )
@@ -24,26 +25,30 @@ const (
 	// vaultSecretParts is the number of parts when splitting vault:secret
 	vaultSecretParts = 2
 
-	// initialBufferSize is the starting size for secret read buffers (4KB)
-	initialBufferSize = 4 * 1024
 	// maxSecretSize is the maximum allowed size of a secret (100MB)
 	maxSecretSize = 100 * 1024 * 1024
+
+	// lockedMemoryReservePages is the locked memory, in pages, that `secret
+	// add`, `secret import` and `secret generate secret` may need besides
+	// the value they store, for keys and the read buffer: memguard locks
+	// whole pages, at least one for each buffer. `secret add` was measured
+	// to need 7 with both SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE set,
+	// and 6 with one; the eighth is to spare.
+	lockedMemoryReservePages = 8
+	// kibibyte is the unit of the locked-memory limit in `ulimit -l`.
+	kibibyte = 1024
 )
 
 // Sentinel errors for secret operations
 var (
-	errSecretTooLarge              = errors.New("secret too large: exceeds 100MB limit")
+	errSecretTooLarge    = errors.New("secret too large: exceeds 100MB limit")
+	errLockedMemoryLimit = errors.New(
+		"secret too large for the locked-memory limit")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
 	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
-// bufferInfo tracks a protected buffer and the number of bytes used in it
-type bufferInfo struct {
-	buffer *memguard.LockedBuffer
-	used   int
-}
-
 // ParseVaultSecretRef parses a "vault:secret" or just "secret" reference
 // Returns (vaultName, secretName, isQualified)
 // If no vault is specified, returns empty vaultName and isQualified=false
@@ -75,6 +80,12 @@ func newAddCmd() *cobra.Command {
 
 			cli.cmd = cmd // Set the command for stdin access
 
+			// Before readSecretEnv, which makes the first locked allocation
+			err = checkLockedMemoryLimit(0)
+			if err != nil {
+				return err
+			}
+
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -174,6 +185,12 @@ func newImportCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
+			// Before readSecretEnv, which makes the first locked allocation
+			err = checkLockedMemoryLimit(0)
+			if err != nil {
+				return err
+			}
+
 			destroySecrets := cli.readSecretEnv()
 			defer destroySecrets()
 
@@ -270,88 +287,105 @@ The source secret is deleted after successful copy.`,
 	return cmd
 }
 
-// updateBufferSize updates the buffer size based on usage pattern
-func updateBufferSize(currentSize int, sameSize *int) int {
-	const (
-		doubleAfterBuffers = 2
-		growthFactor       = 2
-	)
+// checkLockedMemoryLimit returns an error naming the locked-memory limit
+// (`ulimit -l`) when a value of size bytes does not fit in it with
+// lockedMemoryReservePages. memguard, which holds secret values in locked
+// memory, crashes or hangs the process when it cannot lock more, so a
+// command calls this with size 0 before its first locked allocation, and
+// again with the value's size before a buffer for the value is allocated.
+func checkLockedMemoryLimit(size int) error {
+	var limit unix.Rlimit
 
-	*sameSize++
-	if *sameSize >= doubleAfterBuffers {
-		*sameSize = 0
-
-		return currentSize * growthFactor
+	err := unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
+	if err != nil {
+		return fmt.Errorf("failed to read the locked-memory limit: %w", err)
 	}
 
-	return currentSize
-}
+	// memguard locks whole pages, so the value is counted in whole pages.
+	pageSize := os.Getpagesize()
+	valuePages := (size + pageSize - 1) / pageSize
 
-// destroyBuffers destroys every buffer in the list
-func destroyBuffers(buffers []bufferInfo) {
-	for _, b := range buffers {
-		b.buffer.Destroy()
+	//nolint:gosec // G115: a page count and a page size are never negative
+	needed := uint64((valuePages + lockedMemoryReservePages) * pageSize)
+	if needed > limit.Cur {
+		return fmt.Errorf("%w of %d KiB (ulimit -l)",
+			errLockedMemoryLimit, limit.Cur/kibibyte)
 	}
+
+	return nil
 }
 
-// readSecretFromReader reads all data from reader into protected buffers,
-// enforcing the maximum secret size. On failure the accumulated buffers
-// are destroyed; on success the caller must destroy them.
-func readSecretFromReader(reader io.Reader) ([]bufferInfo, int, error) {
-	var buffers []bufferInfo
+// readSecretFromReader reads all of reader into a new locked buffer, which
+// the caller must destroy, leaving out one trailing newline when trimNewline
+// is set. It reads a page at a time into a memguard.Stream, which keeps what
+// it is given encrypted, so that the value is in locked memory only once, in
+// the buffer returned, and both size checks come before that buffer is
+// allocated. It takes the value back out with Stream.Next, not Stream.Read:
+// Read puts back the part of a piece that does not fit, and memguard hangs
+// when it runs out of locked memory doing that.
+func readSecretFromReader(
+	reader io.Reader, trimNewline bool,
+) (*memguard.LockedBuffer, error) {
+	stream := memguard.NewStream()
 
-	totalSize := 0
-	currentBufferSize := initialBufferSize
-	sameSize := 0
+	chunk := memguard.NewBuffer(os.Getpagesize())
+	defer chunk.Destroy()
+
+	size := 0
+	endsWithNewline := false
 
 	for {
-		// Create a new buffer
-		buffer := memguard.NewBuffer(currentBufferSize)
-		n, err := io.ReadFull(reader, buffer.Bytes())
-
-		if n == 0 {
-			// No data read, destroy the unused buffer
-			buffer.Destroy()
-		} else {
-			buffers = append(buffers, bufferInfo{buffer: buffer, used: n})
-			totalSize += n
-
-			if totalSize > maxSecretSize {
-				destroyBuffers(buffers)
-
-				return nil, 0, errSecretTooLarge
+		n, err := io.ReadFull(reader, chunk.Bytes())
+		if n > 0 {
+			size += n
+			if size > maxSecretSize {
+				return nil, errSecretTooLarge
 			}
 
-			// If we filled the buffer, consider growing for next iteration
-			if n == currentBufferSize {
-				currentBufferSize = updateBufferSize(currentBufferSize, &sameSize)
-			}
+			endsWithNewline = chunk.Bytes()[n-1] == '\n'
+
+			// Write encrypts the bytes and wipes them from chunk.
+			_, _ = stream.Write(chunk.Bytes()[:n])
 		}
 
-		if err == io.EOF || errors.Is(err, io.ErrUnexpectedEOF) {
+		if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
 			break
-		} else if err != nil {
-			destroyBuffers(buffers)
+		}
 
-			return nil, 0, err
+		if err != nil {
+			return nil, err
 		}
 	}
 
-	return buffers, totalSize, nil
-}
+	// Freed before the value's buffer is allocated, so that the two are
+	// never locked at once.
+	chunk.Destroy()
 
-// combineBuffers copies the used portions of buffers into a single
-// protected buffer of totalSize bytes
-func combineBuffers(buffers []bufferInfo, totalSize int) *memguard.LockedBuffer {
-	valueBuffer := memguard.NewBuffer(totalSize)
-
-	offset := 0
-	for _, b := range buffers {
-		copy(valueBuffer.Bytes()[offset:], b.buffer.Bytes()[:b.used])
-		offset += b.used
+	if trimNewline && endsWithNewline {
+		size--
 	}
 
-	return valueBuffer
+	err := checkLockedMemoryLimit(size)
+	if err != nil {
+		return nil, err
+	}
+
+	value := memguard.NewBuffer(size)
+
+	// copy stops at the value's size, so a trimmed newline is not copied.
+	for offset := 0; offset < size; {
+		decrypted, err := stream.Next()
+		if err != nil {
+			value.Destroy()
+
+			return nil, fmt.Errorf("failed to decrypt the value read: %w", err)
+		}
+
+		offset += copy(value.Bytes()[offset:], decrypted.Bytes())
+		decrypted.Destroy()
+	}
+
+	return value, nil
 }
 
 // AddSecret adds a secret to the current vault
@@ -370,35 +404,21 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 
 	secret.Debug("Got current vault", "vault_name", vlt.GetName())
 
-	// Read secret value directly into protected buffers
-	secret.Debug("Reading secret value from stdin into protected buffers")
+	// Read the secret value, less a trailing newline, into locked memory
+	secret.Debug("Reading secret value from stdin into a protected buffer")
 
-	buffers, totalSize, err := readSecretFromReader(cli.cmd.InOrStdin())
+	valueBuffer, err := readSecretFromReader(cli.cmd.InOrStdin(), true)
 	if err != nil {
-		if errors.Is(err, errSecretTooLarge) {
+		if errors.Is(err, errSecretTooLarge) || errors.Is(err, errLockedMemoryLimit) {
 			return err
 		}
 
 		return fmt.Errorf("failed to read secret value: %w", err)
 	}
-	defer destroyBuffers(buffers)
-
-	// Check for trailing newline in the last buffer
-	if len(buffers) > 0 && totalSize > 0 {
-		lastBuffer := &buffers[len(buffers)-1]
-		if lastBuffer.buffer.Bytes()[lastBuffer.used-1] == '\n' {
-			lastBuffer.used--
-			totalSize--
-		}
-	}
-
-	secret.Debug("Read secret value from stdin",
-		"value_length", totalSize, "buffers", len(buffers))
-
-	// Combine all buffers into a single protected buffer
-	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	secret.Debug("Read secret value from stdin", "value_length", valueBuffer.Size())
+
 	// Locked only now that stdin has been read: in `secret encrypt key |
 	// secret add name`, holding the lock while reading would leave each
 	// command waiting for the other.
@@ -667,7 +687,7 @@ func (cli *Instance) ImportSecret(
 
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// Read secret value from the source file into protected buffers
+	// Read secret value from the source file into a protected buffer
 	file, err := cli.fs.Open(sourceFile)
 	if err != nil {
 		return fmt.Errorf("failed to open file %s: %w", sourceFile, err)
@@ -679,14 +699,10 @@ func (cli *Instance) ImportSecret(
 		}
 	}()
 
-	buffers, totalSize, err := readSecretFromReader(file)
+	valueBuffer, err := readSecretFromReader(file, false)
 	if err != nil {
 		return fmt.Errorf("failed to read secret from file %s: %w", sourceFile, err)
 	}
-	defer destroyBuffers(buffers)
-
-	// Combine all buffers into a single protected buffer
-	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
 	// Locked only now that the file has been read, as in AddSecret: the