Repository navigation
c.bind() binds both POST request body and query to given struct causing security vulnerability and problems with slices #1670
Description
Issue Description
Commit b129098 removed content length and request method check from c.bind() function. This causes many unwanted side effects
- security vulnerability - now it is possible to inject field values from query param to POST method body with bind. This could allow attacker to inject to fields that are not meant to be filled from bind. I know that you should not pass binded values directly to services etc but before this change this was not possible with query params. It is way easier to manipulate query params than request body.
see example:
func TestSecurityRisk(t *testing.T) {
type User struct {
ID int `json:"id"`
IsAdmin bool // not 'exposed' value should not be bind DEFINITELY from URL/ROUTE parameters
}
e := echo.New()
req := httptest.NewRequest(http.MethodPost, "/api/endpoint?IsAdmin=true", strings.NewReader(`{"id": 1}`))
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
err := func(c echo.Context) error {
var payload User
if err := c.Bind(&payload); err != nil {
return c.JSON(http.StatusBadRequest, echo.Map{"error": err})
}
// passing bind value directly to service is bad practice and could lead to security risks but c.bind()
// doing it implicitly is also very bad (even worse)
// err := service.addUser(c.Request().Context(), user)
if payload.IsAdmin {
panic("passing bind value directly to service is bad practice and could lead to security risks")
}
return c.JSON(http.StatusOK, payload)
}(c)
if err != nil {
t.Fatal(err)
}
}- if request contains query param + body and we are binding to slice request fails with
binding element must be a struct
Example:
func TestBindingToSliceWithQueryParam(t *testing.T) {
type User struct {
ID int `json:"id"`
}
e := echo.New()
req := httptest.NewRequest(http.MethodPost, "/api/endpoint?lang=et", strings.NewReader(`[{"id": 1}]`))
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
err := func(c echo.Context) error {
var payload []User
if err := c.Bind(&payload); err != nil {
return c.JSON(http.StatusBadRequest, echo.Map{"error": err})
}
return c.JSON(http.StatusOK, payload)
}(c)
if err != nil {
t.Fatal(err)
}
response := string(rec.Body.Bytes())
if strings.Contains(response, "binding element must be a struct"){
t.Fatal(response)
}
}Expected behaviour
I think expected behavior is as it was before b129098
Version/commit
v4.1.17
Activity
aldas commentedon Nov 6, 2020
NB: related issue #1638 #1356
pafuent commentedon Nov 8, 2020
Regarding the query parameter that fills what should be filled with the Body, please see my comment
It contains some examples to show how binding works on Echo. Also at the end there is a promise (😉) of a new PR adding a configuration to the default Binder to turn on/off the automatic binding by struct field name. That could prevent the vulnerability only "exposing" the bindings that you want. Please let me know if that could be useful to you and I'll try to come up with something.
aldas commentedon Nov 9, 2020
I agree on idea that where bind takes its data should have choice for user but it should have also 'safe' defaults for all users. Mixing query params with POST/PUT body is not safe or expected behavior in my opinion.
For example POST: your requests have
langparams to decide in which language result messages should be and in your POST (create user endpoint) struct you have fieldlangthat could be optional for business logic. Lets assume I'm German and have switched UI to German for myself, application that I work with is primarily for English speaking people and language defaults to English. So in case you leave it empty it would be still filled from query - setting to German although user would expect English.Anyway this
langfield could have different 'meaning'. We are unintentionally/implicitly mixing things with (possibly) different semantics.For example GET: you have endpoint to list some entities. You provide extensive API to order, filter, paginate etc by query params. So you would have many query parameters. In that case it is nice to bind all query params to struct. Seems that there are even (rare) cases when body is used with GET. But back to this example - in this case this is what I expect to happens and not 'side effect'.
It would be nice if user would have choice to switch on/off binding blocks for:
sometimes you wand some of them (GETs?), sometimes only one specific
NB: this choice could be different for every handler function - definitely GETs have most of the time different requirements than POST/PUT/PATCH etc.
pafuent commentedon Nov 10, 2020
I just noticed that when I copy&paste my previous comment the link were removed (my bad 😞). Please see this
The examples shows how to only bind for query, params or body.
Adding a new config to DefaultBinder
aldas commentedon Nov 20, 2020
workaround would be to use #1681 if it gets merged
or for
POST/PUTbody andjsonit is easy to marshal body by your ownNB: assuming you do not want to bind route params or query params to the same struct
instead of:
use:
feat: Bind data using headers as source
feat: Bind data using headers as source (#1866)
feat: enable always bind query params