Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoids buffering bodies when not accessible. Partial fix for Response body buffer #76

Merged
merged 9 commits into from
Nov 15, 2022
Merged

Avoids buffering bodies when not accessible. Partial fix for Response body buffer #76

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
675620b
Adds no buffer logic if no access, upgrades coraza
M4tteoP Nov 8, 2022
9cc4191
adds tests
M4tteoP Nov 8, 2022
b70f7b1
Merge branch 'corazawaf:main' into no_access_no_buffer
M4tteoP Nov 9, 2022
7c16766
fixes action pause on HttpResponseBody, adds debug log
M4tteoP Nov 9, 2022
f8d4b32
lint and formatting
M4tteoP Nov 9, 2022
a52046a
fix test
M4tteoP Nov 9, 2022
b7652bc
moving to LogDebug
M4tteoP Nov 9, 2022
f82cae7
comment
M4tteoP Nov 9, 2022
b5b0656
Merge branch 'main' into no_access_no_buffer
M4tteoP Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixes action pause on HttpResponseBody, adds debug log
  • Loading branch information
@M4tteoP
M4tteoP committed Nov 9, 2022
commit 7c167668cbeaf6b1e36d6fb7cb2de62a57dc287d
24 changes: 14 additions & 10 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,26 +267,30 @@ func (ctx *httpContext) OnHttpResponseBody(bodySize int, endOfStream bool) types
return types.ActionContinue
}

if bodySize > 0 {
body, err := proxywasm.GetHttpResponseBody(ctx.responseBodySize, bodySize)
ctx.responseBodySize += bodySize
// Wait until we see the entire body. It has to be buffered in order to check that it is fully legit
// before sending it upstream
if !endOfStream {
// TODO(M4tteoP): Update response body interruption logic after https://github.com/corazawaf/coraza-proxy-wasm/issues/26
return types.ActionPause
}

if ctx.responseBodySize > 0 {
body, err := proxywasm.GetHttpResponseBody(0, ctx.responseBodySize)
if len(body) != ctx.responseBodySize {
proxywasm.LogDebugf("warning: retrieved response body size different from the sum of all bodySizes. %d != %d", len(body), ctx.responseBodySize)
}
if err != nil {
proxywasm.LogCriticalf("failed to get response body: %v", err)
return types.ActionContinue
}
ctx.responseBodySize += bodySize
_, err = tx.ResponseBodyWriter().Write(body)
if err != nil {
proxywasm.LogCriticalf("failed to read response body: %v", err)
proxywasm.LogCriticalf("failed to write response body: %v", err)
return types.ActionContinue
}
}

// Response body has to be buffered in order to check that it is fully legit
if !endOfStream {
// TODO(M4tteoP): Update response body interruption logic after https://github.com/corazawaf/coraza-proxy-wasm/issues/26
return types.ActionPause
}

// We have already sent response headers, an unauthorized response can not be sent anymore,
// but we can still drop the response to prevent leaking sensitive content.
// The error will also be logged by Coraza.
Expand Down
114 changes: 57 additions & 57 deletions main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ func TestLifecycle(t *testing.T) {
{
name: "url accepted",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_URI \"@streq /admin\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_URI \"@streq /admin\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -73,8 +73,8 @@ func TestLifecycle(t *testing.T) {
{
name: "url denied",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_URI \"@streq /hello?name=panda\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_URI \"@streq /hello?name=panda\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionPause,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -84,8 +84,8 @@ func TestLifecycle(t *testing.T) {
{
name: "method accepted",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_METHOD \"@streq post\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_METHOD \"@streq post\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -95,8 +95,8 @@ func TestLifecycle(t *testing.T) {
{
name: "method denied",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_METHOD \"@streq get\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_METHOD \"@streq get\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionPause,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -106,8 +106,8 @@ func TestLifecycle(t *testing.T) {
{
name: "protocol accepted",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/2.0\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/2.0\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -116,8 +116,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/2.0\" \"id:101,phase:1,
{
name: "protocol denied",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionPause,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -126,8 +126,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request header name accepted",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_HEADERS_NAMES \"@streq accept-encoding\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_HEADERS_NAMES \"@streq accept-encoding\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -137,8 +137,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request header name denied",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_HEADERS_NAMES \"@streq user-agent\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_HEADERS_NAMES \"@streq user-agent\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionPause,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -148,8 +148,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request header value accepted",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_HEADERS:user-agent \"@streq rusttest\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_HEADERS:user-agent \"@streq rusttest\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -159,8 +159,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request header value denied",
inlineRules: `
SecRuleEngine On\nSecRule REQUEST_HEADERS:user-agent \"@streq gotest\" \"id:101,phase:1,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule REQUEST_HEADERS:user-agent \"@streq gotest\" \"id:101,phase:1,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionPause,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -170,8 +170,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request body accepted",
inlineRules: `
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"name=yogi\" \"id:101,phase:2,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"name=yogi\" \"id:101,phase:2,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -181,8 +181,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request body denied, end of body",
inlineRules: `
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"name=pooh\" \"id:101,phase:2,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"name=pooh\" \"id:101,phase:2,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionPause,
responseHdrsAction: types.ActionContinue,
Expand All @@ -192,8 +192,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "request body denied, start of body",
inlineRules: `
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"animal=bear\" \"id:101,phase:2,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRequestBodyAccess On\nSecRule REQUEST_BODY \"animal=bear\" \"id:101,phase:2,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionPause,
responseHdrsAction: types.ActionContinue,
Expand All @@ -214,8 +214,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "status accepted",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_STATUS \"500\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_STATUS \"500\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -225,8 +225,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "status denied",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_STATUS \"200\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_STATUS \"200\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionPause,
Expand All @@ -236,8 +236,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "status accepted rx",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_STATUS \"@rx [^\\d]+\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_STATUS \"@rx [^\\d]+\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -247,8 +247,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "status denied rx",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_STATUS \"@rx [\\d]+\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_STATUS \"@rx [\\d]+\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionPause,
Expand All @@ -258,8 +258,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "response header name accepted",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_HEADERS_NAMES \"@streq transfer-encoding\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_HEADERS_NAMES \"@streq transfer-encoding\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -269,8 +269,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "response header name denied",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_HEADERS_NAMES \"@streq server\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_HEADERS_NAMES \"@streq server\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionPause,
Expand All @@ -280,8 +280,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "response header value accepted",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_HEADERS:server \"@streq rusttest\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_HEADERS:server \"@streq rusttest\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -291,8 +291,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "response header value denied",
inlineRules: `
SecRuleEngine On\nSecRule RESPONSE_HEADERS:server \"@streq gotest\" \"id:101,phase:3,t:lowercase,deny\"
`,
SecRuleEngine On\nSecRule RESPONSE_HEADERS:server \"@streq gotest\" \"id:101,phase:3,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionPause,
Expand All @@ -302,8 +302,8 @@ SecRuleEngine On\nSecRule REQUEST_PROTOCOL \"@streq http/1.1\" \"id:101,phase:1,
{
name: "response body accepted",
inlineRules: `
SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains pooh\" \"id:101,phase:4,t:lowercase,deny\"
`,
SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains pooh\" \"id:101,phase:4,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -313,8 +313,8 @@ SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains po
{
name: "response body denied, end of body",
inlineRules: `
SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains yogi\" \"id:101,phase:4,t:lowercase,deny\"
`,
SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains yogi\" \"id:101,phase:4,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
Expand All @@ -332,17 +332,17 @@ SecRuleEngine On\nSecResponseBodyAccess On\nSecRule RESPONSE_BODY \"@contains he
responded403: false,
respondedNullBody: true,
},
// {
// name: "response body accepted, no response body access",
// inlineRules: `
// SecRuleEngine On\nSecResponseBodyAccess Off\nSecRule RESPONSE_BODY \"@contains hello\" \"id:101,phase:4,t:lowercase,deny\"
// `,
// requestHdrsAction: types.ActionContinue,
// requestBodyAction: types.ActionContinue,
// responseHdrsAction: types.ActionContinue,
// responded403: false,
// respondedNullBody: false,
// },
{
name: "response body accepted, no response body access",
inlineRules: `
SecRuleEngine On\nSecResponseBodyAccess Off\nSecRule RESPONSE_BODY \"@contains hello\" \"id:101,phase:4,t:lowercase,deny\"
`,
requestHdrsAction: types.ActionContinue,
requestBodyAction: types.ActionContinue,
responseHdrsAction: types.ActionContinue,
responded403: false,
respondedNullBody: false,
},
}

vmTest(t, func(t *testing.T, vm types.VMContext) {
Expand Down