On Thursday 12 June 2008 21:43, Peter Korsgaard wrote:
> From: Peter Korsgaard <[EMAIL PROTECTED]>
>
> 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 <[EMAIL PROTECTED]>
> ---
> 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
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
