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

Remote Code Execution on saveConfig #620

Closed
operatorequals opened this Issue Jun 21, 2017 · 18 comments

Comments

Projects
None yet
6 participants
@operatorequals

operatorequals commented Jun 21, 2017

In line:
https://github.com/E2OpenPlugins/e2openplugin-OpenWebif/blob/1.2.4/plugin/controllers/models/config.py#L150
the eval() call blindly executes any user supplied data.
For example a remote unauthenticated attacker can use the following GET request to create a root-owned file under /tmp/ on a "Dreambox 800HD se" device:

GET /api/saveconfig?key=__import__(%27os%27).system(%27touch%20%2Ftmp%2Fpy100%20%26%27)&value=10&_=1496061368064 HTTP/1.1
Host: 192.168.0.2
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
X-Requested-With: XMLHttpRequest
Referer: http://192.168.0.2/
Connection: close

We will be requesting a CVE for this issue and will report the CVE number once this becomes available, for issue coordination purposes.

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 21, 2017

first shot:
NOT tested!!!!

web.py new P_saveconfig

def P_saveconfig(self, request):
	res = self.testMandatoryArguments(request, ["key", "value"])
	if res:
		return res
	key = request.args["key"][0]
	if "/" in key or "%" in key or " " in key:
		return {
			"result": False
			}
	return saveConfig(request.args["key"][0], request.args["value"][0])


@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 21, 2017

AND/OR
...
if key.startswith( 'config.' ):
...

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 21, 2017

	def P_saveconfig(self, request):
		res = self.testMandatoryArguments(request, ["key", "value"])
		if res:
			return res
		key = request.args["key"][0]
		if "/" in key or "%" in key or " " in key or not key.startswith("config."):
			return {
				"result": False
				}
		return saveConfig(request.args["key"][0], request.args["value"][0])

@Schimmelreiter

This comment has been minimized.

Contributor

Schimmelreiter commented Jun 21, 2017

Do we need any chars except alphanumeric and "." inside a key?
Can't we also test of that key exists?

What will happen with values?

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 21, 2017

Values can be ignored.
We can also test the second part of the key.
config.xxx.
xxx must be exist. -> see config.py getConfigsSections
But these sections needs to be cached.

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 21, 2017

Then we can check the length of the 3rd part : max 30 or 50
And we can use POST for the save request.

@Schimmelreiter

This comment has been minimized.

Contributor

Schimmelreiter commented Jun 22, 2017

Will you test and push those changes?

@operatorequals

This comment has been minimized.

operatorequals commented Jun 22, 2017

This issue has been assigned the CVE 2017-9807.

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 22, 2017

Ich hab grad keine Zeit zum testen.

@operatorequals

This comment has been minimized.

operatorequals commented Jun 23, 2017

Maybe the below snippet:

from string import lowercase
accepted = lowercase + '.'    # "abcdefghijklmnopqrstuvwxyz."
not all([c in accepted for c in key] )
# Returns True if ANY non legitimate character exists in "key"

can replace the below check (found in P_saveconfig) in more depth:

# Blacklists the "/", "%" and space (" ") characters.
"/" in key or "%" in key or " " in key
# Returns True if any blacklisted character exists in "key"

This can happen because it whitelists only acceptable characters (lowercase letters and dot), and returns True if any non-acceptable character exists in the key parameter.

@Schimmelreiter

This comment has been minimized.

Contributor

Schimmelreiter commented Jun 23, 2017

Any alphanumeric (capital, non-capital plus digits) plus "." should be acceptable.

@operatorequals

This comment has been minimized.

operatorequals commented Jun 23, 2017

Pardon me, I am new to the project!
Then my code snippet can be transformed like follows:

from string import letters, digits
accepted = letters + digits + '.'    # "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789."
not all([c in accepted for c in key] )
# Returns True if ANY non legitimate character exists in "key"

The string constants are available here: https://docs.python.org/2/library/string.html

@operatorequals

This comment has been minimized.

operatorequals commented Jun 23, 2017

Or much better use the ascii_letters constant as is not dependent on running system's Locale.
Transformed snippet:

from string import ascii_letters, digits
accepted = ascii_letters + digits + '.'    # "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789."
not all([c in accepted for c in key] )
# Returns True if ANY non legitimate character exists in "key"

As my system has an English Locale the difference cannot be seen in those two snippets right away, but if run on a different Locale system (a Greek one for example) the check I previously posted will always fail for legitimate key values.

I believe that this one is as safe as it gets when comes to Locale issues...

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 24, 2017

I think there is a easy way.
Step 1: Split the key string by . and check count (eq 3)
Step 2: check the 1. value / must equal "config"
Step 3: check the second part / must be one of (config.py getConfigsSections)
Step 4: check the third part / black list letters (#/%)

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 24, 2017

	def P_saveconfig(self, request):
		res = self.testMandatoryArguments(request, ["key", "value"])
		if res:
			return res
		key = request.args["key"][0]
		keys = key.split('.')
		if len(keys) = 3 and keys[0] = 'config':
			sect = getConfigsSections['sections']
			if keys[1] in sect:
				if "/" not in key[2] and "%" not in key[2]:
					return saveConfig(key, request.args["value"][0])
		return {
			"result": False
			}

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 24, 2017

UPDATE:
getConfigsSections do not work as expected.

	def P_saveconfig(self, request):
		res = self.testMandatoryArguments(request, ["key", "value"])
		if res:
			return res
		key = request.args["key"][0]
		if "/" not in key and "%" not in key and "." in key:
			keys = key.split('.')
			if len(keys) == 3 and keys[0] == 'config':
				return saveConfig(key, request.args["value"][0])
		return {"result": False}

@Schimmelreiter

This comment has been minimized.

Contributor

Schimmelreiter commented Jun 24, 2017

Sounds good to me.

@jbleyel

This comment has been minimized.

Contributor

jbleyel commented Jun 24, 2017

I will also switch to POST for this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment