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

YOURLS reads login credentials from GET parameters #2424

Open
dgw opened this Issue Aug 25, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@dgw
Collaborator

dgw commented Aug 25, 2018

This is a follow-up to #2422, which centered around broken login if username and password are passed in the URL via GET parameters (https://your.ls/admin/?username=foo&password=barbaz).

I suggested it should either be fixed, so what that issue's OP wanted to do would work correctly, or removed entirely so the login credentials are ignored if passed in the URL. The consensus is that YOURLS is doing something it shouldn't—the login form fields are meant to be POSTed, and reading them from GET requests is a bug for security reasons.

For the moment I am self-assigning this, perhaps to take a look at it this weekend. (I've already had a cursory look at the code in functions-auth.php and found the "sloppy coding" @ozh mentioned… 😹) Milestone 1.8 because, well, it's not coming out any time soon anyway. 😛

But in all seriousness, this does need fixing. We don't want webservers that handle YOURLS requests storing passwords in their request logs, do we?

Edit: I got issue 2424… I feel special now.

@dgw dgw added core security labels Aug 25, 2018

@dgw dgw added this to the 1.8 milestone Aug 25, 2018

@dgw dgw self-assigned this Aug 25, 2018

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Aug 26, 2018

Member

Neat, thanks

I think it all boils down to https://github.com/YOURLS/YOURLS/blob/master/includes/functions-auth.php#L52-L59 where we're just checking on $_REQUEST which matches either POST or GET. I think it's fine for API auth but we should restrict to $_POST for web auth.

So, this "elseif API or normal: login with username & pwd" block should in fact be split in two, "elseif API login with username & pwd then check REQUEST, elseif web login using username & password: check POST"

Member

ozh commented Aug 26, 2018

Neat, thanks

I think it all boils down to https://github.com/YOURLS/YOURLS/blob/master/includes/functions-auth.php#L52-L59 where we're just checking on $_REQUEST which matches either POST or GET. I think it's fine for API auth but we should restrict to $_POST for web auth.

So, this "elseif API or normal: login with username & pwd" block should in fact be split in two, "elseif API login with username & pwd then check REQUEST, elseif web login using username & password: check POST"

@PopVeKind

This comment has been minimized.

Show comment
Hide comment
@PopVeKind

PopVeKind Aug 28, 2018

Contributor

Ah, the old $_REQUEST problem. Most "Best Practices" suggest "Do NOT Use $_REQUEST" as it opens a gate for security issues. At best $_REQUEST invites sloppy(er) coding. Can we add a YOURLS Coding Standard?


YOURLS Coding Standard

Do NOT use $_REQUEST
Instead, use any of these (as appropriate):

  • $_GET
  • $_POST
  • $_COOKIE

This will prevent a $_GET in the URL from inadvertently becoming an injected $_POST or $_COOKIE. This should work well in the API / POST split Ozh suggested. The nuance value returned in most YOURLS posts may be useful as a trigger also, just something to consider.

Contributor

PopVeKind commented Aug 28, 2018

Ah, the old $_REQUEST problem. Most "Best Practices" suggest "Do NOT Use $_REQUEST" as it opens a gate for security issues. At best $_REQUEST invites sloppy(er) coding. Can we add a YOURLS Coding Standard?


YOURLS Coding Standard

Do NOT use $_REQUEST
Instead, use any of these (as appropriate):

  • $_GET
  • $_POST
  • $_COOKIE

This will prevent a $_GET in the URL from inadvertently becoming an injected $_POST or $_COOKIE. This should work well in the API / POST split Ozh suggested. The nuance value returned in most YOURLS posts may be useful as a trigger also, just something to consider.

@dgw

This comment has been minimized.

Show comment
Hide comment
@dgw

dgw Aug 28, 2018

Collaborator

Old indeed. A quick check of line blame from my phone just now tells me that code has been there for probably 9 years or more.

Definitely going to split up the checks as @ozh suggested when I get a chance.

Collaborator

dgw commented Aug 28, 2018

Old indeed. A quick check of line blame from my phone just now tells me that code has been there for probably 9 years or more.

Definitely going to split up the checks as @ozh suggested when I get a chance.

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