From 54e51b52fdcdb03f85f4d0d59b36d99a1d4c5097 Mon Sep 17 00:00:00 2001 From: Ievgen Bondarenko Date: Tue, 19 May 2026 14:54:37 -0700 Subject: [PATCH] tcp: mark ISN and timestamp-offset secrets nosave for checkpoint ## Summary `pkg/tcpip/transport/tcp/protocol.go` is annotated `+stateify savable`. Two of its fields are 16-byte CSPRNG secrets used for ISN and timestamp-offset generation (RFC 6528): ```go // protocol.go:88-119 (before this PR) // +stateify savable type protocol struct { ... // The following secrets are initialized once and stay unchanged after. seqnumSecret [16]byte tsOffsetSecret [16]byte } ``` Neither field carries `state:"nosave"`. The stateify generator therefore writes both 16-byte values verbatim into the checkpoint state stream. A reader of a checkpoint file recovers the secrets and predicts ISNs and timestamp offsets on the restored sandbox. ## Background Sibling fields in the same struct already carry the tag: - `protocol.go:92`: `mu protocolRWMutex` with `state:"nosave"` - `protocol.go:115`: `probe TCPProbeFunc` with `state:"nosave"` The source of randomness itself, `secureRNG`, is also `state:"nosave"` at `pkg/tcpip/stack/stack.go:159`. The absence of the tag on `seqnumSecret` / `tsOffsetSecret` is an omission against the established codebase convention, not a deliberate choice. ## Linux / BSD reference Linux stores its ISN secret (`net_secret` in `net/core/secure_seq.c`) in kernel memory per netns; FreeBSD and OpenBSD store theirs in `tcp_subr.c` kernel memory. None of those stacks have a checkpoint/restore equivalent. CRIU does not dump kernel memory, so the Linux ISN secret is not exposed through the closest analog. The checkpoint surface is unique to gVisor save-restore. CVE-2024-10026 and the NDSS 2025 paper "You Can Rand but You Can't Hide" (Kaplan, Even, Klein; commits `83f75082e5`, `e54bfde792`, `f956b5ac17`, fixed in `v20231204.0.0`) improved the cryptographic quality of these same secrets. Persistence into checkpoint state was not addressed by that work. ## Change After this PR, `protocol.go:114-120` reads: ```go probe TCPProbeFunc `state:"nosave"` // The following secrets are used for ISN and timestamp-offset // generation. They are not serialized into checkpoint state and are // freshly drawn from the secure RNG on restore (see afterLoad). seqnumSecret [16]byte `state:"nosave"` tsOffsetSecret [16]byte `state:"nosave"` ``` New file `pkg/tcpip/transport/tcp/protocol_state.go` adds an `afterLoad` hook that redraws both secrets from `p.stack.SecureRNG()` on restore. Save: stateify omits both fields from the checkpoint blob, which now contains neither the bytes nor the field names. Load: stateify restores the protocol with both fields zeroed; `afterLoad` reseeds from `stack.SecureRNG()`. Existing live behavior at `newProtocol()` (initial seeding at construction) is unchanged. ## Tests New file `pkg/tcpip/transport/tcp/protocol_state_test.go` adds two regression tests: - `TestProtocolSecretsHaveNosaveTag` is a reflection-based structural check that both secret fields carry `state:"nosave"`. Removing the tag in a future change would fail this test. - `TestProtocolAfterLoadRegeneratesSecrets` zeroes the two secrets (simulating the post-restore state for `nosave` fields), invokes `afterLoad`, and asserts both fields become non-zero and differ from the initial values. ``` $ bazel test //pkg/tcpip/transport/tcp:tcp_test --test_filter='TestProtocolSecretsHaveNosaveTag|TestProtocolAfterLoadRegeneratesSecrets' --- PASS: TestProtocolSecretsHaveNosaveTag (0.00s) --- PASS: TestProtocolAfterLoadRegeneratesSecrets (0.00s) PASS ``` `bazel build //pkg/tcpip/transport/tcp:tcp` builds clean against `master`. ## Framing This is hardening, not an advisory. Disclosure requires read access to a checkpoint file, which is host-side and orchestrator-controlled (`g3doc/user_guide/checkpoint_restore.md`). The fix is minimal, the convention already exists in the same file, and the patch closes a remaining persistence surface adjacent to the CVE-2024-10026 / NDSS 2025 secret-quality work. ## References - RFC 6528 (Defending against Sequence Number Attacks, Gont/Bellovin, 2012) - CVE-2024-10026 (NVD) - NDSS 2025: Kaplan, Even, Klein. "You Can Rand but You Can't Hide." - gVisor `g3doc/user_guide/checkpoint_restore.md` - Sibling `state:"nosave"`: `pkg/tcpip/stack/stack.go:159`, `pkg/tcpip/transport/tcp/protocol.go:92,115`. FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/gvisor/pull/13164 from ibondarenko1:hardening/tcp-isn-secrets-nosave b027f391933f14cf4cb2c03522f960a40ba4aa0b PiperOrigin-RevId: 918043127 --- pkg/tcpip/transport/tcp/BUILD | 2 + pkg/tcpip/transport/tcp/protocol.go | 8 +- pkg/tcpip/transport/tcp/protocol_state.go | 36 ++++++++ .../transport/tcp/protocol_state_test.go | 90 +++++++++++++++++++ 4 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 pkg/tcpip/transport/tcp/protocol_state.go create mode 100644 pkg/tcpip/transport/tcp/protocol_state_test.go diff --git a/pkg/tcpip/transport/tcp/BUILD b/pkg/tcpip/transport/tcp/BUILD index 2087d7efcd..a572782463 100644 --- a/pkg/tcpip/transport/tcp/BUILD +++ b/pkg/tcpip/transport/tcp/BUILD @@ -162,6 +162,7 @@ go_library( "pending_processing_mutex.go", "protocol.go", "protocol_mutex.go", + "protocol_state.go", "rack.go", "rcv.go", "rcv_queue_mutex.go", @@ -217,6 +218,7 @@ go_test( srcs = [ "cubic_test.go", "main_test.go", + "protocol_state_test.go", "sack_scoreboard_test.go", "segment_test.go", "timer_test.go", diff --git a/pkg/tcpip/transport/tcp/protocol.go b/pkg/tcpip/transport/tcp/protocol.go index ec80705d19..7c911632b2 100644 --- a/pkg/tcpip/transport/tcp/protocol.go +++ b/pkg/tcpip/transport/tcp/protocol.go @@ -114,9 +114,11 @@ type protocol struct { // This is immutable after creation. probe TCPProbeFunc `state:"nosave"` - // The following secrets are initialized once and stay unchanged after. - seqnumSecret [16]byte - tsOffsetSecret [16]byte + // The following secrets are used for ISN and timestamp-offset + // generation. They are not serialized into checkpoint state and are + // freshly drawn from the secure RNG on restore (see afterLoad). + seqnumSecret [16]byte `state:"nosave"` + tsOffsetSecret [16]byte `state:"nosave"` } // Number returns the tcp protocol number. diff --git a/pkg/tcpip/transport/tcp/protocol_state.go b/pkg/tcpip/transport/tcp/protocol_state.go new file mode 100644 index 0000000000..4813eff948 --- /dev/null +++ b/pkg/tcpip/transport/tcp/protocol_state.go @@ -0,0 +1,36 @@ +// Copyright 2026 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tcp + +import ( + "context" + "fmt" +) + +// afterLoad is invoked by stateify after the protocol struct is restored +// from a checkpoint. seqnumSecret and tsOffsetSecret are tagged +// state:"nosave" and therefore arrive zero-valued on restore; this hook +// draws fresh 16-byte secrets from the stack secure RNG so that the +// restored protocol does not reuse secret material that could have been +// read out of the checkpoint file. +func (p *protocol) afterLoad(ctx context.Context) { + rng := p.stack.SecureRNG() + if n, err := rng.Reader.Read(p.seqnumSecret[:]); err != nil || n != len(p.seqnumSecret) { + panic(fmt.Sprintf("rng.Reader.Read(seqnumSecret) failed: n=%d err=%v", n, err)) + } + if n, err := rng.Reader.Read(p.tsOffsetSecret[:]); err != nil || n != len(p.tsOffsetSecret) { + panic(fmt.Sprintf("rng.Reader.Read(tsOffsetSecret) failed: n=%d err=%v", n, err)) + } +} diff --git a/pkg/tcpip/transport/tcp/protocol_state_test.go b/pkg/tcpip/transport/tcp/protocol_state_test.go new file mode 100644 index 0000000000..ce58d46a2f --- /dev/null +++ b/pkg/tcpip/transport/tcp/protocol_state_test.go @@ -0,0 +1,90 @@ +// Copyright 2026 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tcp + +import ( + "bytes" + "context" + "reflect" + "testing" + + "gvisor.dev/gvisor/pkg/tcpip/stack" +) + +// TestProtocolSecretsHaveNosaveTag is a structural assertion that the +// seqnumSecret and tsOffsetSecret fields on the protocol struct carry the +// state:"nosave" tag. Removing the tag would silently restore stale +// secrets across checkpoint, defeating afterLoad re-seeding. +func TestProtocolSecretsHaveNosaveTag(t *testing.T) { + var p protocol + typ := reflect.TypeOf(p) + for _, name := range []string{"seqnumSecret", "tsOffsetSecret"} { + f, ok := typ.FieldByName(name) + if !ok { + t.Fatalf("field %s not found on protocol", name) + } + if got, want := f.Tag.Get("state"), "nosave"; got != want { + t.Errorf("field %s state-tag = %q, want %q", name, got, want) + } + } +} + +// TestProtocolAfterLoadRegeneratesSecrets verifies that calling afterLoad on +// a protocol whose secret fields are zero-valued (the post-restore state for +// nosave fields) repopulates them with fresh, non-zero bytes drawn from the +// stack secure RNG. +func TestProtocolAfterLoadRegeneratesSecrets(t *testing.T) { + s := stack.New(stack.Options{ + TransportProtocols: []stack.TransportProtocolFactory{NewProtocol}, + }) + defer s.Destroy() + + tp := s.TransportProtocolInstance(ProtocolNumber) + p, ok := tp.(*protocol) + if !ok { + t.Fatalf("transport protocol instance = %T, want *protocol", tp) + } + + var initialSeq, initialTS [16]byte + copy(initialSeq[:], p.seqnumSecret[:]) + copy(initialTS[:], p.tsOffsetSecret[:]) + + var zero [16]byte + if bytes.Equal(initialSeq[:], zero[:]) { + t.Fatalf("seqnumSecret was zero before afterLoad call (RNG init failed)") + } + if bytes.Equal(initialTS[:], zero[:]) { + t.Fatalf("tsOffsetSecret was zero before afterLoad call (RNG init failed)") + } + + // Simulate post-restore: stateify gives nosave fields their zero value. + p.seqnumSecret = [16]byte{} + p.tsOffsetSecret = [16]byte{} + + p.afterLoad(context.Background()) + + if bytes.Equal(p.seqnumSecret[:], zero[:]) { + t.Errorf("seqnumSecret zero after afterLoad: %x", p.seqnumSecret) + } + if bytes.Equal(p.tsOffsetSecret[:], zero[:]) { + t.Errorf("tsOffsetSecret zero after afterLoad: %x", p.tsOffsetSecret) + } + if bytes.Equal(p.seqnumSecret[:], initialSeq[:]) { + t.Errorf("seqnumSecret unchanged after afterLoad (collision unlikely with 16-byte secret): %x", p.seqnumSecret) + } + if bytes.Equal(p.tsOffsetSecret[:], initialTS[:]) { + t.Errorf("tsOffsetSecret unchanged after afterLoad (collision unlikely with 16-byte secret): %x", p.tsOffsetSecret) + } +}