Skip to content

Commit

Permalink
implement detecting untrusted inputs in contexts
Browse files Browse the repository at this point in the history
  • Loading branch information
rhysd committed Aug 5, 2021
1 parent 8c39ac5 commit d320852
Show file tree
Hide file tree
Showing 3 changed files with 200 additions and 14 deletions.
161 changes: 161 additions & 0 deletions expr_insecure.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
package actionlint

import (
"strings"
)

type untrustedInputMap map[string]untrustedInputMap

func (m untrustedInputMap) findChild(name string) (untrustedInputMap, bool) {
if m == nil {
return nil, false
}
if c, ok := m[name]; ok {
return c, true
}
if c, ok := m["*"]; ok {
return c, true
}
return nil, false
}

// BuiltinUntrustedInputs is list of untrusted inputs. These inputs are detected as untrusted in
// `run:` scripts. See the URL for more details.
// https://securitylab.github.com/research/github-actions-untrusted-input/
var BuiltinUntrustedInputs = untrustedInputMap{
"github": {
"event": {
"issue": {
"title": nil,
"body": nil,
},
"pull_request": {
"title": nil,
"body": nil,
"head": {
"ref": nil,
"label": nil,
"repo": {
"default_branch": nil,
},
},
},
"comment": {
"body": nil,
},
"review": {
"body": nil,
},
"pages": {
"*": {
"page_name": nil,
},
},
"commits": {
"*": {
"message": nil,
"author": {
"email": nil,
"name": nil,
},
},
},
"head_commit": {
"message": nil,
"author": {
"email": nil,
"name": nil,
},
},
},
"head_ref": nil,
},
}

// UntrustedInputChecker is a checker to detect untrusted inputs in an expression syntax tree.
type UntrustedInputChecker struct {
root untrustedInputMap
cur untrustedInputMap
varNode *VariableNode
path []string
errs []*ExprError
}

// NewUntrustedInputChecker creates new UntrustedInputChecker instance.
func NewUntrustedInputChecker(m untrustedInputMap) *UntrustedInputChecker {
return &UntrustedInputChecker{
root: m,
cur: nil,
varNode: nil,
path: nil,
}
}

func (u *UntrustedInputChecker) found() {
err := errorfAtExpr(u.varNode, "%q is possibly untrusted. please avoid using it directly in script by passing it through environment variable. see https://securitylab.github.com/research/github-actions-untrusted-input for more details.", strings.Join(u.path, "."))
u.errs = append(u.errs, err)
u.done()
}

func (u *UntrustedInputChecker) done() {
if u.cur == nil {
return
}
u.cur = nil
u.varNode = nil
u.path = u.path[:0]
}

func (u *UntrustedInputChecker) checkVar(name string) {
m, ok := u.root[name]
if !ok {
u.done()
return
}

u.path = append(u.path, name)

if m == nil {
u.found()
return
}

u.cur = m
}

func (u *UntrustedInputChecker) checkProp(name string) {
c, ok := u.cur.findChild(name)
if !ok {
u.done()
return
}

u.path = append(u.path, name)

if c == nil {
u.found()
return
}

u.cur = c
}

// OnNode is a callback which should be called on visiting node. This method assumes to be called
// in depth-first, bottom-up order.
func (u *UntrustedInputChecker) OnNode(n ExprNode) {
switch n := n.(type) {
case *VariableNode:
u.varNode = n
u.checkVar(n.Name)
case *ObjectDerefNode:
u.checkProp(n.Property)
default:
u.done()
}
}

// Errs returns errors detected by this checker. This method should be called after visiting all
// nodes in a syntax tree.
func (u *UntrustedInputChecker) Errs() []*ExprError {
return u.errs
}
26 changes: 22 additions & 4 deletions expr_sema.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,15 +284,21 @@ type ExprSemanticsChecker struct {
vars map[string]ExprType
errs []*ExprError
varsCopied bool
untrusted *UntrustedInputChecker
}

