Skip to content

Commit

Permalink
Validate queries by checking unsafe builtins
Browse files Browse the repository at this point in the history
Fixes #919

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
  • Loading branch information
ashutosh-narkar authored and tsandall committed Sep 4, 2018
1 parent 7be7fa7 commit 2e03819
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 0 deletions.
46 changes: 46 additions & 0 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ const (

var systemMainPath = ast.MustParseRef("data.system.main")

// map of unsafe buitins
var unsafeBuiltinsMap = map[string]bool{ast.HTTPSend.Name: true}

// Server represents an instance of OPA running in server mode.
type Server struct {
Handler http.Handler
Expand Down Expand Up @@ -619,6 +622,15 @@ func (s *Server) v1CompilePost(w http.ResponseWriter, r *http.Request) {
return
}

unsafeBuiltins, err := validateParsedQuery(request.Query)
if err != nil {
writer.ErrorAuto(w, err)
return
} else if len(unsafeBuiltins) > 0 {
writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "unsafe built-in function calls in query: %v", strings.Join(unsafeBuiltins, ",")))
return
}

m.Timer(metrics.RegoQueryParse).Stop()

txn, err := s.store.NewTransaction(ctx)
Expand Down Expand Up @@ -1247,6 +1259,15 @@ func (s *Server) v1QueryGet(w http.ResponseWriter, r *http.Request) {
}
qStr := qStrs[len(qStrs)-1]

unsafeBuiltins, err := validateQuery(qStr)
if err != nil {
writer.ErrorAuto(w, err)
return
} else if len(unsafeBuiltins) > 0 {
writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "unsafe built-in function calls in query: %v", strings.Join(unsafeBuiltins, ",")))
return
}

watch := getWatch(r.URL.Query()[types.ParamWatchV1])
if watch {
s.watchQuery(qStr, w, r, false)
Expand Down Expand Up @@ -1565,6 +1586,31 @@ func stringPathToRef(s string) (r ast.Ref) {
return r
}

func validateQuery(query string) ([]string, error) {

var body ast.Body
body, err := ast.ParseBody(query)
if err != nil {
return []string{}, err
}

return validateParsedQuery(body)
}

func validateParsedQuery(body ast.Body) ([]string, error) {
unsafeOperators := []string{}
ast.WalkExprs(body, func(x *ast.Expr) bool {
if x.IsCall() {
operator := x.Operator().String()
if _, ok := unsafeBuiltinsMap[operator]; ok {
unsafeOperators = append(unsafeOperators, operator)
}
}
return false
})
return unsafeOperators, nil
}

func getBoolParam(url *url.URL, name string, ifEmpty bool) bool {

p, ok := url.Query()[name]
Expand Down
38 changes: 38 additions & 0 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,25 @@ func TestCompileV1Observability(t *testing.T) {
}
}

func TestCompileV1UnsafeBuiltin(t *testing.T) {
f := newFixture(t)
get := newReqV1(http.MethodPost, `/compile`, `{"query": "http.send({\"method\": \"get\", \"url\": \"foo.com\"}, x)"}`)
f.server.Handler.ServeHTTP(f.recorder, get)

if f.recorder.Code != 400 {
t.Fatalf("Expected bad request but got %v", f.recorder)
}

expected := `{
"code": "invalid_parameter",
"message": "unsafe built-in function calls in query: http.send"
}`

if f.recorder.Body.String() != expected {
t.Fatalf(`Expected %v but got: %v`, expected, f.recorder.Body.String())
}
}

func TestDataV1(t *testing.T) {
testMod1 := `package testmod
Expand Down Expand Up @@ -2184,6 +2203,25 @@ func TestQueryV1(t *testing.T) {
}
}

func TestQueryV1UnsafeBuiltin(t *testing.T) {
f := newFixture(t)
get := newReqV1(http.MethodGet, `/query?q=http.send({"method": "get", "url": "foo.com"}, x)`, "")
f.server.Handler.ServeHTTP(f.recorder, get)

if f.recorder.Code != 400 {
t.Fatalf("Expected bad request but got %v", f.recorder)
}

expected := `{
"code": "invalid_parameter",
"message": "unsafe built-in function calls in query: http.send"
}`

if f.recorder.Body.String() != expected {
t.Fatalf(`Expected %v but got: %v`, expected, f.recorder.Body.String())
}
}

func TestUnversionedPost(t *testing.T) {

f := newFixture(t)
Expand Down

0 comments on commit 2e03819

Please sign in to comment.