Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DoS vulnerability in Revel framework #1424

Closed
SYM01 opened this issue Mar 5, 2019 · 7 comments
Closed

DoS vulnerability in Revel framework #1424

SYM01 opened this issue Mar 5, 2019 · 7 comments
Assignees
Labels
effort-minutes Will take up to 60 minutes to implement priority-must now, top priority status-needs-testing Code complete, needs peer verification type-enhancement New enhancement of existing feature waiting-for-reply Question raised, need answer

Comments

@SYM01
Copy link
Contributor

SYM01 commented Mar 5, 2019

Once the slices parameter feature is used, the website will be suffering from DoS attack.

When we need to get a slice parameter, we may use the following code snippet in our controller.

func (c App) DoS1(name []string) revel.Result {
	return c.RenderText(fmt.Sprint(name))
}

func (c App) DoS2() revel.Result {
	var name []string
	c.Params.Bind(&name, "name")
	return c.RenderText(fmt.Sprint(name))
}

It looks like everything is OK. However, we can exhaust the server's MEM with only one request.
e.g., when simply visit http://localhost:9000/dos1?name[1234567890]=1, the server's CPU and memory usage will soar, and the OOM killer will be triggered eventually.

This vulnerability was caused by the following code:

revel/binder.go

Lines 210 to 277 in a3d7a7c

func bindSlice(params *Params, name string, typ reflect.Type) reflect.Value {
// Collect an array of slice elements with their indexes (and the max index).
maxIndex := -1
numNoIndex := 0
sliceValues := []sliceValue{}
// Factor out the common slice logic (between form values and files).
processElement := func(key string, vals []string, files []*multipart.FileHeader) {
if !strings.HasPrefix(key, name+"[") {
return
}
// Extract the index, and the index where a sub-key starts. (e.g. field[0].subkey)
index := -1
leftBracket, rightBracket := len(name), strings.Index(key[len(name):], "]")+len(name)
if rightBracket > leftBracket+1 {
index, _ = strconv.Atoi(key[leftBracket+1 : rightBracket])
}
subKeyIndex := rightBracket + 1
// Handle the indexed case.
if index > -1 {
if index > maxIndex {
maxIndex = index
}
sliceValues = append(sliceValues, sliceValue{
index: index,
value: Bind(params, key[:subKeyIndex], typ.Elem()),
})
return
}
// It's an un-indexed element. (e.g. element[])
numNoIndex += len(vals) + len(files)
for _, val := range vals {
// Unindexed values can only be direct-bound.
sliceValues = append(sliceValues, sliceValue{
index: -1,
value: BindValue(val, typ.Elem()),
})
}
for _, fileHeader := range files {
sliceValues = append(sliceValues, sliceValue{
index: -1,
value: BindFile(fileHeader, typ.Elem()),
})
}
}
for key, vals := range params.Values {
processElement(key, vals, nil)
}
for key, fileHeaders := range params.Files {
processElement(key, nil, fileHeaders)
}
resultArray := reflect.MakeSlice(typ, maxIndex+1, maxIndex+1+numNoIndex)
for _, sv := range sliceValues {
if sv.index != -1 {
resultArray.Index(sv.index).Set(sv.value)
} else {
resultArray = reflect.Append(resultArray, sv.value)
}
}
return resultArray
}

When the function above is invoked, Revel will calc the maxIndex from user's input, and thus the attacker could make the maxIndex as large as possible. When reflect.MakeSlice is called, Golang will alloc a large memory as the maxIndex needed.

A possible solution for this vuln is to specify the upper bound of the maxIndex in the code or config file.

Before the problem is fixed, plz avoid using the slices parameter feature.

SYM01 added a commit to SYM01/revel that referenced this issue Mar 5, 2019
@notzippy
Copy link
Collaborator

notzippy commented Mar 8, 2019

Yes, I can see how that could be an issue, but would it not be a better idea to place the check on where maxIndex is set (note the check added on the if index > -1 && maxIndex < maxIndexBound { below) ?

	maxIndexBound := Config.IntDefault("params.max_index", 1024)

	// Factor out the common slice logic (between form values and files).
	processElement := func(key string, vals []string, files []*multipart.FileHeader) {
		if !strings.HasPrefix(key, name+"[") {
			return
		}

		// Extract the index, and the index where a sub-key starts. (e.g. field[0].subkey)
		index := -1
		leftBracket, rightBracket := len(name), strings.Index(key[len(name):], "]")+len(name)
		if rightBracket > leftBracket+1 {
			index, _ = strconv.Atoi(key[leftBracket+1 : rightBracket])
		}
		subKeyIndex := rightBracket + 1

		// Handle the indexed case.
		if index > -1 && maxIndex < maxIndexBound {
			if index > maxIndex {
				maxIndex = index
			}
			sliceValues = append(sliceValues, sliceValue{
				index: index,
				value: Bind(params, key[:subKeyIndex], typ.Elem()),
			})
			return
		}
...

@SYM01
Copy link
Contributor Author

SYM01 commented Mar 8, 2019

Although it looks more concise, it may be confused when visiting https://example.com/?name[1024]=1, the developer will get a slice like []string{1} which indicate that name[0] == "1".

@notzippy
Copy link
Collaborator

notzippy commented Mar 8, 2019

OK, so in this situation it would be best to ignore the parameter all together ? And likely a good idea to log the error.

		if index > -1  {
                        if maxIndex > maxIndexBound {
                            binderLog.Error.Println("Invalid parameter index, ignoring parameter","index", maxIndex,"key",key)
                            return
                        } 
			if index > maxIndex {
				maxIndex = index
			}
			sliceValues = append(sliceValues, sliceValue{
				index: index,
				value: Bind(params, key[:subKeyIndex], typ.Elem()),
			})
			return
		}

@SYM01
Copy link
Contributor Author

SYM01 commented Mar 8, 2019 via email

SYM01 added a commit to SYM01/revel that referenced this issue Mar 9, 2019
notzippy added a commit that referenced this issue Mar 10, 2019
@brendensoares brendensoares added status-needs-testing Code complete, needs peer verification waiting-for-reply Question raised, need answer labels Feb 8, 2020
@brendensoares
Copy link
Member

I've marked this as needs testing, though it may be ready for release. @notzippy any idea what the latest status is here?

@brendensoares brendensoares added effort-minutes Will take up to 60 minutes to implement priority-must now, top priority type-enhancement New enhancement of existing feature labels Feb 8, 2020
@brendensoares
Copy link
Member

pinging this; high priority

@notzippy notzippy self-assigned this Mar 5, 2022
@notzippy
Copy link
Collaborator

notzippy commented Mar 5, 2022

@brendensoares This has been fixed and merged see #1427 and should be closed

https://github.com/revel/revel/blob/master/binder.go#L232-L247

@notzippy notzippy closed this as completed Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-minutes Will take up to 60 minutes to implement priority-must now, top priority status-needs-testing Code complete, needs peer verification type-enhancement New enhancement of existing feature waiting-for-reply Question raised, need answer
Projects
None yet
Development

No branches or pull requests

3 participants