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

Fix security issue when url starts from multiple slashes. #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

valkuc
Copy link
Contributor

@valkuc valkuc commented Oct 11, 2017

No description provided.

@chmorgan
Copy link

Hi @valkuc. Appreciate the patch very much! If you've been watching there are a ton of improvements to this fork of the repository and even more due to cooperation with another fork.

For the authentication check does that require the leading slashes to work correctly and that is why removing them breaks going through the authentication check? I'm not sure I'm familiar with the authentication path.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 11, 2017

Suppose you have a next URL mappings:

{"/private/*", authBasic, cgi_private_area_auth_handler},
{"/private/secure.html", cgiEspFsTemplate, cgi_secure_tpl}

Navigating to /private/secure.html will prompt user to enter credentials. This can be "hacked" by entering url with extra slash appended. Without this patch accessing URL by //private/secure.html (note the extra slash at beginning) will skip authentication handler.

@chmorgan
Copy link

It skips the handler because it isn't an exact match for the authentication handler but is a match for the url handler?

Would it make sense to strip all but one leading '/'? The original implementation seems to remove them all.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 11, 2017

Yes
I'm actually don't see the reason to "strip all except one" but variant is acceptable I guess.

@chmorgan
Copy link

It's interesting that stripping all of the slashes results in an authentication issue. In any case I trust your testing and will apply the changes.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 11, 2017

I have thinking of why this happens, but now don't remember. I have found this issue 4-5 months ago and now decided to create a pull request with it. You can check it, the setup is quite simple. Here is a code snippet for auth handler:

int ICACHE_FLASH_ATTR cgi_private_area_auth_handler(HttpdConnData *connData, int no, char *user, int userLen, char *pass, int passLen)
{
	if (no == 0)
	{
		os_strcpy(user, PRIV_AREA_USERNAME);
		os_strcpy(pass, PRIV_AREA_PASSWORD);

		return 1;
	}

	return 0;
}

And handler for espfs page:

void ICACHE_FLASH_ATTR cgi_secure_tpl(HttpdConnData *connData, char *token, void **arg)
{
	if (token == NULL) return;

	char buf[8];
	os_strcpy(buf, "test");
	httpdSend(connData, buf, -1);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants