From 17ba8fb3fa8b76dbd6d5ef99b2f88cc16822c8ff Mon Sep 17 00:00:00 2001 From: toki Date: Sun, 2 Aug 2026 12:16:43 +0900 Subject: [PATCH] =?UTF-8?q?fix(credential):=20Windows=20=ED=82=A4=20?= =?UTF-8?q?=ED=8C=8C=EC=9D=BC=20=EA=B2=80=EC=A6=9D=EC=9D=84=20=EC=A7=80?= =?UTF-8?q?=EC=9B=90=ED=95=9C=EB=8B=A4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/go/credentiallease/envelope.go | 8 +- .../go/credentiallease/keyfile_other_test.go | 10 +++ .../credentiallease/keyfile_security_other.go | 9 ++ .../credentiallease/keyfile_security_unix.go | 16 ++++ .../keyfile_security_windows.go | 83 +++++++++++++++++++ packages/go/credentiallease/keyfile_test.go | 44 ++++++++++ .../go/credentiallease/keyfile_unix_test.go | 15 ++++ .../credentiallease/keyfile_windows_test.go | 64 ++++++++++++++ 8 files changed, 242 insertions(+), 7 deletions(-) create mode 100644 packages/go/credentiallease/keyfile_other_test.go create mode 100644 packages/go/credentiallease/keyfile_security_other.go create mode 100644 packages/go/credentiallease/keyfile_security_unix.go create mode 100644 packages/go/credentiallease/keyfile_security_windows.go create mode 100644 packages/go/credentiallease/keyfile_test.go create mode 100644 packages/go/credentiallease/keyfile_unix_test.go create mode 100644 packages/go/credentiallease/keyfile_windows_test.go diff --git a/packages/go/credentiallease/envelope.go b/packages/go/credentiallease/envelope.go index d4734796..908633ec 100644 --- a/packages/go/credentiallease/envelope.go +++ b/packages/go/credentiallease/envelope.go @@ -21,7 +21,6 @@ import ( "path/filepath" "strings" "sync" - "syscall" "time" ) @@ -394,14 +393,9 @@ func readKeyFile(path string, size int, private bool) ([]byte, error) { if err != nil || !info.Mode().IsRegular() { return nil, ErrKey } - if private && info.Mode().Perm()&0o077 != 0 { + if private && !privateKeyFileSecure(file, info) { return nil, ErrKey } - if private { - if st, ok := info.Sys().(*syscall.Stat_t); !ok || (st.Uid != uint32(os.Getuid()) && st.Uid != 0) { - return nil, ErrKey - } - } raw, err := io.ReadAll(io.LimitReader(file, int64(size*4+129))) if err != nil { return nil, ErrKey diff --git a/packages/go/credentiallease/keyfile_other_test.go b/packages/go/credentiallease/keyfile_other_test.go new file mode 100644 index 00000000..61d0b8c5 --- /dev/null +++ b/packages/go/credentiallease/keyfile_other_test.go @@ -0,0 +1,10 @@ +//go:build !unix && !windows + +package credentiallease + +import "testing" + +func secureTestKeyFile(t *testing.T, _ string) { + t.Helper() + t.Skip("private key files are unsupported on this platform") +} diff --git a/packages/go/credentiallease/keyfile_security_other.go b/packages/go/credentiallease/keyfile_security_other.go new file mode 100644 index 00000000..d285d752 --- /dev/null +++ b/packages/go/credentiallease/keyfile_security_other.go @@ -0,0 +1,9 @@ +//go:build !unix && !windows + +package credentiallease + +import "os" + +// Unknown permission models fail closed until the platform has an explicit +// ownership and access-control implementation. +func privateKeyFileSecure(_ *os.File, _ os.FileInfo) bool { return false } diff --git a/packages/go/credentiallease/keyfile_security_unix.go b/packages/go/credentiallease/keyfile_security_unix.go new file mode 100644 index 00000000..64772823 --- /dev/null +++ b/packages/go/credentiallease/keyfile_security_unix.go @@ -0,0 +1,16 @@ +//go:build unix + +package credentiallease + +import ( + "os" + "syscall" +) + +func privateKeyFileSecure(_ *os.File, info os.FileInfo) bool { + if info.Mode().Perm()&0o077 != 0 { + return false + } + st, ok := info.Sys().(*syscall.Stat_t) + return ok && (st.Uid == uint32(os.Getuid()) || st.Uid == 0) +} diff --git a/packages/go/credentiallease/keyfile_security_windows.go b/packages/go/credentiallease/keyfile_security_windows.go new file mode 100644 index 00000000..4114d17e --- /dev/null +++ b/packages/go/credentiallease/keyfile_security_windows.go @@ -0,0 +1,83 @@ +//go:build windows + +package credentiallease + +import ( + "os" + "unsafe" + + "golang.org/x/sys/windows" +) + +const ( + accessAllowedObjectACEType = 5 + accessAllowedCallbackACEType = 9 + accessAllowedCallbackObjectACEType = 11 +) + +// privateKeyFileSecure validates Windows ownership and discretionary ACLs. +// FileMode permission bits do not represent Windows ACLs, so checking 0600 +// would reject every normal Windows key file without proving confidentiality. +func privateKeyFileSecure(file *os.File, _ os.FileInfo) bool { + sd, err := windows.GetSecurityInfo( + windows.Handle(file.Fd()), + windows.SE_FILE_OBJECT, + windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION, + ) + if err != nil || sd == nil || !sd.IsValid() { + return false + } + + current, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil || current == nil || current.User.Sid == nil { + return false + } + owner, _, err := sd.Owner() + if err != nil || !trustedKeyFileSID(owner, current.User.Sid) { + return false + } + + dacl, _, err := sd.DACL() + if err != nil || dacl == nil { + return false + } + for i := uint32(0); i < uint32(dacl.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, i, &ace); err != nil || ace == nil { + return false + } + if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { + continue + } + switch ace.Header.AceType { + case windows.ACCESS_DENIED_ACE_TYPE: + continue + case windows.ACCESS_ALLOWED_ACE_TYPE: + if !aceGrantsFileRead(ace.Mask) { + continue + } + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + if !trustedKeyFileSID(sid, current.User.Sid) { + return false + } + case accessAllowedObjectACEType, accessAllowedCallbackACEType, accessAllowedCallbackObjectACEType: + // These layouts carry additional fields before the SID. Reject them + // instead of guessing whether an untrusted principal can read the key. + return false + default: + return false + } + } + return true +} + +func aceGrantsFileRead(mask windows.ACCESS_MASK) bool { + const readMask = windows.FILE_READ_DATA | windows.GENERIC_READ | windows.GENERIC_ALL | windows.MAXIMUM_ALLOWED + return mask&readMask != 0 +} + +func trustedKeyFileSID(candidate, current *windows.SID) bool { + return candidate != nil && current != nil && (candidate.Equals(current) || + candidate.IsWellKnown(windows.WinLocalSystemSid) || + candidate.IsWellKnown(windows.WinBuiltinAdministratorsSid)) +} diff --git a/packages/go/credentiallease/keyfile_test.go b/packages/go/credentiallease/keyfile_test.go new file mode 100644 index 00000000..67c1b88e --- /dev/null +++ b/packages/go/credentiallease/keyfile_test.go @@ -0,0 +1,44 @@ +package credentiallease + +import ( + "bytes" + "encoding/base64" + "errors" + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestLoadPrivateKeyFile(t *testing.T) { + want := bytes.Repeat([]byte{0x5a}, 32) + path := filepath.Join(t.TempDir(), "recipient.key") + if err := os.WriteFile(path, []byte(base64.StdEncoding.EncodeToString(want)), 0o600); err != nil { + t.Fatal(err) + } + secureTestKeyFile(t, path) + + got, err := LoadPrivateKeyFile(path, len(want)) + if err != nil { + t.Fatalf("LoadPrivateKeyFile() error = %v", err) + } + if !bytes.Equal(got, want) { + t.Fatal("LoadPrivateKeyFile() returned unexpected key material") + } +} + +func TestLoadPrivateKeyFileRejectsInsecureUnixMode(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows private-key access is validated from the file DACL") + } + path := filepath.Join(t.TempDir(), "recipient.key") + encoded := base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{0x5a}, 32)) + if err := os.WriteFile(path, []byte(encoded), 0o644); err != nil { + t.Fatal(err) + } + + _, err := LoadPrivateKeyFile(path, 32) + if !errors.Is(err, ErrKey) { + t.Fatalf("LoadPrivateKeyFile() error = %v, want ErrKey", err) + } +} diff --git a/packages/go/credentiallease/keyfile_unix_test.go b/packages/go/credentiallease/keyfile_unix_test.go new file mode 100644 index 00000000..a7f1ee69 --- /dev/null +++ b/packages/go/credentiallease/keyfile_unix_test.go @@ -0,0 +1,15 @@ +//go:build unix + +package credentiallease + +import ( + "os" + "testing" +) + +func secureTestKeyFile(t *testing.T, path string) { + t.Helper() + if err := os.Chmod(path, 0o600); err != nil { + t.Fatal(err) + } +} diff --git a/packages/go/credentiallease/keyfile_windows_test.go b/packages/go/credentiallease/keyfile_windows_test.go new file mode 100644 index 00000000..46395496 --- /dev/null +++ b/packages/go/credentiallease/keyfile_windows_test.go @@ -0,0 +1,64 @@ +//go:build windows + +package credentiallease + +import ( + "bytes" + "encoding/base64" + "errors" + "os" + "path/filepath" + "testing" + + "golang.org/x/sys/windows" +) + +func secureTestKeyFile(t *testing.T, path string) { + t.Helper() + current, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatal(err) + } + setTestKeyFileDACL(t, path, "D:P(A;;FA;;;"+current.User.Sid.String()+")(A;;FA;;;SY)(A;;FA;;;BA)") +} + +func TestLoadPrivateKeyFileRejectsUntrustedWindowsReader(t *testing.T) { + path := filepath.Join(t.TempDir(), "recipient.key") + encoded := base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{0x5a}, 32)) + if err := os.WriteFile(path, []byte(encoded), 0o600); err != nil { + t.Fatal(err) + } + current, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatal(err) + } + setTestKeyFileDACL(t, path, "D:P(A;;FA;;;"+current.User.Sid.String()+")(A;;FR;;;WD)") + + _, err = LoadPrivateKeyFile(path, 32) + if !errors.Is(err, ErrKey) { + t.Fatalf("LoadPrivateKeyFile() error = %v, want ErrKey", err) + } +} + +func setTestKeyFileDACL(t *testing.T, path, sddl string) { + t.Helper() + sd, err := windows.SecurityDescriptorFromString(sddl) + if err != nil { + t.Fatal(err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatal(err) + } + if err := windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, + nil, + dacl, + nil, + ); err != nil { + t.Fatal(err) + } +}