diff --git a/README.md b/README.md index 72f4964..ca8e7bf 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,17 @@ one. ### Secret Management +#### Secret Size + +A secret's value is held in locked memory, which is never written to swap. +`secret` raises its locked-memory limit (`ulimit -l`) to the hard limit when it +starts, and `secret add`, `secret import` and `secret generate secret` refuse a +value that does not fit in that limit with 16 memory pages to spare (64 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 [--force]` Adds a secret to the current vault. Reads the secret value from stdin. diff --git a/TODO.md b/TODO.md index b608afe..506e099 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,17 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: A secret too large for the locked-memory limit (`ulimit -l`) makes + `secret add`, `secret import` and `secret generate secret` fail with an error + that names the limit, not crash with a panic + (https://git.eeqj.de/sneak/secret/issues/145). memguard panics when it cannot + lock memory, and the value was held twice, so with the usual 8 MiB limit any + value over about 4 MiB crashed. `secret` now raises the limit to the hard + limit at startup. The value is read a page at a time into a `memguard.Stream`, + which keeps it encrypted, and copied once into locked memory only after its + size is checked against 100 MiB and against the limit less 16 pages for the + command's keys. `generate secret` checks the length before it generates + anything. - 2026-10-14: Removing the current unlocker no longer leaves the vault unable to open through the unlockers it still has (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on diff --git a/internal/cli/generate.go b/internal/cli/generate.go index e02dab0..ebf147d 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -133,10 +133,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": diff --git a/internal/cli/root.go b/internal/cli/root.go index 8a7929f..f4b40b2 100644 --- a/internal/cli/root.go +++ b/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 terminal echo // off until the read finishes, so a signal there would leave echo off. @@ -38,6 +40,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") diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 1728e2b..9d99f4c 100644 --- a/internal/cli/secrets.go +++ b/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,28 @@ 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 a command + // needs besides the value it stores, for its keys and its read buffer, + // with room to spare: memguard locks whole pages, at least one for each + // buffer, and `secret add` needs 7. + lockedMemoryReservePages = 16 + // 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 @@ -270,88 +273,100 @@ 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 the process when it cannot lock more, so this check comes +// 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 -} - -// destroyBuffers destroys every buffer in the list -func destroyBuffers(buffers []bufferInfo) { - for _, b := range buffers { - b.buffer.Destroy() + //nolint:gosec // G115: a size and a page count are never negative + needed := uint64(size + lockedMemoryReservePages*os.Getpagesize()) + 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 +385,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. @@ -653,7 +654,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) @@ -665,14 +666,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 diff --git a/internal/cli/secrets_size_test.go b/internal/cli/secrets_size_test.go index ae707c4..f2bcbcf 100644 --- a/internal/cli/secrets_size_test.go +++ b/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,77 @@ func TestAddSecretStreamingBehavior(t *testing.T) { "Retrieved secret should match original") } +// TestSecretsAtLockedMemoryLimit runs the secret binary with the +// locked-memory limit (ulimit -l) lowered. 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 generate secret +// one over 100 MB, with exit status 1 and an error saying why, where memguard +// would crash the process with a panic. +func TestSecretsAtLockedMemoryLimit(t *testing.T) { + t.Parallel() + + const ( + limitKiB = 1024 + commandWait = 10 * time.Second + ) + + fits := limitKiB*kibibyte - lockedMemoryReservePages*os.Getpagesize() + limitError := fmt.Sprintf("locked-memory limit of %d KiB", limitKiB) + + stateDir := t.TempDir() + _, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + secretPath, err := filepath.Abs("../../secret") + require.NoError(t, err) + + tooBigFile := filepath.Join(t.TempDir(), "too-big") + require.NoError(t, os.WriteFile(tooBigFile, make([]byte, fits+1), 0o600)) + + run := func(stdin []byte, args ...string) (string, error) { + 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 = bytes.NewReader(stdin) + + output, err := cmd.CombinedOutput() + + return string(output), err + } + + output, err := run(make([]byte, fits), "add", "fits") + require.NoError(t, err, "the largest value that fits: %s", output) + + refused := func(wantError string, stdin []byte, args ...string) { + output, err := run(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) + } + + refused(limitError, make([]byte, fits+1), "add", "too-big") + refused(limitError, nil, "import", "too-big", "--source", tooBigFile) + refused(limitError, nil, "generate", "secret", "too-big", + "--length", strconv.Itoa(fits+1)) + refused(errSecretTooLarge.Error(), nil, "generate", "secret", "too-big", + "--length", strconv.Itoa(maxSecretSize+1)) +} + // slowReader simulates a reader that returns data in small chunks type slowReader struct { data []byte