[bug] Make SDDL tokenizer parenthesis-aware and surface errors on malformed input (#80)

sddl/sddl.go

@@ -1,24 +1,33 @@
 package sddl
 
 import (
+	"fmt"
 	"strings"
 )
 
 // CutSDDL parses an SDDL string into its component parts.
 //
+// The scan is parenthesis-aware: the O:, G:, D:, and S: component markers are
+// only recognised at the top level (parenthesis depth 0), so a ':' or a marker
+// letter appearing inside an ACE body (for example in a conditional or
+// resource-attribute ACE) does not split the string. Malformed input — leading
+// characters before the first marker, or unbalanced parentheses — is reported
+// as an error instead of being silently discarded.
+//
 // Parameters:
 //   - sddlString (string): The SDDL string to parse.
 //
 // Returns:
-//   - (string, string, []string, []string): The owner SID, group SID, DACL ACEs, and SACL ACEs.
-func CutSDDL(sddlString string) (string, string, []string, []string) {
+//   - (string, string, []string, []string, error): The owner SID, group SID,
+//     DACL ACEs, SACL ACEs, and an error if the string is malformed.
+func CutSDDL(sddlString string) (string, string, []string, []string, error) {
 	sddlString = strings.TrimSpace(sddlString)
 
 	if len(sddlString) == 0 {
-		return "", "", nil, nil
+		return "", "", nil, nil, nil
 	}
 
-	// Match components starting with O:, G:, D:, or S: using regex
+	// Match components starting with O:, G:, D:, or S:.
 	components := map[string]string{
 		"O:": "",
 		"G:": "",
@@ -27,41 +36,79 @@ func CutSDDL(sddlString string) (string, string, []string, []string) {
 	}
 
 	currentComponent := ""
+	depth := 0
 	k := 0
 	for k < len(sddlString) {
-		upperChar := strings.ToUpper(string(sddlString[k]))
-		if k+1 < len(sddlString) && (upperChar == "O" || upperChar == "G" || upperChar == "D" || upperChar == "S") && sddlString[k+1] == ':' {
-			// Normalise the key to uppercase so lowercase markers (o:, g:, d:, s:)
-			// land in the same bucket as their uppercase counterparts; SDDL is
-			// case-insensitive at the component-marker level.
-			currentComponent = upperChar + ":"
-			k += 2
-			continue
+		c := sddlString[k]
+
+		// A component marker can only start at the top level. Recognising markers
+		// at depth > 0 would let an ACE body containing "D:" (etc.) be mistaken
+		// for a new component.
+		if depth == 0 && k+1 < len(sddlString) && sddlString[k+1] == ':' {
+			upperChar := strings.ToUpper(string(c))
+			if upperChar == "O" || upperChar == "G" || upperChar == "D" || upperChar == "S" {
+				// Normalise the key to uppercase so lowercase markers (o:, g:, d:,
+				// s:) land in the same bucket as their uppercase counterparts; SDDL
+				// is case-insensitive at the component-marker level.
+				currentComponent = upperChar + ":"
+				k += 2
+				continue
+			}
+		}
+
+		switch c {
+		case '(':
+			depth++
+		case ')':
+			depth--
+			if depth < 0 {
+				return "", "", nil, nil, fmt.Errorf("malformed SDDL: unbalanced ')' at position %d", k)
+			}
 		}
-		if currentComponent != "" {
-			components[currentComponent] += string(sddlString[k])
+
+		if currentComponent == "" {
+			// Any character before the first component marker is invalid; the
+			// string has already been trimmed, so this is genuine garbage.
+			return "", "", nil, nil, fmt.Errorf("malformed SDDL: unexpected character %q at position %d before any component marker", c, k)
 		}
+		components[currentComponent] += string(c)
 		k++
 	}
 
-	daclAces := CutAces(components["D:"])
-	saclAces := CutAces(components["S:"])
+	if depth != 0 {
+		return "", "", nil, nil, fmt.Errorf("malformed SDDL: unbalanced '(' (missing %d closing parenthesis)", depth)
+	}
+
+	daclAces, err := CutAces(components["D:"])
+	if err != nil {
+		return "", "", nil, nil, fmt.Errorf("invalid DACL: %w", err)
+	}
+	saclAces, err := CutAces(components["S:"])
+	if err != nil {
+		return "", "", nil, nil, fmt.Errorf("invalid SACL: %w", err)
+	}
 
-	return components["O:"], components["G:"], daclAces, saclAces
+	return components["O:"], components["G:"], daclAces, saclAces, nil
 }
 
 // CutAces extracts individual ACE strings from a DACL/SACL component.
 // Handles the format: flags(ace1)(ace2)...(aceN)
-func CutAces(aclStr string) []string {
+//
+// Nested parentheses inside an ACE (for example in a conditional expression)
+// are preserved: only top-level parentheses delimit ACEs. Unbalanced
+// parentheses and stray characters between or after ACEs are reported as an
+// error rather than silently dropping or truncating ACEs.
+func CutAces(aclStr string) ([]string, error) {
 	var aces []string
 
-	// Find first ( to separate flags
+	// Find first ( to separate flags. No '(' means the component has no ACEs
+	// (e.g. an empty DACL with only flags), which is valid.
 	start := strings.Index(aclStr, "(")
 	if start == -1 {
-		return aces
+		return aces, nil
 	}
 
-	// Extract ACEs between parentheses
+	// Extract ACEs between top-level parentheses.
 	depth := 0
 	aceStart := start
 	for i := start; i < len(aclStr); i++ {
@@ -73,11 +120,22 @@ func CutAces(aclStr string) []string {
 			depth++
 		case ')':
 			depth--
+			if depth < 0 {
+				return nil, fmt.Errorf("unbalanced ')' at position %d", i)
+			}
 			if depth == 0 {
 				aces = append(aces, aclStr[aceStart:i])
 			}
+		default:
+			if depth == 0 {
+				return nil, fmt.Errorf("unexpected character %q at position %d (outside any ACE)", aclStr[i], i)
+			}
 		}
 	}
 
-	return aces
+	if depth != 0 {
+		return nil, fmt.Errorf("unbalanced '(' (missing %d closing parenthesis)", depth)
+	}
+
+	return aces, nil
 }

sddl/sddl_test.go

@@ -62,7 +62,10 @@ func TestSddlCut(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotOwner, gotGroup, gotDaclAces, gotSaclAces := sddl.CutSDDL(tt.input)
+			gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(tt.input)
+			if err != nil {
+				t.Fatalf("CutSDDL() unexpected error = %v", err)
+			}
 
 			if gotOwner != tt.wantOwner {
 				t.Errorf("CutSDDL() owner = %v, want %v", gotOwner, tt.wantOwner)
@@ -135,7 +138,10 @@ func TestSddlCut_LowercaseMarkers(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotOwner, gotGroup, gotDaclAces, gotSaclAces := sddl.CutSDDL(tt.input)
+			gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(tt.input)
+			if err != nil {
+				t.Fatalf("CutSDDL() unexpected error = %v", err)
+			}
 
 			if gotOwner != tt.wantOwner {
 				t.Errorf("CutSDDL() owner = %q, want %q", gotOwner, tt.wantOwner)
@@ -153,6 +159,52 @@ func TestSddlCut_LowercaseMarkers(t *testing.T) {
 	}
 }
 
+// TestSddlCut_ConditionalAceSingleToken verifies that an ACE containing nested
+// parentheses and a ':' (as in a conditional expression) is tokenized as a
+// single ACE, and that a marker-like substring inside the ACE body does not
+// start a new component.
+func TestSddlCut_ConditionalAceSingleToken(t *testing.T) {
+	// The inner "(@xD:1)" carries both nested parens and a "D:" substring.
+	input := "D:(XA;;FA;;;WD;(@xD:1))"
+
+	gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(input)
+	if err != nil {
+		t.Fatalf("CutSDDL() unexpected error = %v", err)
+	}
+	if gotOwner != "" || gotGroup != "" {
+		t.Errorf("CutSDDL() owner/group = %q/%q, want empty", gotOwner, gotGroup)
+	}
+	if len(gotSaclAces) != 0 {
+		t.Errorf("CutSDDL() saclAces = %v, want none (inner \"D:\" must not start a component)", gotSaclAces)
+	}
+	want := []string{"XA;;FA;;;WD;(@xD:1)"}
+	if !slices.Equal(gotDaclAces, want) {
+		t.Errorf("CutSDDL() daclAces = %v, want %v", gotDaclAces, want)
+	}
+}
+
+// TestSddlCut_MalformedReturnsError verifies that malformed SDDL is rejected
+// with an error instead of being silently truncated.
+func TestSddlCut_MalformedReturnsError(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+	}{
+		{name: "unbalanced open paren in DACL", input: "D:(A;;GA;;;WD"},
+		{name: "extra close paren", input: "D:(A;;GA;;;WD))"},
+		{name: "leading garbage before marker", input: "garbageO:BA"},
+		{name: "trailing garbage after last ACE", input: "D:(A;;GA;;;WD)junk"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if _, _, _, _, err := sddl.CutSDDL(tt.input); err == nil {
+				t.Errorf("CutSDDL(%q) = nil error, want error", tt.input)
+			}
+		})
+	}
+}
+
 // func TestSDDLToBinary(t *testing.T) {
 // 	tests := []struct {
 // 		name    string