// NewExprSemanticsChecker creates new ExprSemanticsChecker instance.
func NewExprSemanticsChecker() *ExprSemanticsChecker {
return &ExprSemanticsChecker{
// NewExprSemanticsChecker creates new ExprSemanticsChecker instance. When checkUntrustedInput is
// set to true, the checker will make use of possibly untrusted inputs error.
func NewExprSemanticsChecker(checkUntrustedInput bool) *ExprSemanticsChecker {
c := &ExprSemanticsChecker{
funcs: BuiltinFuncSignatures,
vars: BuiltinGlobalVariableTypes,
varsCopied: false,
}
if checkUntrustedInput {
c.untrusted = NewUntrustedInputChecker(BuiltinUntrustedInputs)
}
return c
}

func errorAtExpr(e ExprNode, msg string) *ExprError {
Expand Down Expand Up @@ -346,6 +352,12 @@ func (sema *ExprSemanticsChecker) UpdateNeeds(ty *ObjectType) {
sema.vars["needs"] = ty
}

func (sema *ExprSemanticsChecker) checkUntrustedInput(n ExprNode) {
if sema.untrusted != nil {
sema.untrusted.OnNode(n)
}
}

func (sema *ExprSemanticsChecker) checkVariable(n *VariableNode) ExprType {
v, ok := sema.vars[n.Name]
if !ok {
Expand Down Expand Up @@ -606,6 +618,8 @@ func (sema *ExprSemanticsChecker) checkLogicalOp(n *LogicalOpNode) ExprType {
}

func (sema *ExprSemanticsChecker) check(expr ExprNode) ExprType {
defer sema.checkUntrustedInput(expr) // Call this method in bottom-up order

switch e := expr.(type) {
case *VariableNode:
return sema.checkVariable(e)
Expand Down Expand Up @@ -642,5 +656,9 @@ func (sema *ExprSemanticsChecker) check(expr ExprNode) ExprType {
func (sema *ExprSemanticsChecker) Check(expr ExprNode) (ExprType, []*ExprError) {
sema.errs = []*ExprError{}
ty := sema.check(expr)
return ty, sema.errs
errs := sema.errs
if sema.untrusted != nil {
errs = append(errs, sema.untrusted.Errs()...)
}
return ty, errs
}
27 changes: 17 additions & 10 deletions rule_expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (rule *RuleExpression) VisitStep(n *Step) error {
var spec *String
switch e := n.Exec.(type) {
case *ExecRun:
rule.checkString(e.Run)
rule.checkScriptString(e.Run)
rule.checkString(e.Shell)
rule.checkString(e.WorkingDirectory)
case *ExecAction:
Expand Down Expand Up @@ -422,7 +422,7 @@ func (rule *RuleExpression) checkIfCondition(str *String) {
return
}

condTy = rule.checkSemanticsOfExprNode(expr, line, col)
condTy = rule.checkSemanticsOfExprNode(expr, line, col, false)
}

if condTy != nil && !(BoolType{}).Assignable(condTy) {
Expand All @@ -434,7 +434,14 @@ func (rule *RuleExpression) checkString(str *String) []ExprType {
if str == nil {
return nil
}
return rule.checkExprsIn(str.Value, str.Pos, str.Quoted)
return rule.checkExprsIn(str.Value, str.Pos, str.Quoted, false)
}

func (rule *RuleExpression) checkScriptString(str *String) []ExprType {
if str == nil {
return nil
}
return rule.checkExprsIn(str.Value, str.Pos, str.Quoted, true)
}

func (rule *RuleExpression) checkBool(b *Bool) {
Expand Down Expand Up @@ -464,7 +471,7 @@ func (rule *RuleExpression) checkFloat(f *Float) {
rule.checkNumberExpression(f.Expression, "float number value")
}

func (rule *RuleExpression) checkExprsIn(s string, pos *Pos, quoted bool) []ExprType {
func (rule *RuleExpression) checkExprsIn(s string, pos *Pos, quoted bool, checkUntrusted bool) []ExprType {
// TODO: Line number is not correct when the string contains newlines.

line, col := pos.Line, pos.Col
Expand All @@ -483,7 +490,7 @@ func (rule *RuleExpression) checkExprsIn(s string, pos *Pos, quoted bool) []Expr
s = s[start:]
offset += start

ty, offsetAfter := rule.checkSemantics(s, line, col+offset)
ty, offsetAfter := rule.checkSemantics(s, line, col+offset, checkUntrusted)
if ty == nil || offsetAfter == 0 {
return nil
}
Expand All @@ -507,7 +514,7 @@ func (rule *RuleExpression) checkRawYAMLValue(v RawYAMLValue) {
rule.checkRawYAMLValue(v)
}
case *RawYAMLString:
rule.checkExprsIn(v.Value, v.Pos(), false)
rule.checkExprsIn(v.Value, v.Pos(), false, false)
default:
panic("unreachable")
}
Expand All @@ -518,8 +525,8 @@ func (rule *RuleExpression) exprError(err *ExprError, lineBase, colBase int) {
rule.error(pos, err.Message)
}

func (rule *RuleExpression) checkSemanticsOfExprNode(expr ExprNode, line, col int) ExprType {
c := NewExprSemanticsChecker()
func (rule *RuleExpression) checkSemanticsOfExprNode(expr ExprNode, line, col int, checkUntrusted bool) ExprType {
c := NewExprSemanticsChecker(checkUntrusted)
if rule.matrixTy != nil {
c.UpdateMatrix(rule.matrixTy)
}
Expand All @@ -538,15 +545,15 @@ func (rule *RuleExpression) checkSemanticsOfExprNode(expr ExprNode, line, col in
return ty
}

func (rule *RuleExpression) checkSemantics(src string, line, col int) (ExprType, int) {
func (rule *RuleExpression) checkSemantics(src string, line, col int, checkUntrusted bool) (ExprType, int) {
l := NewExprLexer(src)
p := NewExprParser()
expr, err := p.Parse(l)
if err != nil {
rule.exprError(err, line, col)
return nil, l.Offset()
}
return rule.checkSemanticsOfExprNode(expr, line, col), l.Offset()
return rule.checkSemanticsOfExprNode(expr, line, col, checkUntrusted), l.Offset()
}

func (rule *RuleExpression) calcNeedsType(job *Job) *ObjectType {
Expand Down

0 comments on commit d320852

Please sign in to comment.