Conform SID string format to MS-DTYP 2.4.2.1 (hex authority + no spurious RID) (Fixes #104) (#105)

Author: p0dalirius

sid/SecurityIdentifier.go

@@ -404,8 +404,16 @@ func (sid *SID) FromString(sidString string) error {
 	// Parse the identifier authority (S-<Revision>-<IdentifierAuthority>).
 	// The binary SID format stores this in 6 bytes (see MS-DTYP 2.4.2.1
 	// SID_IDENTIFIER_AUTHORITY), so reject values that cannot be represented
-	// rather than silently truncating them at marshal time.
-	identifierAuthority, err := strconv.ParseUint(parts[2], 10, 64)
+	// rather than silently truncating them at marshal time. Per MS-DTYP 2.4.2.1
+	// an identifier authority >= 2^32 is written in hexadecimal ("0x" followed
+	// by hex digits); smaller values are decimal.
+	authorityToken := parts[2]
+	var identifierAuthority uint64
+	if len(authorityToken) > 2 && (authorityToken[0:2] == "0x" || authorityToken[0:2] == "0X") {
+		identifierAuthority, err = strconv.ParseUint(authorityToken[2:], 16, 64)
+	} else {
+		identifierAuthority, err = strconv.ParseUint(authorityToken, 10, 64)
+	}
 	if err != nil {
 		return fmt.Errorf("invalid identifier authority in SID: %v", err)
 	}
@@ -451,13 +459,32 @@ func (sid *SID) FromString(sidString string) error {
 //     This includes the revision level, identifier authority, all sub-authorities, and the
 //     relative identifier (RID).
 func (sid *SID) ToString() string {
-	sidstring := fmt.Sprintf("S-%d-%d", sid.RevisionLevel, sid.IdentifierAuthority.Value)
+	// Per MS-DTYP 2.4.2.1, an identifier authority value less than 2^32 is
+	// written in decimal, while a value >= 2^32 is written in hexadecimal as
+	// "0x" followed by 12 hex digits.
+	var authorityStr string
+	if sid.IdentifierAuthority.Value >= (uint64(1) << 32) {
+		authorityStr = fmt.Sprintf("0x%012x", sid.IdentifierAuthority.Value)
+	} else {
+		authorityStr = fmt.Sprintf("%d", sid.IdentifierAuthority.Value)
+	}
+	sidstring := fmt.Sprintf("S-%d-%s", sid.RevisionLevel, authorityStr)
 
 	for k := range sid.SubAuthorities {
 		sidstring += fmt.Sprintf("-%d", sid.SubAuthorities[k])
 	}
 
-	sidstring += fmt.Sprintf("-%d", sid.RelativeIdentifier)
+	// Append the RID only when the SID actually carries a final sub-authority.
+	// A SID with no sub-authorities at all — SubAuthorityCount == 0, no entries
+	// in SubAuthorities, and a zero RID, e.g. the bare NT Authority SID S-1-5
+	// produced by Unmarshal — has no RID; appending RelativeIdentifier
+	// unconditionally fabricated a spurious trailing "-0", producing the string
+	// of a different SID. Any evidence of a RID (a non-zero count, prior
+	// sub-authorities, or a non-zero RID value) means one is present and must be
+	// emitted.
+	if sid.SubAuthorityCount > 0 || len(sid.SubAuthorities) > 0 || sid.RelativeIdentifier != 0 {
+		sidstring += fmt.Sprintf("-%d", sid.RelativeIdentifier)
+	}
 
 	return sidstring
 }

sid/SecurityIdentifier_string_conformance_test.go

@@ -0,0 +1,80 @@
+package sid_test
+
+import (
+	"testing"
+
+	"github.com/TheManticoreProject/winacl/sid"
+)
+
+// TestSID_ToString_NoSpuriousRID is a regression test for ToString fabricating a
+// trailing "-0" for a SID with SubAuthorityCount == 0. A bare NT Authority SID
+// (binary: revision 1, 0 sub-authorities, identifier authority 5) must render
+// as "S-1-5", not "S-1-5-0" (which is the canonical string of a different SID).
+func TestSID_ToString_NoSpuriousRID(t *testing.T) {
+	// 01 00 000000000005 : Revision 1, SubAuthorityCount 0, IdentifierAuthority 5.
+	bare := []byte{0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05}
+
+	s := &sid.SID{}
+	if _, err := s.Unmarshal(bare); err != nil {
+		t.Fatalf("Unmarshal() error = %v", err)
+	}
+	if s.SubAuthorityCount != 0 {
+		t.Fatalf("SubAuthorityCount = %d, want 0", s.SubAuthorityCount)
+	}
+	if got := s.ToString(); got != "S-1-5" {
+		t.Fatalf("ToString() = %q, want %q", got, "S-1-5")
+	}
+
+	// A normal SID must still include its RID.
+	normal := &sid.SID{}
+	if err := normal.FromString("S-1-5-32-544"); err != nil {
+		t.Fatalf("FromString() error = %v", err)
+	}
+	if got := normal.ToString(); got != "S-1-5-32-544" {
+		t.Fatalf("ToString() = %q, want %q", got, "S-1-5-32-544")
+	}
+}
+
+// TestSID_HexIdentifierAuthority is a regression test for the identifier
+// authority >= 2^32 not being formatted/parsed in the hexadecimal form that
+// MS-DTYP 2.4.2.1 mandates ("0x" followed by 12 hex digits).
+func TestSID_HexIdentifierAuthority(t *testing.T) {
+	// 2^40 = 0x10000000000, which is >= 2^32 and within the 48-bit field.
+	const authority = uint64(1) << 40
+	const wantStr = "S-1-0x010000000000-1-2"
+
+	// ToString must emit the hex form.
+	s := &sid.SID{}
+	if err := s.FromString("S-1-1099511627776-1-2"); err != nil {
+		t.Fatalf("FromString(decimal) error = %v", err)
+	}
+	if s.IdentifierAuthority.Value != authority {
+		t.Fatalf("parsed authority = %d, want %d", s.IdentifierAuthority.Value, authority)
+	}
+	if got := s.ToString(); got != wantStr {
+		t.Fatalf("ToString() = %q, want %q", got, wantStr)
+	}
+
+	// FromString must accept the hex form and decode the same value.
+	h := &sid.SID{}
+	if err := h.FromString(wantStr); err != nil {
+		t.Fatalf("FromString(hex) error = %v", err)
+	}
+	if h.IdentifierAuthority.Value != authority {
+		t.Fatalf("hex-parsed authority = %d, want %d", h.IdentifierAuthority.Value, authority)
+	}
+
+	// Round-trip: the hex string re-serializes and re-parses to the same value.
+	if got := h.ToString(); got != wantStr {
+		t.Fatalf("hex round-trip ToString() = %q, want %q", got, wantStr)
+	}
+
+	// A value < 2^32 must still be decimal.
+	dec := &sid.SID{}
+	if err := dec.FromString("S-1-5-18"); err != nil {
+		t.Fatalf("FromString() error = %v", err)
+	}
+	if got := dec.ToString(); got != "S-1-5-18" {
+		t.Fatalf("ToString() = %q, want %q (small authority must stay decimal)", got, "S-1-5-18")
+	}
+}