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

README.md

@@ -127,6 +127,17 @@ one.
 
 ### Secret Management
 
+#### Secret Size
+
+While `secret` works on a secret's value it keeps the value in locked memory, so
+the value counts against the locked-memory limit (`ulimit -l`). `secret` raises
+that limit to the hard limit when it starts. `secret add`, `secret import` and
+`secret generate secret` refuse a value that does not fit in the limit with 8
+memory pages to spare (32 KiB with 4 KiB pages), with an error that names the
+limit. Linux's usual limit of 8 MiB allows a value of nearly 8 MiB; a larger one
+needs a higher hard limit, such as `memlock` in `/etc/security/limits.conf`. No
+value may be over 100 MiB, whatever the limit.
+
 #### `secret add <secret-name> [--force]`
 
 Adds a secret to the current vault. Reads the secret value from stdin.

internal/cli/generate.go

@@ -72,6 +72,12 @@ func newGenerateSecretCmd() *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()
 
@@ -133,10 +139,16 @@ func (cli *Instance) GenerateSecret(
 		return errLengthTooSmall
 	}
 
-	var (
-		secretValue string
-		err         error
-	)
+	if length > maxSecretSize {
+		return errSecretTooLarge
+	}
+
+	err := checkLockedMemoryLimit(length)
+	if err != nil {
+		return err
+	}
+
+	var secretValue string
 
 	switch secretType {
 	case "base58":

internal/cli/root.go

@@ -14,6 +14,8 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
+	raiseLockedMemoryLimit()
+
 	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
 	// and exits with status 1. The passphrase prompt turns echo off on the
 	// controlling terminal until the read finishes, so a signal there would
@@ -51,6 +53,19 @@ func Entry() int {
 	return 0
 }
 
+// raiseLockedMemoryLimit raises this process's limit on locked memory, which
+// holds every secret value, to the hard limit, the most a process may set
+// without privileges. When that fails the limit stays as it was, and
+// checkLockedMemoryLimit names it in its error.
+func raiseLockedMemoryLimit() {
+	var limit unix.Rlimit
+
+	if unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit) == nil {
+		limit.Cur = limit.Max
+		_ = unix.Setrlimit(unix.RLIMIT_MEMLOCK, &limit)
+	}
+}
+
 func newRootCmd() *cobra.Command {
 	secret.Debug("newRootCmd starting")
 

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.
@@ -661,7 +681,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)
@@ -673,14 +693,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

internal/cli/secrets_size_test.go

@@ -3,17 +3,23 @@ package cli
 
 import (
 	"bytes"
+	"context"
 	"crypto/rand"
 	"fmt"
 	"io"
+	"os"
+	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 	"sneak.berlin/go/secret/pkg/agehd"
 )
@@ -315,6 +321,159 @@ func TestAddSecretStreamingBehavior(t *testing.T) {
 		"Retrieved secret should match original")
 }
 
+// newBinaryTestStateDir returns a new state directory holding a vault made
+// from the test mnemonic, for the secret binary to use.
+func newBinaryTestStateDir(t *testing.T) string {
+	t.Helper()
+
+	stateDir := t.TempDir()
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	return stateDir
+}
+
+// runAtLockedMemoryLimit runs the secret binary with args, stdin, the state
+// directory stateDir and the test mnemonic, its locked-memory limit (ulimit
+// -l) lowered to limitKiB, and returns what it printed.
+func runAtLockedMemoryLimit(
+	t *testing.T, stateDir string, limitKiB int, stdin io.Reader,
+	args ...string,
+) (string, error) {
+	t.Helper()
+
+	const commandWait = 10 * time.Second
+
+	secretPath, err := filepath.Abs("../../secret")
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	// The shell lowers the limit for itself and the binary only.
+	script := fmt.Sprintf(`ulimit -l %d && exec "$0" "$@"`, limitKiB)
+
+	//nolint:gosec // G204: runs the freshly built secret binary
+	cmd := exec.CommandContext(ctx, "sh",
+		append([]string{"-c", script, secretPath}, args...)...)
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvMnemonic + "=" + testMnemonic,
+	}
+	cmd.Stdin = stdin
+
+	output, err := cmd.CombinedOutput()
+
+	return string(output), err
+}
+
+// requireRefusedAtLockedMemoryLimit runs the secret binary as
+// runAtLockedMemoryLimit does, and requires it to exit with status 1 and
+// print wantError, with no panic.
+func requireRefusedAtLockedMemoryLimit(
+	t *testing.T, stateDir string, limitKiB int, wantError string,
+	stdin io.Reader, args ...string,
+) {
+	t.Helper()
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB, stdin, args...)
+
+	var exitErr *exec.ExitError
+
+	require.ErrorAs(t, err, &exitErr, "%v: %s", args, output)
+	assert.Equal(t, 1, exitErr.ExitCode(), "%v: %s", args, output)
+	assert.Contains(t, output, wantError, args)
+	assert.NotContains(t, output, "panic", args)
+}
+
+// TestSecretsAtLockedMemoryLimit runs the secret binary with the
+// locked-memory limit (ulimit -l) lowered to 1 MiB, where memguard would crash
+// the process with a panic instead of returning an error. add must store the
+// largest value that fits, which takes holding it in locked memory only once.
+// add, import and generate secret must refuse a value one byte larger, and one
+// over 100 MB, with exit status 1 and an error saying why.
+func TestSecretsAtLockedMemoryLimit(t *testing.T) {
+	t.Parallel()
+
+	const limitKiB = 1024
+
+	fits := limitKiB*kibibyte - lockedMemoryReservePages*os.Getpagesize()
+	limitError := fmt.Sprintf("locked-memory limit of %d KiB", limitKiB)
+	stateDir := newBinaryTestStateDir(t)
+
+	tooBigFile := filepath.Join(t.TempDir(), "too-big")
+	require.NoError(t, os.WriteFile(tooBigFile, make([]byte, fits+1), 0o600))
+
+	// Sparse, so that it takes no room on disk
+	overMaxSize, err := os.Create(filepath.Join(t.TempDir(), "over-max-size"))
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = overMaxSize.Close() })
+	require.NoError(t, overMaxSize.Truncate(maxSecretSize+1))
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, limitKiB,
+		bytes.NewReader(make([]byte, fits)), "add", "fits")
+	require.NoError(t, err, "the largest value that fits: %s", output)
+
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		bytes.NewReader(make([]byte, fits+1)), "add", "too-big")
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		nil, "import", "too-big", "--source", tooBigFile)
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB, limitError,
+		nil, "generate", "secret", "too-big", "--length", strconv.Itoa(fits+1))
+
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB,
+		errSecretTooLarge.Error(), overMaxSize, "add", "too-big")
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB,
+		errSecretTooLarge.Error(), nil,
+		"import", "too-big", "--source", overMaxSize.Name())
+	requireRefusedAtLockedMemoryLimit(t, stateDir, limitKiB,
+		errSecretTooLarge.Error(), nil,
+		"generate", "secret", "too-big", "--length", strconv.Itoa(maxSecretSize+1))
+}
+
+// TestSecretsUnderSmallLockedMemoryLimits runs add, import and generate
+// secret with a short value under two small locked-memory limits (ulimit
+// -l). Under 20 KiB, too little for the buffers they need besides the value,
+// each must refuse it with exit status 1 and an error naming the limit, where
+// memguard would panic or hang. Under 64 KiB, the default of older Linux
+// kernels, each must store it.
+func TestSecretsUnderSmallLockedMemoryLimits(t *testing.T) {
+	t.Parallel()
+
+	const (
+		tooSmallLimitKiB   = 20
+		oldDefaultLimitKiB = 64
+		shortValue         = "hunter2"
+	)
+
+	tooSmallLimitError := fmt.Sprintf("locked-memory limit of %d KiB",
+		tooSmallLimitKiB)
+	stateDir := newBinaryTestStateDir(t)
+
+	shortFile := filepath.Join(t.TempDir(), "short")
+	require.NoError(t, os.WriteFile(shortFile, []byte(shortValue), 0o600))
+
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, strings.NewReader(shortValue), "add", "added")
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, nil, "import", "imported", "--source", shortFile)
+	requireRefusedAtLockedMemoryLimit(t, stateDir, tooSmallLimitKiB,
+		tooSmallLimitError, nil, "generate", "secret", "generated")
+
+	output, err := runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		strings.NewReader(shortValue), "add", "added")
+	require.NoError(t, err, "add: %s", output)
+
+	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		nil, "import", "imported", "--source", shortFile)
+	require.NoError(t, err, "import: %s", output)
+
+	output, err = runAtLockedMemoryLimit(t, stateDir, oldDefaultLimitKiB,
+		nil, "generate", "secret", "generated")
+	require.NoError(t, err, "generate secret: %s", output)
+}
+
 // slowReader simulates a reader that returns data in small chunks
 type slowReader struct {
 	data      []byte