fix(session): Set last-password-confirm for token logins

[provokateurin]

Summary

This allows using app passwords for endpoints which require password confirmation. Unlike previous investigations suggested this fixes it regardless of the cookies.

For history: #43000

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[provokateurin]

/backport to stable28

[provokateurin]

/backport to stable27

[provokateurin]

/backport to stable26

[ChristophWurst]

Honestly not sure if this is what password confirmation protected endpoints were designed for.

Is there anything preventing session tokens to be used as app passwords to work around password confirmation?

[provokateurin]

If you only have session cookies then you are missing the Authorization header and the method will return false before running the added line.

[ChristophWurst]

I mean someone intentionally sending the session token as auth header to bypass password protection when they acquired a user session but don't know the password

[provokateurin]

Hm, I didn't know you could do that. @nickvergessen do you have an idea if this is a problem?

[ChristophWurst]

How does Github handle it with their sudo mode? Are operations protected by that accessible via API?

[provokateurin]

Using an app password is completely fine, since you can already workaround this limitation by using basic auth with the app password as the password (and if you don't know the username you can get it from an endpoint that doesn't require password auth.

[provokateurin]

We might want to move the cookie check before the token check and always early return if the cookie is set so the token login is never tried:

server/lib/base.php

Lines 1145 to 1153 in 5940161

	if ($userSession->tryTokenLogin($request)) {
	return true;
	}
	if (isset($_COOKIE['nc_username'])
	&& isset($_COOKIE['nc_token'])
	&& isset($_COOKIE['nc_session_id'])
	&& $userSession->loginWithCookie($_COOKIE['nc_username'], $_COOKIE['nc_token'], $_COOKIE['nc_session_id'])) {
	return true;
	}

[ChristophWurst]

https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/sudo-mode is what I'm referring to.

I am not sure if an app password or session token qualifies as enough proof for sensitive operations that require the user password in a web session.

[provokateurin]

I somewhat agree, but as I said it is already possible to workaround this limitation, even if you only have an app password.
Other tokens shouldn't be allowed though.

[ChristophWurst]

it is already possible to workaround this limitation

how? \OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware::beforeController doesn't check for app_password

[provokateurin]

I don't know, but I know it works because I've used it already many times.

[ChristophWurst]

I've tested using a new app token for bearer auth against a controller method protected with @PasswordConfirmationRequired and received

{"message":"Password confirmation is required"}

last-password-confirm is null

it is only bypassable on this branch, not on master.

[bigcat88]

I always thought this behavior was intended since we already get a lot of requests from clients who are afraid to use app_password.
This change will scare them even more.

Imagine the situation:

You create an application password for the file sync client and you are an administrator.
These changes will allow the application to send your application password somewhere, and the hacker will be able to create a new user with admin rights and covertly act on their behalf from another location.
Without support for API scopes, this change is dangerous.

Do you have any thoughts on this @nickvergessen as you already accepted it?

[provokateurin]

I see your point.
What I see as a failure is the differentiation between user and admin changes. There are a bunch of user settings which you are only allowed to change by confirming the password, but generally they are not harmful.
On the other side there are the admin endpoints that need password confirmation where this makes much more sense.
Maybe we should reconsider which endpoints really need password confirmation and which endpoints don't.

I was talking to @ChristophWurst the other day and brought up the idea again that it should just be possible to do the actual password confirmation when you only have an app password: #43000
This way it is still secured by asking the user to enter their password which matches the current behaviour on web.
So far that option seems to be more sensible as it will mimic the current state and just allow all clients to use password protected endpoints without opening up any potential security holes.

[nickvergessen]

I was talking to ChristophWurst the other day and brought up the idea again that it should just be possible to do the actual password confirmation when you only have an app password:

Yeah I think that makes sense. I missed somehow that you only have an app password and not the real password.

[provokateurin]

Closing in favor of #43000