Skip to content

Commit ad20632

Browse files
johanfyllingjohanfylling
authored
Merge commit from fork
to disable code injection attacks, where Rego code can be injected into the constructed evaluation query. See Security Advisory: GHSA-6m8w-jc87-6cr7 Fixes: #GHSA-6m8w-jc87-6cr7
1 parent 24ff9cf commit ad20632

File tree

3 files changed

+226
-17
lines changed

3 files changed

+226
-17
lines changed

‎v1/server/server.go‎

Lines changed: 77 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,19 +1148,23 @@ func (s *Server) v0QueryPath(w http.ResponseWriter, r *http.Request, urlPath str
11481148
}
11491149

11501150
if len(rs) == 0 {
1151-
ref := stringPathToDataRef(urlPath)
1151+
ref, err := stringPathToDataRef(urlPath)
1152+
if err != nil {
1153+
writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "invalid path: %v", err))
1154+
return
1155+
}
11521156

11531157
var messageType = types.MsgMissingError
11541158
if len(s.getCompiler().GetRulesForVirtualDocument(ref)) > 0 {
11551159
messageType = types.MsgFoundUndefinedError
11561160
}
1157-
err := types.NewErrorV1(types.CodeUndefinedDocument, "%v: %v", messageType, ref)
1158-
if err := logger.Log(ctx, txn, urlPath, "", goInput, input, nil, ndbCache, err, m); err != nil {
1161+
errV1 := types.NewErrorV1(types.CodeUndefinedDocument, "%v: %v", messageType, ref)
1162+
if err := logger.Log(ctx, txn, urlPath, "", goInput, input, nil, ndbCache, errV1, m); err != nil {
11591163
writer.ErrorAuto(w, err)
11601164
return
11611165
}
11621166

1163-
writer.Error(w, http.StatusNotFound, err)
1167+
writer.Error(w, http.StatusNotFound, errV1)
11641168
return
11651169
}
11661170
err = logger.Log(ctx, txn, urlPath, "", goInput, input, &rs[0].Expressions[0].Value, ndbCache, nil, m)
@@ -1319,10 +1323,15 @@ func (s *Server) unversionedGetHealthWithPolicy(w http.ResponseWriter, r *http.R
13191323
vars := mux.Vars(r)
13201324
urlPath := vars["path"]
13211325
healthDataPath := "/system/health/" + urlPath
1322-
healthDataPath = stringPathToDataRef(healthDataPath).String()
1326+
1327+
healthDataPathQuery, err := stringPathToQuery(healthDataPath)
1328+
if err != nil {
1329+
writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "invalid path: %v", err))
1330+
return
1331+
}
13231332

13241333
rego := rego.New(
1325-
rego.Query(healthDataPath),
1334+
rego.ParsedQuery(healthDataPathQuery),
13261335
rego.Compiler(s.getCompiler()),
13271336
rego.Store(s.store),
13281337
rego.Input(input),
@@ -1337,7 +1346,7 @@ func (s *Server) unversionedGetHealthWithPolicy(w http.ResponseWriter, r *http.R
13371346
}
13381347

13391348
if len(rs) == 0 {
1340-
writeHealthResponse(w, fmt.Errorf("health check (%v) was undefined", healthDataPath))
1349+
writeHealthResponse(w, fmt.Errorf("health check (%v) was undefined", healthDataPathQuery))
13411350
return
13421351
}
13431352

@@ -1347,7 +1356,7 @@ func (s *Server) unversionedGetHealthWithPolicy(w http.ResponseWriter, r *http.R
13471356
return
13481357
}
13491358

1350-
writeHealthResponse(w, fmt.Errorf("health check (%v) returned unexpected value", healthDataPath))
1359+
writeHealthResponse(w, fmt.Errorf("health check (%v) returned unexpected value", healthDataPathQuery))
13511360
}
13521361

