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 encodeBase64 for empty password or user in HTTP Basic Authentication #4326

Merged
merged 4 commits into from
Jan 6, 2024

Conversation

Saibamen
Copy link
Contributor

@Saibamen Saibamen commented Jan 5, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fix basic authorization for servers with empty password or empty user.

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Proof of empty user auth with Postman

No auth - error 401

image

Auth with empty user - 200 OK

image

@CommanderStorm CommanderStorm added the area:monitor Everything related to monitors label Jan 5, 2024
@CommanderStorm
Copy link
Collaborator

I cannot reproduce (via https://webhook.site/) the current system not working.
(Also your reproduction steps in the description are using postman, not our system):

  • YWRtaW46 -> admin:
    image

  • YWRtaW46 -> :admin
    image

@louislam
Copy link
Owner

louislam commented Jan 5, 2024

Maybe off topic and maybe an edge case. If user contains a colon :, it properly will not be working.

@Saibamen
Copy link
Contributor Author

Saibamen commented Jan 5, 2024

Commited a simplified version:

Saibamen@dbf4929

@Saibamen
Copy link
Contributor Author

Saibamen commented Jan 5, 2024

Before changes:

image

After changes:

image

Please reopen this bugfix.

HTTP Auth was set up for C# Server (Kestrel):

public async Task InvokeAsync(HttpContext context)
{
    // Make sure we are hitting the swagger path, and not doing it locally as it just gets annoying :-)
    if (context.Request.Path.StartsWithSegments("/swagger") && !IsLocalRequest(context))
    {
        string authHeader = context.Request.Headers["Authorization"];
        if (authHeader != null && authHeader.StartsWith("Basic "))
        {
            // Get the encoded username and password
            var encodedUsernamePassword = authHeader.Split(' ', 2, StringSplitOptions.RemoveEmptyEntries)[1]?.Trim();

            // Decode from Base64 to string
            var decodedUsernamePassword = Encoding.UTF8.GetString(Convert.FromBase64String(encodedUsernamePassword));

            // Split username and password
            var username = decodedUsernamePassword.Split(':', 2)[0];
            var password = decodedUsernamePassword.Split(':', 2)[1];

            if (IsAuthorized(username, password))
            {
                await _next.Invoke(context);
                return;
            }
        }

        // Return authentication type (causes browser to show login dialog)
        context.Response.Headers["WWW-Authenticate"] = "Basic";

        // Return unauthorized
        context.Response.StatusCode = (int)HttpStatusCode.Unauthorized;
    }
    else
    {
        await _next.Invoke(context);
    }
}

public bool IsAuthorized(string username, string password)
{
    // Check that username and password are correct
    return username.Equals("", StringComparison.InvariantCultureIgnoreCase) && password.Equals("xxxxxxxxxxxxxxxxxx");
}

@louislam louislam reopened this Jan 5, 2024
@louislam
Copy link
Owner

louislam commented Jan 5, 2024

Is it because the user is undefined, so it becomes undefined:yourPassword?

@Saibamen
Copy link
Contributor Author

Saibamen commented Jan 5, 2024

null as string:
image

@CommanderStorm
Copy link
Collaborator

The reason, why I could not reproduce this is that I tested with set pw/username first.

Initially these values are null
image

but after edting (adding+removing a char) these values become ""
image

@CommanderStorm
Copy link
Collaborator

Please include a type hint here
https://github.com/louislam/uptime-kuma/pull/4326/files#diff-0c4874f149ad01732ad730f36c95c99e37f5c3f13e9b52f628e2e284daa72d27R232

like

/**
 * Encode user and password to Base64 encoding
 * for HTTP "basic" auth, as per RFC-7617
 * @param {string|null} user - The username (nullable if not changed by a user)
 * @param {string|null} pass - The password (nullable if not changed by a user)
 * @returns {string}
 */

@CommanderStorm
Copy link
Collaborator

@Saibamen

Note

This function in your code is not resistent to timing attacks

public bool IsAuthorized(string username, string password)
{
    // Check that username and password are correct
    return username.Equals("", StringComparison.InvariantCultureIgnoreCase) && password.Equals("xxxxxxxxxxxxxxxxxx");
}

See CWE-385 for further details: https://cwe.mitre.org/data/definitions/385.html

@louislam louislam merged commit 458cdf9 into louislam:1.23.X Jan 6, 2024
14 checks passed
@louislam louislam added this to the 1.23.12 milestone Jan 6, 2024
@Saibamen Saibamen deleted the fix_encodeBase64 branch January 6, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants