Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions ftwhttp/header.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package ftwhttp

import (
"bufio"
"fmt"
"io"
"net/textproto"
"strings"
Expand Down Expand Up @@ -110,6 +111,10 @@ func (h *Header) GetAll(name string) []HeaderTuple {
func (h *Header) Write(writer io.Writer) error {
buf := bufio.NewWriter(writer)
for index, tuple := range h.entries {
if strings.ContainsAny(tuple.Name, "\r\n") || strings.ContainsAny(tuple.Value, "\r\n") {
return fmt.Errorf("header %q contains a raw CR or LF character, which would corrupt the request; "+
"use 'encoded_request' or 'encoded_data' to send control characters intentionally", tuple.Name)
}
if log.Trace().Enabled() {
log.Trace().Msgf("Writing header %d: %s: %s", index, tuple.Name, tuple.Value)
}
Expand Down
21 changes: 21 additions & 0 deletions ftwhttp/header_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,27 @@ func (s *headerTestSuite) TestWrite() {
}
}

func (s *headerTestSuite) TestWriteRejectsCRLFInValue() {
// A YAML block scalar (`|`) silently appends a trailing "\n" to a header
// value. Left unchecked, this breaks header framing on the wire instead
// of producing a visible error (see coreruleset/go-ftw#662).
h := NewHeaderWithEntries([]*HeaderTuple{
{"X-Test", "value\n"},
})
buf := &bytes.Buffer{}
err := h.Write(buf)
s.Error(err)
}

func (s *headerTestSuite) TestWriteRejectsCRLFInName() {
h := NewHeaderWithEntries([]*HeaderTuple{
{"X-Test\r\nInjected", "value"},
})
buf := &bytes.Buffer{}
err := h.Write(buf)
s.Error(err)
}

func (s *headerTestSuite) TestAdd() {
h := NewHeader()
h.Add("CustOm", "Value")
Expand Down
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ require (
github.com/Masterminds/sprig/v3 v3.3.0
github.com/corazawaf/coraza/v3 v3.8.1
github.com/coreruleset/ftw-tests-schema/v2 v2.3.0
github.com/coreruleset/ftw-tests-schema/v3 v3.0.1
github.com/creativeprojects/go-selfupdate v1.5.2
github.com/go-logr/zerologr v1.2.3
github.com/google/uuid v1.6.0
Expand Down
1 change: 0 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ github.com/corazawaf/libinjection-go v0.3.3 h1:NhbXKRfRpqKzBMzv8zpCcnjyEw7BCVhBO
github.com/corazawaf/libinjection-go v0.3.3/go.mod h1:Ik/+w3UmTWH9yn366RgS9D95K3y7Atb5m/H/gXzzPCk=
github.com/coreruleset/ftw-tests-schema/v2 v2.3.0 h1:zs2US8z7sDba1O//qgaD1qj5R5B90m6FjHuRAuKK03g=
github.com/coreruleset/ftw-tests-schema/v2 v2.3.0/go.mod h1:wvQApSsBz7qy0aGb88UwYWOnu6k6DlyPsJ3QoJ+tRhA=
github.com/coreruleset/ftw-tests-schema/v3 v3.0.1/go.mod h1:S1MlcTtcRrohmajCl2gS/if1rky0qIuN0VN95lqrLZg=
github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
github.com/creativeprojects/go-selfupdate v1.5.2 h1:3KR3JLrq70oplb9yZzbmJ89qRP78D1AN/9u+l3k0LJ4=
github.com/creativeprojects/go-selfupdate v1.5.2/go.mod h1:BCOuwIl1dRRCmPNRPH0amULeZqayhKyY2mH/h4va7Dk=
Expand Down
3 changes: 1 addition & 2 deletions runner/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ func Run(runnerConfig *config.RunnerConfig, tests []*test.FTWTest, out *output.O
if err != nil {
return &TestRunContext{}, err
}
defer cleanLogs(logLines)
Comment thread
fzipi marked this conversation as resolved.

client, err := ftwhttp.NewClient(runnerConfig)
if err != nil {
Expand Down Expand Up @@ -61,8 +62,6 @@ func Run(runnerConfig *config.RunnerConfig, tests []*test.FTWTest, out *output.O

runContext.Stats.printSummary(out)

defer cleanLogs(logLines)

return runContext, nil
}

Expand Down
9 changes: 9 additions & 0 deletions runner/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,15 @@ func (s *runTestSuite) TestLogsRun() {
s.LessOrEqual(0, res.Stats.TotalFailed(), "Oops, test run failed!")
}

func (s *runTestSuite) TestHeaderCRLFInjectionRun() {
// A header value containing a raw CR or LF (e.g. from an unquoted YAML
// block scalar) must not be silently sent as a broken request; it should
// surface as an error instead of trivially passing. See coreruleset/go-ftw#662.
_, err := Run(s.runnerConfig, s.ftwTests, s.out)
s.Require().Error(err)
s.Contains(err.Error(), "CR or LF")
}

func (s *runTestSuite) TestFailedTestsRun() {
res, err := Run(s.runnerConfig, s.ftwTests, s.out)
s.Require().NoError(err)
Expand Down
20 changes: 20 additions & 0 deletions runner/testdata/TestHeaderCRLFInjectionRun.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
meta:
author: "tester"
description: "Regression test for coreruleset/go-ftw#662"
tests:
- test_id: 10
stages:
- input:
dest_addr: "{{ .TestAddr }}"
port: {{ .TestPort }}
headers:
User-Agent: "ModSecurity CRS 3 Tests"
Accept: "*/*"
Host: "localhost"
# A YAML block scalar appends a trailing "\n" to the value.
sec-ch-ua-full-version-list: |
"Google Chrome";v="149.0.0.0", "Chromium";v="149.0.0.0", "Not)A;Brand";v="24.0.0.0"
output:
log:
no_expect_ids: [920274]
Loading