From b027f391933f14cf4cb2c03522f960a40ba4aa0b Mon Sep 17 00:00:00 2001 From: ibondarenko1 Date: Wed, 13 May 2026 11:49:13 -0700 Subject: [PATCH] tcp: mark ISN and timestamp-offset secrets nosave for checkpoint The TCP protocol struct in pkg/tcpip/transport/tcp/protocol.go is +stateify savable. seqnumSecret and tsOffsetSecret are 16-byte CSPRNG secrets used for ISN and timestamp-offset generation (RFC 6528). Neither field carried the state:"nosave" tag, so both 16-byte values were written verbatim into the checkpoint state stream by the generator. A reader of a checkpoint file could recover the secrets and predict ISNs and timestamp offsets on the restored sandbox. Sibling fields in the same struct already carry the tag (mu at protocol.go:92, probe at protocol.go:115). secureRNG itself is state:"nosave" at pkg/tcpip/stack/stack.go:159. The absence of the tag on the two secret fields is an omission against the established convention rather than a design choice. Tag both fields state:"nosave" and add a protocol.afterLoad hook that redraws fresh 16-byte secrets from stack.SecureRNG() on restore. Save: the two fields are omitted 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. Adds two regression tests in protocol_state_test.go: a reflection check that the tag is present, and a runtime check that afterLoad repopulates zero-valued fields with fresh non-zero bytes drawn from the secure RNG. Tested: bazel build //pkg/tcpip/transport/tcp:tcp bazel test //pkg/tcpip/transport/tcp:tcp_test \ --test_filter='TestProtocolSecretsHaveNosaveTag|TestProtocolAfterLoadRegeneratesSecrets' Related: CVE-2024-10026 and the NDSS 2025 paper "You Can Rand but You Can't Hide" (Kaplan, Even, Klein) improved the cryptographic quality of these same secrets (commits 83f75082e5, e54bfde792, f956b5ac17) but did not address persistence into checkpoint state. --- 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) + } +}