From 84b4b30f2159144bf706deb0173d2444781efa05 Mon Sep 17 00:00:00 2001 From: Devon Steenberg Date: Wed, 6 Aug 2025 10:19:15 +1200 Subject: [PATCH] fix(rand): Use crypto/rand instead of math/rand in FIPS mode [BE-12071] (#961) Co-authored-by: codecov-ai[bot] <156709835+codecov-ai[bot]@users.noreply.github.com> --- api/chisel/tunnel.go | 8 +- api/chisel/tunnel_test.go | 79 ++++++++++++++++++ api/internal/randomstring/random_string.go | 4 +- .../randomstring/random_string_test.go | 65 +++++++++++++++ api/stacks/deployments/deployer_remote.go | 4 +- pkg/librand/rand.go | 47 +++++++++++ pkg/librand/rand_test.go | 81 +++++++++++++++++++ 7 files changed, 281 insertions(+), 7 deletions(-) create mode 100644 api/chisel/tunnel_test.go create mode 100644 api/internal/randomstring/random_string_test.go create mode 100644 pkg/librand/rand.go create mode 100644 pkg/librand/rand_test.go diff --git a/api/chisel/tunnel.go b/api/chisel/tunnel.go index 5533978a8..fe4415b04 100644 --- a/api/chisel/tunnel.go +++ b/api/chisel/tunnel.go @@ -4,7 +4,6 @@ import ( "encoding/base64" "errors" "fmt" - "math/rand" "net" "strings" "time" @@ -14,6 +13,7 @@ import ( "github.com/portainer/portainer/api/internal/edge/cache" "github.com/portainer/portainer/api/internal/endpointutils" "github.com/portainer/portainer/pkg/libcrypto" + "github.com/portainer/portainer/pkg/librand" "github.com/dchest/uniuri" "github.com/rs/zerolog/log" @@ -200,7 +200,9 @@ func (service *Service) getUnusedPort() int { conn, err := net.DialTCP("tcp", nil, &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1), Port: port}) if err == nil { - conn.Close() + if err := conn.Close(); err != nil { + log.Warn().Msg("failed to close tcp connection that checks if port is free") + } log.Debug(). Int("port", port). @@ -213,7 +215,7 @@ func (service *Service) getUnusedPort() int { } func randomInt(min, max int) int { - return min + rand.Intn(max-min) + return min + librand.Intn(max-min) } func generateRandomCredentials() (string, string) { diff --git a/api/chisel/tunnel_test.go b/api/chisel/tunnel_test.go new file mode 100644 index 000000000..6959bfa3a --- /dev/null +++ b/api/chisel/tunnel_test.go @@ -0,0 +1,79 @@ +package chisel + +import ( + "net" + "strings" + "testing" + + portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/dataservices" +) + +type testSettingsService struct { + dataservices.SettingsService +} + +func (s *testSettingsService) Settings() (*portainer.Settings, error) { + return &portainer.Settings{ + EdgeAgentCheckinInterval: 1, + }, nil +} + +type testStore struct { + dataservices.DataStore +} + +func (s *testStore) Settings() dataservices.SettingsService { + return &testSettingsService{} +} + +func TestGetUnusedPort(t *testing.T) { + testCases := []struct { + name string + existingTunnels map[portainer.EndpointID]*portainer.TunnelDetails + expectedError error + }{ + { + name: "simple case", + }, + { + name: "existing tunnels", + existingTunnels: map[portainer.EndpointID]*portainer.TunnelDetails{ + portainer.EndpointID(1): { + Port: 53072, + }, + portainer.EndpointID(2): { + Port: 63072, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + store := &testStore{} + s := NewService(store, nil, nil) + s.activeTunnels = tc.existingTunnels + port := s.getUnusedPort() + + if port < 49152 || port > 65535 { + t.Fatalf("Expected port to be inbetween 49152 and 65535 but got %d", port) + } + + for _, tun := range tc.existingTunnels { + if tun.Port == port { + t.Fatalf("returned port %d already has an existing tunnel", port) + } + } + + conn, err := net.DialTCP("tcp", nil, &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1), Port: port}) + if err == nil { + // Ignore error + _ = conn.Close() + t.Fatalf("expected port %d to be unused", port) + } else if !strings.Contains(err.Error(), "connection refused") { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/api/internal/randomstring/random_string.go b/api/internal/randomstring/random_string.go index 0f2074e58..44d43368c 100644 --- a/api/internal/randomstring/random_string.go +++ b/api/internal/randomstring/random_string.go @@ -1,6 +1,6 @@ package randomstring -import "math/rand" +import "github.com/portainer/portainer/pkg/librand" const letterBytes = "abcdefghijklmnopqrstuvwxyz0123456789" @@ -8,7 +8,7 @@ const letterBytes = "abcdefghijklmnopqrstuvwxyz0123456789" func RandomString(n int) string { b := make([]byte, n) for i := range b { - b[i] = letterBytes[rand.Intn(len(letterBytes))] + b[i] = letterBytes[librand.Intn(len(letterBytes))] } return string(b) } diff --git a/api/internal/randomstring/random_string_test.go b/api/internal/randomstring/random_string_test.go new file mode 100644 index 000000000..371374b5a --- /dev/null +++ b/api/internal/randomstring/random_string_test.go @@ -0,0 +1,65 @@ +package randomstring + +import ( + "testing" + + "github.com/portainer/portainer/pkg/fips" + "github.com/stretchr/testify/require" +) + +func init() { + fips.InitFIPS(false) +} + +func TestRandomString(t *testing.T) { + testCases := []struct { + name string + length int + expected int + }{ + { + name: "zero length", + length: 0, + expected: 0, + }, + { + name: "short string", + length: 5, + expected: 5, + }, + { + name: "longer string", + length: 20, + expected: 20, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := RandomString(tc.length) + require.Equal(t, tc.expected, len(result)) + + // Verify all characters are from the expected alphabet + for _, char := range result { + require.Contains(t, letterBytes, string(char)) + } + }) + } +} + +func TestRandomStringUniqueness(t *testing.T) { + // Generate multiple random strings and verify they are different + const numStrings = 100 + const stringLength = 10 + + generated := make(map[string]bool) + + for range numStrings { + str := RandomString(stringLength) + require.Equal(t, stringLength, len(str)) + + // Check if we've seen this string before (very unlikely for random strings) + require.False(t, generated[str], "Generated duplicate random string: %s", str) + generated[str] = true + } +} diff --git a/api/stacks/deployments/deployer_remote.go b/api/stacks/deployments/deployer_remote.go index 524211c37..2098d0a03 100644 --- a/api/stacks/deployments/deployer_remote.go +++ b/api/stacks/deployments/deployer_remote.go @@ -5,13 +5,13 @@ import ( "context" "fmt" "io" - "math/rand" "os" "strings" "time" portainer "github.com/portainer/portainer/api" "github.com/portainer/portainer/api/filesystem" + "github.com/portainer/portainer/pkg/librand" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" @@ -210,7 +210,7 @@ func (d *stackDeployer) remoteStack(stack *portainer.Stack, endpoint *portainer. fmt.Sprintf("%s:%s", composeDestination, composeDestination), fmt.Sprintf("%s:%s", targetSocketBindHost, targetSocketBindContainer), }, - }, nil, nil, fmt.Sprintf("portainer-unpacker-%d-%s-%d", stack.ID, stack.Name, rand.Intn(100))) + }, nil, nil, fmt.Sprintf("portainer-unpacker-%d-%s-%d", stack.ID, stack.Name, librand.Intn(100))) if err != nil { return errors.Wrap(err, "unable to create unpacker container") diff --git a/pkg/librand/rand.go b/pkg/librand/rand.go new file mode 100644 index 000000000..10b5321b5 --- /dev/null +++ b/pkg/librand/rand.go @@ -0,0 +1,47 @@ +package librand + +import ( + "crypto/rand" + "fmt" + "math/big" + mrand "math/rand/v2" + + "github.com/portainer/portainer/pkg/fips" +) + +func Intn(max int) int { + return intn(max, fips.FIPSMode()) +} + +func intn(max int, fips bool) int { + return int(int64n(int64(max), fips)) +} + +func int64n(max int64, fips bool) int64 { + if !fips { + return mrand.Int64N(max) + } + + i, err := rand.Int(rand.Reader, big.NewInt(max)) + if err != nil { + panic(fmt.Sprintf("failed to generate a random number: %v", err)) + } + if !i.IsInt64() { + panic("generated random number cannot be represented as an int64") + } + + return i.Int64() +} + +func Float64() float64 { + return randomFloat64(fips.FIPSMode()) +} + +func randomFloat64(fips bool) float64 { + if !fips { + return mrand.Float64() + } + + // This is based of this comment https://cs.opensource.google/go/go/+/refs/tags/go1.24.5:src/math/rand/v2/rand.go;l=209 + return float64(int64n(1<<53, fips) / (1 << 53)) +} diff --git a/pkg/librand/rand_test.go b/pkg/librand/rand_test.go new file mode 100644 index 000000000..43f7b6dba --- /dev/null +++ b/pkg/librand/rand_test.go @@ -0,0 +1,81 @@ +package librand + +import ( + "testing" + + "github.com/portainer/portainer/pkg/fips" +) + +func init() { + fips.InitFIPS(false) +} + +func TestIntn(t *testing.T) { + i := Intn(10) + + if i >= 10 || i < 0 { + t.Fatalf("random number %d wasn't within interval", i) + } +} + +func TestInternalIntn(t *testing.T) { + testCases := []struct { + name string + max int + fips bool + }{ + { + name: "non-fips mode", + max: 10, + fips: false, + }, + { + name: "fips mode", + max: 10, + fips: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + i := intn(tc.max, tc.fips) + + if i >= tc.max || i < 0 { + t.Fatalf("random number %d wasn't within interval", i) + } + }) + } +} + +func TestFloat64(t *testing.T) { + f := Float64() + + if f >= 1 || f < 0 { + t.Fatalf("random float %v wasn't within interval", f) + } +} + +func TestInternalFloat64(t *testing.T) { + testCases := []struct { + name string + fips bool + }{ + { + name: "non-fips mode", + fips: false, + }, + { + name: "fips mode", + fips: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := randomFloat64(tc.fips) + if f >= 1 || f < 0 { + t.Fatalf("random float %v wasn't within interval", f) + } + }) + } +}