Skip to content

Commit

Permalink
Asserting every domain is an collection type before evaluation (#6763)
Browse files Browse the repository at this point in the history
Fixing an issue where a non-collection `every`-domain didn’t fail evaluation.
Removing a possible attack surface, where an attacker with the ability to craft portions of the input document could replace a value with an expected collection type, that is known to be processed by an `every`-statement, with a non-collection value and thereby would cause the policy to accept a query that should otherwise be rejected.

Fixes: #6762
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
  • Loading branch information
johanfylling authored May 28, 2024
1 parent 27da341 commit 62834a2
Show file tree
Hide file tree
Showing 6 changed files with 329 additions and 21 deletions.
11 changes: 11 additions & 0 deletions internal/compiler/wasm/wasm.go
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,17 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err
instrs = append(instrs, instruction.Br{Index: 0})
break
}
case *ir.IsSetStmt:
if loc, ok := stmt.Source.Value.(ir.Local); ok {
instrs = append(instrs, instruction.GetLocal{Index: c.local(loc)})
instrs = append(instrs, instruction.Call{Index: c.function(opaValueType)})
instrs = append(instrs, instruction.I32Const{Value: opaTypeSet})
instrs = append(instrs, instruction.I32Ne{})
instrs = append(instrs, instruction.BrIf{Index: 0})
} else {
instrs = append(instrs, instruction.Br{Index: 0})
break
}
case *ir.IsUndefinedStmt:
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Source)})
instrs = append(instrs, instruction.I32Const{Value: 0})
Expand Down
34 changes: 34 additions & 0 deletions internal/planner/planner.go
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,40 @@ func (p *Planner) planExprEvery(e *ast.Expr, iter planiter) error {
})

err := p.planTerm(every.Domain, func() error {
// Assert that the domain is a collection type:
// block outer
// block a
// isArray
// br 1: break outer, and continue
// block b
// isObject
// br 1: break outer, and continue
// block c
// isSet
// br 1: break outer, and continue
// br 1: invalid domain, break every

aBlock := &ir.Block{}
p.appendStmtToBlock(&ir.IsArrayStmt{Source: p.ltarget}, aBlock)
p.appendStmtToBlock(&ir.BreakStmt{Index: 1}, aBlock)

bBlock := &ir.Block{}
p.appendStmtToBlock(&ir.IsObjectStmt{Source: p.ltarget}, bBlock)
p.appendStmtToBlock(&ir.BreakStmt{Index: 1}, bBlock)

cBlock := &ir.Block{}
p.appendStmtToBlock(&ir.IsSetStmt{Source: p.ltarget}, cBlock)
p.appendStmtToBlock(&ir.BreakStmt{Index: 1}, cBlock)

outerBlock := &ir.BlockStmt{Blocks: []*ir.Block{
{
Stmts: []ir.Stmt{
&ir.BlockStmt{Blocks: []*ir.Block{aBlock, bBlock, cBlock}},
&ir.BreakStmt{Index: 1}},
},
}}
p.appendStmt(outerBlock)

return p.planScan(every.Key, func(ir.Local) error {
p.appendStmt(&ir.ResetLocalStmt{
Target: cond1,
Expand Down
7 changes: 7 additions & 0 deletions ir/ir.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,13 @@ type IsObjectStmt struct {
Location
}

// IsSetStmt represents a dynamic type check on a local variable.
type IsSetStmt struct {
Source Operand `json:"source"`

Location
}

// IsDefinedStmt represents a check of whether a local variable is defined.
type IsDefinedStmt struct {
Source Local `json:"source"`
Expand Down
93 changes: 91 additions & 2 deletions test/cases/testdata/every/every.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,50 @@ cases:
p {
every x in [] { x != x }
}
note: every/empty domain
note: every/empty domain (array)
query: data.test.p = x
want_result:
- x: true
- data:
modules:
- |
package test
import future.keywords.every
p {
every x in set() { x != x }
}
note: every/empty domain (set)
query: data.test.p = x
want_result:
- x: true
- data:
modules:
- |
package test
import future.keywords.every
p {
every x in {} { x != x }
}
note: every/empty domain (object)
query: data.test.p = x
want_result:
- x: true
- data:
modules:
- |
package test
import future.keywords.every
l[1] {
false
}
p {
every x in l { x != x }
}
note: every/empty domain (partial rule ref)
query: data.test.p = x
want_result:
- x: true
Expand All @@ -22,7 +65,19 @@ cases:
p {
every _ in input { true }
}
note: every/domain undefined
note: every/domain undefined (input)
query: data.test.p = x
want_result: []
- data:
modules:
- |
package test
import future.keywords.every
p {
every _ in data.foo { true }
}
note: every/domain undefined (data ref)
query: data.test.p = x
want_result: []
- data:
Expand Down Expand Up @@ -57,6 +112,40 @@ cases:
package test
import future.keywords.every
p {
every k, v in {1, 2} { k == v }
}
note: every/simple key/val (set)
query: data.test.p = x
want_result:
- x: true
- data:
modules:
- |
package test
import future.keywords.every
l[1] {
true
}
l[2] {
true
}
p {
every k, v in l { k == v }
}
note: every/simple key/val (partial rule ref)
query: data.test.p = x
want_result:
- x: true
- data:
modules:
- |
package test
import future.keywords.every
p {
i := 10
every k, v in [1, 2] { k+v != i }
Expand Down
151 changes: 151 additions & 0 deletions test/cases/testdata/every/non_iterable_domain.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
---
cases:
- note: "every/non-iter domain: int"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in 42 { v > 1 }
}
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: string"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in "foobar" { v > 1 }
}
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: bool"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in true { v > 1 }
}
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: null"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in null { v > 1 }
}
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: built-in call"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in floor(13.37) { v > 1 }
}
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: function call"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in foo(1, 2) { v > 1 }
}
foo(a, b) := a + b
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: rule ref"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in q { v > 1 }
}
q := 1
query: data.test.p = x
want_result:
- x: 1
- note: "every/non-iter domain: data int"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in data.iterate_me { v > 1 }
}
query: data.test.p = x
data:
iterate_me: 1
want_result:
- x: 1
- note: "every/non-iter domain: input int"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in input.iterate_me { v > 1 }
}
query: data.test.p = x
input:
iterate_me: 1
want_result:
- x: 1
- note: "every/non-iter domain: input int (1st level)"
modules:
- |
package test
import future.keywords.every
default p := 1
p := 2 {
every v in input { v > 1 }
}
query: data.test.p = x
input: 1
want_result:
- x: 1
Loading

0 comments on commit 62834a2

Please sign in to comment.