Prevent NPE if multipart form is empty #13

zeripath · 2022-10-09T12:28:18Z

zeripath commented

2022-10-09 12:28:18 +00:00

In https://github.com/go-gitea/gitea/issues/19698 a fuzzer has passed an
empty multipart form which causes go-chi/binding to throw an NPE.

This PR simply protects against this by checking if the multipart form is nil
before trying to map it.

Signed-off-by: Andrew Thornton art27@cantab.net

In https://github.com/go-gitea/gitea/issues/19698 a fuzzer has passed an empty multipart form which causes go-chi/binding to throw an NPE. This PR simply protects against this by checking if the multipart form is nil before trying to map it. Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath added 1 commit 2022-10-09 12:28:18 +00:00

Prevent NPE if multipart form is empty

In https://github.com/go-gitea/gitea/issues/19698 a fuzzer has passed an
empty multipart form which causes go-chi/binding to throw an NPE.

This PR simply protects against this by checking if the multipart form is nil
before trying to map it.

Signed-off-by: Andrew Thornton <art27@cantab.net>

continuous-integration/drone/pr Build is failing

Details

f995d8c90c

zeripath added 1 commit 2022-10-09 12:28:25 +00:00

Merge branch 'master' into prevent-panic-with-empty-multipart-form

continuous-integration/drone/pr Build is passing

Details

4cb1bae3ff

wxiaoguang reviewed 2022-10-09 12:34:54 +00:00

binding.go

						
				@ -150,2 +150,4 @@

								req.ParseForm()

							}

							if form == nil {

								return append(errors, Validate(req, formStruct)...)

wxiaoguang commented

2022-10-09 12:34:54 +00:00

It looks good.

ps: if the form, parseErr := multipartReader.ReadForm(MaxMemory) gets error, the error should have been reported by line 146.

Maybe the change code be:

			req.MultipartForm, err = multipartReader.ReadForm(MaxMemory)
			if err != nil {
				errors.Add([]string{}, ERR_DESERIALIZATION, err.Error())
			} else {
				if req.Form == nil {
					req.ParseForm()
				}
				for k, v := range form.Value {
					req.Form[k] = append(req.Form[k], v...)
				}
			}
		}
	}
	if req.MultipartForm != nil {
		errors = mapForm(formStructV, req.MultipartForm.Value, req.MultipartForm.File, errors)
	}
	return append(errors, Validate(req, formStruct)...)
}

-- OR --

Just return as early as multipartReader.ReadForm returns error. If the form is broken, it's not necessary to do req.ParseForm() for the broken form IMO.

It looks good. ps: if the `form, parseErr := multipartReader.ReadForm(MaxMemory)` gets error, the error should have been reported by line 146. Maybe the change code be: ``` req.MultipartForm, err = multipartReader.ReadForm(MaxMemory) if err != nil { errors.Add([]string{}, ERR_DESERIALIZATION, err.Error()) } else { if req.Form == nil { req.ParseForm() } for k, v := range form.Value { req.Form[k] = append(req.Form[k], v...) } } } } if req.MultipartForm != nil { errors = mapForm(formStructV, req.MultipartForm.Value, req.MultipartForm.File, errors) } return append(errors, Validate(req, formStruct)...) } ``` -- OR -- Just return as early as `multipartReader.ReadForm` returns error. If the form is broken, it's not necessary to do `req.ParseForm()` for the broken form IMO.