c.bind() binds both POST request body and query to given struct causing security vulnerability and problems with slices

[aldas]

Issue Description

Commit b129098 removed content length and request method check from c.bind() function. This causes many unwanted side effects

1. 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)
    }
}

2. 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

[aldas]

NB: related issue #1638 #1356

[pafuent]

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]

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 lang params to decide in which language result messages should be and in your POST (create user endpoint) struct you have field lang that 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 lang field 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:

• params
• query
• body

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]

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.

[aldas]

workaround would be to use #1681 if it gets merged

or for POST/PUT body and json it is easy to marshal body by your own
NB: assuming you do not want to bind route params or query params to the same struct

instead of:

if err := c.Bind(&payload); err != nil {
	return err
}

use:

if err := json.NewDecoder(c.Request().Body).Decode(&payload); err != nil {
	return err
}