[PATCH] httpd: fix username verification with md5 auth

[Denys Vlasenko]

On Thursday 12 June 2008 21:43, Peter Korsgaard wrote:
> From: Peter Korsgaard <jacmet at sunsite.dk>
> 
> checkPerm only verified as many characters of the username as provided
> by the client, so E.G. an empty username would always match.
> 
> Cleanup and save a few bytes while we are at it:
> 
> function                                             old     new   delta
> checkPerm                                            359     350      -9
> ------------------------------------------------------------------------------
> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9)               Total: -9 bytes
> 
> Based on (incorrect) patch by Lubos Stanek (lubek) sent to the openwrt list:
> http://thread.gmane.org/gmane.comp.embedded.openwrt.devel/1464
> 
> Signed-off-by: Peter Korsgaard <jacmet at sunsite.dk>
> ---
>  networking/httpd.c |   28 +++++++++++++---------------
>  1 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/networking/httpd.c b/networking/httpd.c
> index 352a97d..db04cde 100644
> --- a/networking/httpd.c
> +++ b/networking/httpd.c
> @@ -1710,29 +1710,27 @@ static int checkPerm(const char *path, const char *request)
>  		if (strncmp(p0, path, l) == 0
>  		 && (l == 1 || path[l] == '/' || path[l] == '\0')
>  		) {
> -			char *u;
>  			/* path match found.  Check request */
>  			/* for check next /path:user:password */
>  			prev = p0;
> -			u = strchr(request, ':');
> -			if (u == NULL) {
> -				/* bad request, ':' required */
> -				break;
> -			}

I don't see what ensures ':' is always there.
The code we are called from is:
                        if (STRNCASECMP(iobuf, "Authorization:") == 0) {
                                /* We only allow Basic credentials.
                                 * It shows up as "Authorization: Basic <userid:password>" where
                                 * the userid:password is base64 encoded.
                                 */
                                tptr = skip_whitespace(iobuf + sizeof("Authorization:")-1);
                                if (STRNCASECMP(tptr, "Basic") != 0)
                                        continue;
                                tptr += sizeof("Basic")-1;
                                /* decodeBase64() skips whitespace itself */
                                decodeBase64(tptr);
                                credentials = checkPerm(urlcopy, tptr); <=======
                        }
and here user can supply bad <userid:password> without ':'.

>  
>  			if (ENABLE_FEATURE_HTTPD_AUTH_MD5) {
>  				char *pp;
>  
> -				if (strncmp(p, request, u - request) != 0) {
> -					/* user doesn't match */
> -					continue;
> -				}
>  				pp = strchr(p, ':');
>  				if (pp && pp[1] == '$' && pp[2] == '1'
> -				 && pp[3] == '$' && pp[4]
> -				) {
> -					char *encrypted = pw_encrypt(u+1, ++pp, 1);
> -					int r = strcmp(encrypted, pp);
> +				 && pp[3] == '$' && pp[4]) {
> +					char *encrypted;
> +					int r, len;
> +
> +					len = 1 + pp - p;
> +					if (strncmp(p, request, len) != 0) {
> +						/* user doesn't match */
> +						continue;
> +					}
> +
> +					encrypted = pw_encrypt(request+len, p+len, 1);
> +					r = strcmp(encrypted, p+len);

I spent some ten minutes understanding what is done here.

>  					free(encrypted);
>  					if (r == 0)
>  						goto set_remoteuser_var;   /* Ok */
> @@ -1743,7 +1741,7 @@ static int checkPerm(const char *path, const char *request)
>  
>  			if (strcmp(p, request) == 0) {
>   set_remoteuser_var:
> -				remoteuser = xstrndup(request, u - request);
> +				remoteuser = xstrndup(request, strchr(request, ':') - request);

Here where request without ':' will bite us.

BTW: any idea why this routine locks on first found path?
This code part:

                p0 = cur->before_colon;
                if (prev != NULL && strcmp(prev, p0) != 0)
                        continue;       /* find next identical */


Also: any idea what is going here in handle_incoming_and_exit()?

#if ENABLE_FEATURE_HTTPD_BASIC_AUTH
        int credentials = -1;  /* if not required this is Ok */
#endif
        ...
        ... (checks "Authorization: Basic <userid:password>" if present, sets credentials to 0/1)
        ...
#if ENABLE_FEATURE_HTTPD_BASIC_AUTH
        if (credentials <= 0 && checkPerm(urlcopy, ":") == 0) {
                send_headers_and_exit(HTTP_UNAUTHORIZED);
        }
#endif

So, if there were no "Authorization: Basic", we still check empty username/passwd.
ok. But the same will happen if "Authorization: Basic" was given with wrong passwd!
Should it be "if (credentials < 0..."?

I applied your patch, and then did heavy editing on top -
variable names, comments, etc in config file parsing were atrocious.

Can you test current svn?

Thanks!
--
vda