Skip to content

Commit 9a78803

Browse files
mattnmattn
authored
Merge pull request #60 from scumfrog/security-fix-cve
fix(parser): prevent unintended command execution via unmatched ')'
2 parents 551a1d0 + b074fa0 commit 9a78803

2 files changed

Lines changed: 133 additions & 10 deletions

File tree

‎shellwords.go‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -200,23 +200,51 @@ loop:
200200
backtick = ""
201201
backQuote = !backQuote
202202
}
203+
203204
case ')':
204205
if !singleQuoted && !doubleQuoted && !backQuote {
205206
if p.ParseBacktick {
206-
if dollarQuote {
207-
out, err := shellRun(backtick, p.Dir)
208-
if err != nil {
209-
return nil, err
210-
}
211-
buf = buf[:len(buf)-len(backtick)-2] + out
207+
// Security fix:
208+
// A bare ')' must never open dollarQuote state.
209+
// Preserve prior behavior by rejecting unmatched ')'
210+
// when command substitution parsing is enabled.
211+
if !dollarQuote {
212+
return nil, errors.New("invalid command line string")
213+
}
214+
215+
out, err := shellRun(backtick, p.Dir)
216+
if err != nil {
217+
return nil, err
218+
}
219+
220+
// Defensive guard: valid $(...) implies the buffer must contain
221+
// the "$(" prefix plus the collected command body.
222+
if len(buf) < len(backtick)+2 {
223+
return nil, errors.New("invalid command line string")
212224
}
225+
226+
buf = buf[:len(buf)-len(backtick)-2] + out
213227
backtick = ""
214-
dollarQuote = !dollarQuote
228+
dollarQuote = false
215229
continue
216230
}
217-
backtick = ""
218-
dollarQuote = !dollarQuote
231+
232+
// Backtick parsing disabled:
233+
// preserve literal text for $(...) constructs instead of
234+
// silently dropping the closing ')'.
235+
if dollarQuote {
236+
buf += string(r)
237+
backtick = ""
238+
dollarQuote = false
239+
got = argSingle
240+
continue
241+
}
242+
243+
buf += string(r)
244+
got = argSingle
245+
continue
219246
}
247+
220248
case '(':
221249
if !singleQuoted && !doubleQuoted && !backQuote {
222250
if !dollarQuote && strings.HasSuffix(buf, "$") {
@@ -227,6 +255,7 @@ loop:
227255
return nil, errors.New("invalid command line string")
228256
}
229257
}
258+
230259
case '"':
231260
if !singleQuoted && !dollarQuote {
232261
if doubleQuoted {
@@ -235,6 +264,7 @@ loop:
235264
doubleQuoted = !doubleQuoted
236265
continue
237266
}
267+
238268
case '\'':
239269
if !doubleQuoted && !dollarQuote {
240270
if singleQuoted {
@@ -243,6 +273,7 @@ loop:
243273
singleQuoted = !singleQuoted
244274
continue
245275
}
276+
246277
case ';', '&', '|', '<', '>':
247278
if !(escaped || singleQuoted || doubleQuoted || backQuote || dollarQuote) {
248279
if r == '>' && len(buf) > 0 {
@@ -320,4 +351,4 @@ func Parse(line string) ([]string, error) {
320351

321352
func ParseWithEnvs(line string) (envs []string, args []string, err error) {
322353
return NewParser().ParseWithEnvs(line)
323-
}
354+
}

‎shellwords_security_test.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
package shellwords
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
// ParseBacktick=true → an unmatched ‘)’ should be treated as an ERROR (not a literal)
9+
func TestUnmatchedClosingParen_ReturnsError_ParseBacktickEnabled(t *testing.T) {
10+
p := NewParser()
11+
p.ParseBacktick = true
12+
13+
_, err := p.Parse("))")
14+
if err == nil {
15+
t.Fatal("expected error for unmatched ')', got nil")
16+
}
17+
}
18+
19+
// ParseBacktick=true → Do not execute; must fail
20+
func TestBareClosingParen_NoExecution_ReturnsError(t *testing.T) {
21+
p := NewParser()
22+
p.ParseBacktick = true
23+
24+
_, err := p.Parse("prefix)echo PWNED)")
25+
if err == nil {
26+
t.Fatal("expected error for unmatched ')', got nil")
27+
}
28+
}
29+
30+
// Default behavior → ‘)’ is a literal; it does not break parsing
31+
func TestBareClosingParen_DefaultParsingLiteral(t *testing.T) {
32+
out, err := Parse(")a b)c d")
33+
if err != nil {
34+
t.Fatalf("unexpected error: %v", err)
35+
}
36+
37+
want := []string{")a", "b)c", "d"}
38+
if len(out) != len(want) {
39+
t.Fatalf("unexpected token count: got %d, want %d, out=%#v", len(out), len(want), out)
40+
}
41+
for i := range want {
42+
if out[i] != want[i] {
43+
t.Fatalf("unexpected token at %d: got %q, want %q, out=%#v", i, out[i], want[i], out)
44+
}
45+
}
46+
}
47+
48+
// ParseBacktick=false → $(...) remains a FULL literal
49+
func TestDollarParenLiteralWhenParseBacktickDisabled(t *testing.T) {
50+
p := NewParser()
51+
p.ParseBacktick = false
52+
53+
out, err := p.Parse(`$(echo "foo")`)
54+
if err != nil {
55+
t.Fatalf("unexpected error: %v", err)
56+
}
57+
58+
want := []string{`$(echo "foo")`}
59+
if len(out) != len(want) {
60+
t.Fatalf("unexpected token count: got %d, want %d, out=%#v", len(out), len(want), out)
61+
}
62+
if out[0] != want[0] {
63+
t.Fatalf("unexpected output: got %q, want %q", out[0], want[0])
64+
}
65+
}
66+
67+
// If true → $(...) continues to work with ParseBacktick=true
68+
func TestValidDollarCommandSubstitutionStillWorks(t *testing.T) {
69+
p := NewParser()
70+
p.ParseBacktick = true
71+
72+
out, err := p.Parse("prefix $(printf hello) suffix")
73+
if err != nil {
74+
t.Fatalf("unexpected error: %v", err)
75+
}
76+
77+
got := strings.Join(out, " ")
78+
if !strings.Contains(got, "hello") {
79+
t.Fatalf("expected valid command substitution to work, got: %q", got)
80+
}
81+
}
82+
83+
// Error → Unclosed $(
84+
func TestUnclosedDollarCommandSubstitutionReturnsError(t *testing.T) {
85+
p := NewParser()
86+
p.ParseBacktick = true
87+
88+
_, err := p.Parse("prefix $(echo hello")
89+
if err == nil {
90+
t.Fatal("expected error for unclosed command substitution, got nil")
91+
}
92+
}

0 commit comments

Comments
 (0)