Skip to content

Commit f2262c8

Browse files
ccojocarccojocar
authored
G120: avoid false positive when MaxBytesReader is applied in middleware (#1547)
Signed-off-by: Cosmin Cojocar <cosmin@cojocar.ch>
1 parent 5b580c7 commit f2262c8

File tree

2 files changed

+244
-1
lines changed

2 files changed

+244
-1
lines changed

‎analyzers/form_parsing_limits.go‎

Lines changed: 199 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,15 @@ func runFormParsingLimitAnalysis(pass *analysis.Pass) (any, error) {
4444
}
4545

4646
issuesByPos := make(map[token.Pos]*issue.Issue)
47+
handlerProtection := computeFormParsingHandlerProtection(ssaResult.SSA.SrcFuncs)
4748

4849
for _, fn := range collectAnalyzerFunctions(ssaResult.SSA.SrcFuncs) {
4950
requestParam, writerParam := findHandlerRequestAndWriterParams(fn)
5051
if requestParam == nil || writerParam == nil {
5152
continue
5253
}
5354

54-
hasRequestBodyLimit := functionHasRequestBodyLimit(fn, requestParam, writerParam)
55+
hasRequestBodyLimit := handlerProtection[fn]
5556
if hasRequestBodyLimit {
5657
continue
5758
}
@@ -82,6 +83,203 @@ func runFormParsingLimitAnalysis(pass *analysis.Pass) (any, error) {
8283
return issues, nil
8384
}
8485

86+
func computeFormParsingHandlerProtection(srcFuncs []*ssa.Function) map[*ssa.Function]bool {
87+
protection := make(map[*ssa.Function]bool)
88+
allFuncs := collectAnalyzerFunctions(srcFuncs)
89+
for _, fn := range allFuncs {
90+
requestParam, writerParam := findHandlerRequestAndWriterParams(fn)
91+
if requestParam == nil || writerParam == nil {
92+
continue
93+
}
94+
if functionHasRequestBodyLimit(fn, requestParam, writerParam) {
95+
protection[fn] = true
96+
continue
97+
}
98+
if isProtectedByWrapperCall(fn, allFuncs) {
99+
protection[fn] = true
100+
}
101+
}
102+
103+
return protection
104+
}
105+
106+
func isProtectedByWrapperCall(handler *ssa.Function, allFuncs []*ssa.Function) bool {
107+
for _, fn := range allFuncs {
108+
if fn == nil {
109+
continue
110+
}
111+
for _, block := range fn.Blocks {
112+
for _, instr := range block.Instrs {
113+
callInstr, ok := instr.(ssa.CallInstruction)
114+
if !ok {
115+
continue
116+
}
117+
common := callInstr.Common()
118+
if common == nil {
119+
continue
120+
}
121+
wrapper := common.StaticCallee()
122+
if wrapper == nil {
123+
continue
124+
}
125+
126+
for argIndex, arg := range common.Args {
127+
if !valueDependsOn(arg, handler, 0) {
128+
continue
129+
}
130+
if wrapperProtectsParamHandler(wrapper, argIndex) {
131+
return true
132+
}
133+
}
134+
}
135+
}
136+
}
137+
138+
return false
139+
}
140+
141+
func wrapperProtectsParamHandler(wrapper *ssa.Function, paramIndex int) bool {
142+
if wrapper == nil || paramIndex < 0 || paramIndex >= len(wrapper.Params) {
143+
return false
144+
}
145+
handlerParam := wrapper.Params[paramIndex]
146+
147+
if wrapperDelegatesWithRequestLimit(wrapper, handlerParam) {
148+
return true
149+
}
150+
151+
for _, block := range wrapper.Blocks {
152+
for _, instr := range block.Instrs {
153+
makeClosure, ok := instr.(*ssa.MakeClosure)
154+
if !ok {
155+
continue
156+
}
157+
closureFn, ok := makeClosure.Fn.(*ssa.Function)
158+
if !ok || closureFn == nil {
159+
continue
160+
}
161+
162+
requestParam, writerParam := findHandlerRequestAndWriterParams(closureFn)
163+
if requestParam == nil || writerParam == nil {
164+
continue
165+
}
166+
if !functionHasRequestBodyLimit(closureFn, requestParam, writerParam) {
167+
continue
168+
}
169+
170+
for bindingIndex, binding := range makeClosure.Bindings {
171+
if !bindingDependsOnValue(binding, handlerParam) {
172+
continue
173+
}
174+
if closureDelegatesWithRequestLimit(closureFn, bindingIndex, requestParam, writerParam) {
175+
return true
176+
}
177+
}
178+
}
179+
}
180+
181+
return false
182+
}
183+
184+
func bindingDependsOnValue(binding ssa.Value, target ssa.Value) bool {
185+
if valueDependsOn(binding, target, 0) {
186+
return true
187+
}
188+
189+
alloc, ok := binding.(*ssa.Alloc)
190+
if !ok {
191+
return false
192+
}
193+
194+
for _, ref := range safeReferrers(alloc) {
195+
store, ok := ref.(*ssa.Store)
196+
if !ok {
197+
continue
198+
}
199+
if store.Addr != alloc {
200+
continue
201+
}
202+
if valueDependsOn(store.Val, target, 0) {
203+
return true
204+
}
205+
}
206+
207+
return false
208+
}
209+
210+
func wrapperDelegatesWithRequestLimit(wrapper *ssa.Function, handlerValue ssa.Value) bool {
211+
requestParam, writerParam := findHandlerRequestAndWriterParams(wrapper)
212+
if requestParam == nil || writerParam == nil {
213+
return false
214+
}
215+
if !functionHasRequestBodyLimit(wrapper, requestParam, writerParam) {
216+
return false
217+
}
218+
return hasServeHTTPDelegation(wrapper, handlerValue, writerParam, requestParam)
219+
}
220+
221+
func closureDelegatesWithRequestLimit(closure *ssa.Function, freeVarIndex int, requestParam *ssa.Parameter, writerParam *ssa.Parameter) bool {
222+
if freeVarIndex < 0 || freeVarIndex >= len(closure.FreeVars) {
223+
return false
224+
}
225+
handlerValue := closure.FreeVars[freeVarIndex]
226+
return hasServeHTTPDelegation(closure, handlerValue, writerParam, requestParam)
227+
}
228+
229+
func hasServeHTTPDelegation(fn *ssa.Function, handlerValue ssa.Value, writerValue ssa.Value, requestValue ssa.Value) bool {
230+
for _, block := range fn.Blocks {
231+
for _, instr := range block.Instrs {
232+
call, ok := instr.(*ssa.Call)
233+
if !ok {
234+
continue
235+
}
236+
common := call.Common()
237+
if common == nil {
238+
continue
239+
}
240+
241+
var (
242+
receiver ssa.Value
243+
writer ssa.Value
244+
request ssa.Value
245+
)
246+
247+
if method := common.Method; method != nil && method.Name() == "ServeHTTP" {
248+
if len(common.Args) < 2 {
249+
continue
250+
}
251+
receiver = common.Value
252+
writer = common.Args[0]
253+
request = common.Args[1]
254+
} else {
255+
callee := common.StaticCallee()
256+
if callee == nil || callee.Name() != "ServeHTTP" || callee.Signature == nil || callee.Signature.Recv() == nil {
257+
continue
258+
}
259+
if len(common.Args) < 3 {
260+
continue
261+
}
262+
receiver = common.Args[0]
263+
writer = common.Args[1]
264+
request = common.Args[2]
265+
}
266+
267+
if !valueDependsOn(receiver, handlerValue, 0) {
268+
continue
269+
}
270+
if !valueDependsOn(writer, writerValue, 0) {
271+
continue
272+
}
273+
if !valueDependsOn(request, requestValue, 0) {
274+
continue
275+
}
276+
return true
277+
}
278+
}
279+
280+
return false
281+
}
282+
85283
func findHandlerRequestAndWriterParams(fn *ssa.Function) (*ssa.Parameter, *ssa.Parameter) {
86284
if fn == nil {
87285
return nil, nil

‎testutils/g120_samples.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,4 +63,49 @@ func handler(w http.ResponseWriter, r *http.Request) {
6363
_ = r.FormValue("name")
6464
}
6565
`}, 0, gosec.NewConfig()},
66+
67+
// Safe: middleware bounds request body before wrapped handler ParseForm call
68+
{[]string{`
69+
package main
70+
71+
import "net/http"
72+
73+
func middleware(next http.Handler) http.Handler {
74+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
75+
r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
76+
next.ServeHTTP(w, r)
77+
})
78+
}
79+
80+
func handler(w http.ResponseWriter, r *http.Request) {
81+
_ = w
82+
_ = r.ParseForm()
83+
}
84+
85+
func register() {
86+
http.Handle("/safe", middleware(http.HandlerFunc(handler)))
87+
}
88+
`}, 0, gosec.NewConfig()},
89+
90+
// Vulnerable: middleware does not bound body before wrapped handler ParseForm call
91+
{[]string{`
92+
package main
93+
94+
import "net/http"
95+
96+
func middleware(next http.Handler) http.Handler {
97+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
98+
next.ServeHTTP(w, r)
99+
})
100+
}
101+
102+
func handler(w http.ResponseWriter, r *http.Request) {
103+
_ = w
104+
_ = r.ParseForm()
105+
}
106+
107+
func register() {
108+
http.Handle("/unsafe", middleware(http.HandlerFunc(handler)))
109+
}
110+
`}, 1, gosec.NewConfig()},
66111
}

0 commit comments

Comments
 (0)