Skip to content

Commit e0ded4e

Browse files
p0daliriusp0dalirius
authored
Merge pull request #89 from TheManticoreProject/enhancement-sddl-tokenizer-parenthesis-aware
Make SDDL tokenizer parenthesis-aware and surface errors on malformed input (#80)
2 parents ee7fc53 + ee087d9 commit e0ded4e

2 files changed

Lines changed: 134 additions & 24 deletions

File tree

‎sddl/sddl.go‎

Lines changed: 80 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,33 @@
11
package sddl
22

33
import (
4+
"fmt"
45
"strings"
56
)
67

78
// CutSDDL parses an SDDL string into its component parts.
89
//
10+
// The scan is parenthesis-aware: the O:, G:, D:, and S: component markers are
11+
// only recognised at the top level (parenthesis depth 0), so a ':' or a marker
12+
// letter appearing inside an ACE body (for example in a conditional or
13+
// resource-attribute ACE) does not split the string. Malformed input — leading
14+
// characters before the first marker, or unbalanced parentheses — is reported
15+
// as an error instead of being silently discarded.
16+
//
917
// Parameters:
1018
// - sddlString (string): The SDDL string to parse.
1119
//
1220
// Returns:
13-
// - (string, string, []string, []string): The owner SID, group SID, DACL ACEs, and SACL ACEs.
14-
func CutSDDL(sddlString string) (string, string, []string, []string) {
21+
// - (string, string, []string, []string, error): The owner SID, group SID,
22+
// DACL ACEs, SACL ACEs, and an error if the string is malformed.
23+
func CutSDDL(sddlString string) (string, string, []string, []string, error) {
1524
sddlString = strings.TrimSpace(sddlString)
1625

1726
if len(sddlString) == 0 {
18-
return "", "", nil, nil
27+
return "", "", nil, nil, nil
1928
}
2029

21-
// Match components starting with O:, G:, D:, or S: using regex
30+
// Match components starting with O:, G:, D:, or S:.
2231
components := map[string]string{
2332
"O:": "",
2433
"G:": "",
@@ -27,41 +36,79 @@ func CutSDDL(sddlString string) (string, string, []string, []string) {
2736
}
2837

2938
currentComponent := ""
39+
depth := 0
3040
k := 0
3141
for k < len(sddlString) {
32-
upperChar := strings.ToUpper(string(sddlString[k]))
33-
if k+1 < len(sddlString) && (upperChar == "O" || upperChar == "G" || upperChar == "D" || upperChar == "S") && sddlString[k+1] == ':' {
34-
// Normalise the key to uppercase so lowercase markers (o:, g:, d:, s:)
35-
// land in the same bucket as their uppercase counterparts; SDDL is
36-
// case-insensitive at the component-marker level.
37-
currentComponent = upperChar + ":"
38-
k += 2
39-
continue
42+
c := sddlString[k]
43+
44+
// A component marker can only start at the top level. Recognising markers
45+
// at depth > 0 would let an ACE body containing "D:" (etc.) be mistaken
46+
// for a new component.
47+
if depth == 0 && k+1 < len(sddlString) && sddlString[k+1] == ':' {
48+
upperChar := strings.ToUpper(string(c))
49+
if upperChar == "O" || upperChar == "G" || upperChar == "D" || upperChar == "S" {
50+
// Normalise the key to uppercase so lowercase markers (o:, g:, d:,
51+
// s:) land in the same bucket as their uppercase counterparts; SDDL
52+
// is case-insensitive at the component-marker level.
53+
currentComponent = upperChar + ":"
54+
k += 2
55+
continue
56+
}
57+
}
58+
59+
switch c {
60+
case '(':
61+
depth++
62+
case ')':
63+
depth--
64+
if depth < 0 {
65+
return "", "", nil, nil, fmt.Errorf("malformed SDDL: unbalanced ')' at position %d", k)
66+
}
4067
}
41-
if currentComponent != "" {
42-
components[currentComponent] += string(sddlString[k])
68+
69+
if currentComponent == "" {
70+
// Any character before the first component marker is invalid; the
71+
// string has already been trimmed, so this is genuine garbage.
72+
return "", "", nil, nil, fmt.Errorf("malformed SDDL: unexpected character %q at position %d before any component marker", c, k)
4373
}
74+
components[currentComponent] += string(c)
4475
k++
4576
}
4677

47-
daclAces := CutAces(components["D:"])
48-
saclAces := CutAces(components["S:"])
78+
if depth != 0 {
79+
return "", "", nil, nil, fmt.Errorf("malformed SDDL: unbalanced '(' (missing %d closing parenthesis)", depth)
80+
}
81+
82+
daclAces, err := CutAces(components["D:"])
83+
if err != nil {
84+
return "", "", nil, nil, fmt.Errorf("invalid DACL: %w", err)
85+
}
86+
saclAces, err := CutAces(components["S:"])
87+
if err != nil {
88+
return "", "", nil, nil, fmt.Errorf("invalid SACL: %w", err)
89+
}
4990

50-
return components["O:"], components["G:"], daclAces, saclAces
91+
return components["O:"], components["G:"], daclAces, saclAces, nil
5192
}
5293

5394
// CutAces extracts individual ACE strings from a DACL/SACL component.
5495
// Handles the format: flags(ace1)(ace2)...(aceN)
55-
func CutAces(aclStr string) []string {
96+
//
97+
// Nested parentheses inside an ACE (for example in a conditional expression)
98+
// are preserved: only top-level parentheses delimit ACEs. Unbalanced
99+
// parentheses and stray characters between or after ACEs are reported as an
100+
// error rather than silently dropping or truncating ACEs.
101+
func CutAces(aclStr string) ([]string, error) {
56102
var aces []string
57103

58-
// Find first ( to separate flags
104+
// Find first ( to separate flags. No '(' means the component has no ACEs
105+
// (e.g. an empty DACL with only flags), which is valid.
59106
start := strings.Index(aclStr, "(")
60107
if start == -1 {
61-
return aces
108+
return aces, nil
62109
}
63110

64-
// Extract ACEs between parentheses
111+
// Extract ACEs between top-level parentheses.
65112
depth := 0
66113
aceStart := start
67114
for i := start; i < len(aclStr); i++ {
@@ -73,11 +120,22 @@ func CutAces(aclStr string) []string {
73120
depth++
74121
case ')':
75122
depth--
123+
if depth < 0 {
124+
return nil, fmt.Errorf("unbalanced ')' at position %d", i)
125+
}
76126
if depth == 0 {
77127
aces = append(aces, aclStr[aceStart:i])
78128
}
129+
default:
130+
if depth == 0 {
131+
return nil, fmt.Errorf("unexpected character %q at position %d (outside any ACE)", aclStr[i], i)
132+
}
79133
}
80134
}
81135

82-
return aces
136+
if depth != 0 {
137+
return nil, fmt.Errorf("unbalanced '(' (missing %d closing parenthesis)", depth)
138+
}
139+
140+
return aces, nil
83141
}

‎sddl/sddl_test.go‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,10 @@ func TestSddlCut(t *testing.T) {
6262

6363
for _, tt := range tests {
6464
t.Run(tt.name, func(t *testing.T) {
65-
gotOwner, gotGroup, gotDaclAces, gotSaclAces := sddl.CutSDDL(tt.input)
65+
gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(tt.input)
66+
if err != nil {
67+
t.Fatalf("CutSDDL() unexpected error = %v", err)
68+
}
6669

6770
if gotOwner != tt.wantOwner {
6871
t.Errorf("CutSDDL() owner = %v, want %v", gotOwner, tt.wantOwner)
@@ -135,7 +138,10 @@ func TestSddlCut_LowercaseMarkers(t *testing.T) {
135138

136139
for _, tt := range tests {
137140
t.Run(tt.name, func(t *testing.T) {
138-
gotOwner, gotGroup, gotDaclAces, gotSaclAces := sddl.CutSDDL(tt.input)
141+
gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(tt.input)
142+
if err != nil {
143+
t.Fatalf("CutSDDL() unexpected error = %v", err)
144+
}
139145

140146
if gotOwner != tt.wantOwner {
141147
t.Errorf("CutSDDL() owner = %q, want %q", gotOwner, tt.wantOwner)
@@ -153,6 +159,52 @@ func TestSddlCut_LowercaseMarkers(t *testing.T) {
153159
}
154160
}
155161

162+
// TestSddlCut_ConditionalAceSingleToken verifies that an ACE containing nested
163+
// parentheses and a ':' (as in a conditional expression) is tokenized as a
164+
// single ACE, and that a marker-like substring inside the ACE body does not
165+
// start a new component.
166+
func TestSddlCut_ConditionalAceSingleToken(t *testing.T) {
167+
// The inner "(@xD:1)" carries both nested parens and a "D:" substring.
168+
input := "D:(XA;;FA;;;WD;(@xD:1))"
169+
170+
gotOwner, gotGroup, gotDaclAces, gotSaclAces, err := sddl.CutSDDL(input)
171+
if err != nil {
172+
t.Fatalf("CutSDDL() unexpected error = %v", err)
173+
}
174+
if gotOwner != "" || gotGroup != "" {
175+
t.Errorf("CutSDDL() owner/group = %q/%q, want empty", gotOwner, gotGroup)
176+
}
177+
if len(gotSaclAces) != 0 {
178+
t.Errorf("CutSDDL() saclAces = %v, want none (inner \"D:\" must not start a component)", gotSaclAces)
179+
}
180+
want := []string{"XA;;FA;;;WD;(@xD:1)"}
181+
if !slices.Equal(gotDaclAces, want) {
182+
t.Errorf("CutSDDL() daclAces = %v, want %v", gotDaclAces, want)
183+
}
184+
}
185+
186+
// TestSddlCut_MalformedReturnsError verifies that malformed SDDL is rejected
187+
// with an error instead of being silently truncated.
188+
func TestSddlCut_MalformedReturnsError(t *testing.T) {
189+
tests := []struct {
190+
name string
191+
input string
192+
}{
193+
{name: "unbalanced open paren in DACL", input: "D:(A;;GA;;;WD"},
194+
{name: "extra close paren", input: "D:(A;;GA;;;WD))"},
195+
{name: "leading garbage before marker", input: "garbageO:BA"},
196+
{name: "trailing garbage after last ACE", input: "D:(A;;GA;;;WD)junk"},
197+
}
198+
199+
for _, tt := range tests {
200+
t.Run(tt.name, func(t *testing.T) {
201+
if _, _, _, _, err := sddl.CutSDDL(tt.input); err == nil {
202+
t.Errorf("CutSDDL(%q) = nil error, want error", tt.input)
203+
}
204+
})
205+
}
206+
}
207+
156208
// func TestSDDLToBinary(t *testing.T) {
157209
// tests := []struct {
158210
// name string

0 commit comments

Comments
 (0)