diff --git a/README.md b/README.md index dceb58b..77ddd42 100644 --- a/README.md +++ b/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 [--force]` Adds a secret to the current vault. Reads the secret value from stdin. diff --git a/internal/cli/generate.go b/internal/cli/generate.go index 631e6bd..b020a49 100644 --- a/internal/cli/generate.go +++ b/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": diff --git a/internal/cli/root.go b/internal/cli/root.go index 2921c8c..65d1620 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 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") diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 0c0ba27..1bf7580 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,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 diff --git a/internal/cli/secrets_size_test.go b/internal/cli/secrets_size_test.go index ae707c4..2ec47d0 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,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