From b0f50ab9c9cb54d58e91eee7fce0a42a47edd8ba Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Thu, 11 Jan 2024 10:49:03 -0800 Subject: [PATCH 1/2] WSL-Helper: Certs: Make a copy of foreign memory We enumerate system certificates on Windows asynchronously and return the results (as *x509.Certificate objects) in a channel. It turns out that those certificates can refer to memory passed in via ParseCertificate(), so we ended up using a certificate that referred to freed memory. Avoid the issue by explicitly making a copy of that slice. Signed-off-by: Mark Yen --- .../wsl-helper/pkg/certificates/certificates_windows.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/go/wsl-helper/pkg/certificates/certificates_windows.go b/src/go/wsl-helper/pkg/certificates/certificates_windows.go index 80c98b7482b..881b2aee7c1 100644 --- a/src/go/wsl-helper/pkg/certificates/certificates_windows.go +++ b/src/go/wsl-helper/pkg/certificates/certificates_windows.go @@ -74,7 +74,13 @@ func GetSystemCertificates(storeName string) (<-chan Entry, error) { } break } - cert, err := x509.ParseCertificate(unsafe.Slice(certCtx.EncodedCert, certCtx.Length)) + // Make a copy of the encoded cert, because the parsed cert may have + // references to the memory (that isn't owned by the GC) and we'll return + // it in a channel, so HeapFree() might get called on it before it's used. + // See #6295 / #6307. + certData := make([]byte, certCtx.Length) + copy(certData, unsafe.Slice(certCtx.EncodedCert, certCtx.Length)) + cert, err := x509.ParseCertificate(certData) if err != nil { // Skip invalid certs logrus.Tracef("Skipping invalid certificate %q in %q: %s", getCertName(certCtx), storeName, err) From 765822c5e0ffd45241e0314336ed93dfd4898b26 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Thu, 11 Jan 2024 11:43:57 -0800 Subject: [PATCH 2/2] WSL-Helper: Add test for cert use-after-free Signed-off-by: Mark Yen --- .github/actions/spelling/expect.txt | 1 + .../certificates/certificates_windows_test.go | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 src/go/wsl-helper/pkg/certificates/certificates_windows_test.go diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 3c6fae75f2b..b54510f379d 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -243,6 +243,7 @@ fav fcb fdx featurename +FEEEFEEE femto ffi ficlone diff --git a/src/go/wsl-helper/pkg/certificates/certificates_windows_test.go b/src/go/wsl-helper/pkg/certificates/certificates_windows_test.go new file mode 100644 index 00000000000..51157aa106c --- /dev/null +++ b/src/go/wsl-helper/pkg/certificates/certificates_windows_test.go @@ -0,0 +1,47 @@ +package certificates_test + +import ( + "bytes" + "crypto/x509" + "encoding/pem" + "testing" + + "github.com/rancher-sandbox/rancher-desktop/src/go/wsl-helper/pkg/certificates" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Test that we don't use memory that we don't own +func TestGetSystemCertificates_UseAfterFree(t *testing.T) { + var certs []*x509.Certificate + ch, err := certificates.GetSystemCertificates("CA") + require.NoError(t, err, "failed to get CA certificates") + for entry := range ch { + if assert.NoError(t, err, entry.Err) { + certs = append(certs, entry.Cert) + } + } + ch, err = certificates.GetSystemCertificates("ROOT") + require.NoError(t, err, "failed to get ROOT certificates") + for entry := range ch { + if assert.NoError(t, err, entry.Err) { + certs = append(certs, entry.Cert) + } + } + + // By this point, both channels have been closed, which also means we have + // closed both cert stores. + for _, cert := range certs { + buf := bytes.Buffer{} + block := &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw} + err = pem.Encode(&buf, block) + if assert.NoError(t, err, "Failed to encode certificate") { + // Look for invalid certificates: + // - A line of all A (nulls) + // - A line with 0xFEEEFEEE (HeapAlloc freed marker) + output := buf.String() + assert.NotContains(t, output, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "encoded cert contains nulls") + assert.NotContains(t, output, "7v7u/u7+7v7u/u7+7v7u/u7+7v7u/u7+7v7u/u7+7v7u/u7+7v7u/u7+7v7u/u7+", "encoded cert contains FEEEFEEE") + } + } +}