13531362
func writeHealthResponse(w http.ResponseWriter, err error) {
@@ -2549,12 +2558,15 @@ func (s *Server) makeRego(_ context.Context,
25492558
tracer topdown.QueryTracer,
25502559
opts []func(*rego.Rego),
25512560
) (*rego.Rego, error) {
2552-
queryPath := stringPathToDataRef(urlPath).String()
2561+
query, err := stringPathToQuery(urlPath)
2562+
if err != nil {
2563+
return nil, types.NewErrorV1(types.CodeInvalidParameter, "invalid path: %v", err)
2564+
}
25532565

25542566
opts = append(
25552567
opts,
25562568
rego.Transaction(txn),
2557-
rego.Query(queryPath),
2569+
rego.ParsedQuery(query),
25582570
rego.ParsedInput(input),
25592571
rego.Metrics(m),
25602572
rego.QueryTracer(tracer),
@@ -2569,6 +2581,43 @@ func (s *Server) makeRego(_ context.Context,
25692581
return rego.New(opts...), nil
25702582
}
25712583

2584+
func stringPathToQuery(urlPath string) (ast.Body, error) {
2585+
ref, err := stringPathToDataRef(urlPath)
2586+
if err != nil {
2587+
return nil, err
2588+
}
2589+
2590+
return parseRefQuery(ref.String())
2591+
}
2592+
2593+
// parseRefQuery parses a string into a query ast.Body.
2594+
// The resulting query must be comprised of a single ref, or an error will be returned.
2595+
func parseRefQuery(str string) (ast.Body, error) {
2596+
query, err := ast.ParseBody(str)
2597+
if err != nil {
2598+
return nil, errors.New("failed to parse query")
2599+
}
2600+
2601+
// assert the query is exactly one statement
2602+
if l := len(query); l == 0 {
2603+
return nil, errors.New("no ref")
2604+
} else if l > 1 {
2605+
return nil, errors.New("complex query")
2606+
}
2607+
2608+
// assert the single statement is a lone ref
2609+
expr := query[0]
2610+
switch t := expr.Terms.(type) {
2611+
case *ast.Term:
2612+
switch t.Value.(type) {
2613+
case ast.Ref:
2614+
return query, nil
2615+
}
2616+
}
2617+
2618+
return nil, errors.New("complex query")
2619+
}
2620+
25722621
func (*Server) prepareV1PatchSlice(root string, ops []types.PatchV1) (result []patchImpl, err error) {
25732622

25742623
root = "/" + strings.Trim(root, "/")
@@ -2677,31 +2726,44 @@ func (s *Server) updateNDCache(enabled bool) {
26772726
s.ndbCacheEnabled = enabled
26782727
}
26792728

2680-
func stringPathToDataRef(s string) (r ast.Ref) {
2729+
func stringPathToDataRef(s string) (ast.Ref, error) {
26812730
result := ast.Ref{ast.DefaultRootDocument}
2682-
return append(result, stringPathToRef(s)...)
2731+
r, err := stringPathToRef(s)
2732+
if err != nil {
2733+
return nil, err
2734+
}
2735+
return append(result, r...), nil
26832736
}
26842737

2685-
func stringPathToRef(s string) (r ast.Ref) {
2738+
func stringPathToRef(s string) (ast.Ref, error) {
2739+
r := ast.Ref{}
2740+
26862741
if len(s) == 0 {
2687-
return r
2742+
return r, nil
26882743
}
2744+
26892745
p := strings.Split(s, "/")
26902746
for _, x := range p {
26912747
if x == "" {
26922748
continue
26932749
}
2750+
26942751
if y, err := url.PathUnescape(x); err == nil {
26952752
x = y
26962753
}
2754+
2755+
if strings.Contains(x, "\"") {
2756+
return nil, fmt.Errorf("invalid ref term '%s'", x)
2757+
}
2758+
26972759
i, err := strconv.Atoi(x)
26982760
if err != nil {
26992761
r = append(r, ast.StringTerm(x))
27002762
} else {
27012763
r = append(r, ast.IntNumberTerm(i))
27022764
}
27032765
}
2704-
return r
2766+
return r, nil
27052767
}
27062768

27072769
func validateQuery(query string, opts ast.ParserOptions) (ast.Body, error) {

‎v1/server/server_test.go‎

Lines changed: 149 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3383,7 +3383,6 @@ func TestDataMetricsEval(t *testing.T) {
33833383
"counter_disk_read_keys",
33843384
"counter_disk_read_bytes",
33853385
"timer_rego_input_parse_ns",
3386-
"timer_rego_query_parse_ns",
33873386
"timer_rego_query_compile_ns",
33883387
"timer_rego_query_eval_ns",
33893388
"timer_server_handler_ns",
@@ -6811,3 +6810,152 @@ func zipString(input string) []byte {
68116810
}
68126811
return b.Bytes()
68136812
}
6813+
6814+
func TestStringPathToDataRef(t *testing.T) {
6815+
t.Parallel()
6816+
6817+
cases := []struct {
6818+
note string
6819+
path string
6820+
expRef string
6821+
expErr string
6822+
}{
6823+
{path: "foo", expRef: `data.foo`},
6824+
{path: "foo/", expRef: `data.foo`},
6825+
{path: "foo/bar", expRef: `data.foo.bar`},
6826+
{path: "foo/bar/", expRef: `data.foo.bar`},
6827+
{path: "foo/../bar", expRef: `data.foo[".."].bar`},
6828+
6829+
// Path injection attack
6830+
// url path: `foo%22%5D%3Bmalicious_call%28%29%3Bx%3D%5B%22`
6831+
// url decoded: `foo"];malicious_call();x=["`
6832+
// data ref .String(): `data.foo["\"];malicious_call();x=[\""]`
6833+
// Above attack is mitigated by rejecting any ref component containing string terminators (`"`).
6834+
{
6835+
note: "string terminals inside ref term",
6836+
path: "foo%22%5D%3Bmalicious_call%28%29%3Bx%3D%5B%22", // foo"];malicious_call();x=["
6837+
expErr: `invalid ref term 'foo"];malicious_call();x=["'`,
6838+
},
6839+
}
6840+
6841+
for _, tc := range cases {
6842+
note := tc.note
6843+
if note == "" {
6844+
note = strings.ReplaceAll(tc.path, "/", "_")
6845+
}
6846+
6847+
t.Run(note, func(t *testing.T) {
6848+
ref, err := stringPathToDataRef(tc.path)
6849+
6850+
if tc.expRef != "" {
6851+
if err != nil {
6852+
t.Fatalf("Expected ref:\n\n%s\n\nbut got error:\n\n%s", tc.expRef, err)
6853+
}
6854+
if refStr := ref.String(); refStr != tc.expRef {
6855+
t.Fatalf("Expected ref:\n\n%s\n\nbut got:\n\n%s", tc.expRef, refStr)
6856+
}
6857+
}
6858+
6859+
if tc.expErr != "" {
6860+
if ref != nil {
6861+
t.Fatalf("Expected error:\n\n%s\n\nbut got ref:\n\n%s", tc.expErr, ref.String())
6862+
}
6863+
if errStr := err.Error(); errStr != tc.expErr {
6864+
t.Fatalf("Expected error:\n\n%s\n\nbut got ref:\n\n%s", tc.expErr, errStr)
6865+
}
6866+
}
6867+
})
6868+
}
6869+
}
6870+
6871+
func TestParseRefQuery(t *testing.T) {
6872+
t.Parallel()
6873+
6874+
cases := []struct {
6875+
note string
6876+
raw string
6877+
expBody ast.Body
6878+
expErr string
6879+
}{
6880+
{
6881+
note: "unparseable",
6882+
raw: `}abc{`,
6883+
expErr: "failed to parse query",
6884+
},
6885+
{
6886+
note: "empty",
6887+
raw: ``,
6888+
expErr: "no ref",
6889+
},
6890+
{
6891+
note: "single ref",
6892+
raw: `data.foo.bar`,
6893+
expBody: ast.MustParseBody(`data.foo.bar`),
6894+
},
6895+
{
6896+
note: "multiple refs,';' separated",
6897+
raw: `data.foo.bar;data.baz.qux`,
6898+
expErr: "complex query",
6899+
},
6900+
{
6901+
note: "multiple refs,newline separated",
6902+
raw: `data.foo.bar
6903+
data.baz.qux`,
6904+
expErr: "complex query",
6905+
},
6906+
{
6907+
note: "single ref + call",
6908+
raw: `data.foo.bar;data.baz.qux()`,
6909+
expErr: "complex query",
6910+
},
6911+
{
6912+
note: "single ref + assignment",
6913+
raw: `data.foo.bar;x := 42`,
6914+
expErr: "complex query",
6915+
},
6916+
{
6917+
note: "single call",
6918+
raw: `data.foo.bar()`,
6919+
expErr: "complex query",
6920+
},
6921+
{
6922+
note: "single assignment",
6923+
raw: `x := 42`,
6924+
expErr: "complex query",
6925+
},
6926+
{
6927+
note: "single unification",
6928+
raw: `x = 42`,
6929+
expErr: "complex query",
6930+
},
6931+
{
6932+
note: "single equality",
6933+
raw: `x == 42`,
6934+
expErr: "complex query",
6935+
},
6936+
}
6937+
6938+
for _, tc := range cases {
6939+
t.Run(tc.note, func(t *testing.T) {
6940+
body, err := parseRefQuery(tc.raw)
6941+
6942+
if tc.expBody != nil {
6943+
if err != nil {
6944+
t.Fatalf("Expected body:\n\n%s\n\nbut got error:\n\n%s", tc.expBody, err)
6945+
}
6946+
if body.String() != tc.expBody.String() {
6947+
t.Fatalf("Expected body:\n\n%s\n\nbut got:\n\n%s", tc.expBody, body.String())
6948+
}
6949+
}
6950+
6951+
if tc.expErr != "" {
6952+
if body != nil {
6953+
t.Fatalf("Expected error:\n\n%s\n\nbut got body:\n\n%s", tc.expErr, body.String())
6954+
}
6955+
if errStr := err.Error(); errStr != tc.expErr {
6956+
t.Fatalf("Expected error:\n\n%s\n\nbut got body:\n\n%s", tc.expErr, errStr)
6957+
}
6958+
}
6959+
})
6960+
}
6961+
}

‎v1/test/e2e/metrics/metrics_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,6 @@ func assertDataInstrumentationMetricsInMap(t *testing.T, includeCompile bool, me
212212
"timer_server_handler_ns",
213213
}
214214
compileStageKeys := []string{
215-
"timer_rego_query_parse_ns",
216215
"timer_rego_query_compile_ns",
217216
"timer_query_compile_stage_build_comprehension_index_ns",
218217
"timer_query_compile_stage_check_safety_ns",

0 commit comments

Comments
 (0)