Revamp public shares authentication model

[ishank011]

Why?

With the current workflow with which public shares are handled, we run the risk of exposing the resource owner's token to random users who access the shared link. Using only the token corresponding to the public shares, the Authenticate gateway method can be called, which will return the owner's token.

reva/internal/http/services/owncloud/ocdav/dav.go

Lines 177 to 183 in 3b7e1f0

	authenticateRequest := gatewayv1beta1.AuthenticateRequest{
	Type: "publicshares",
	ClientId: token,
	ClientSecret: pass,
	}
	res, err := c.Authenticate(r.Context(), &authenticateRequest)

Anonymous users

I propose using anonymous users to grant share-specific access. When a public share is created, a temporary user is created and assigned the appropriate grant for the shared file.

• For a share corresponding to #/s/RMxHPGhqbRySVOH, we add an ACL u:public_RMxHPGhqbRySVOH:rwx.
• When this link is accessed, the Authenticate method of type publicshares returns this user if it's able to find the share corresponding to it.
• This user is set in the context, and the file systems correctly handle the requests as the ACLs are already set.

Possible issues

• Users can misuse these anonymous users like they can in the current implementation. While they won't be able to access actual files as they would be protected by ACLs, the returned token can still be used to perform other operations (manipulating shares, accessing info about other users). This would have to be handled in the HTTP and GRPC auth interceptors with checks that these anonymous users should only be allowed access to the /remote.php/dav/public-files/ endpoint and those exposed by the public storage provider service.
• For file systems which use local extended attributes, adding ACLs corresponding to non-existent users should be trivial. However, for EOS, this won't work as EOS expects the UID to actually exist when performing operations. Hence, it would have to be handled separately. While setting arbitrary ACLs does work, to actually access the resource, we'll need to impersonate the owner.
• For every public share created, we'll need to add an ACL. This does not seem to be too major an issue, as for one particular resource, we shouldn't expect more than ~10 links to be created. Especially when we have the janitor cleaning up expired links, we can remove the ACLs at the same time.

Please let me know your thoughts about this @labkode @butonic @refs @dragotin

[butonic]

cc @C0rby

[C0rby]

I agree the the public shares authentication model needs to be revamped.
Is there no other way than to create users for the share? I'm not too familiar with the authentication model yet.

[C0rby]

While I was implementing the signed URL public links I got some insights and see some more problems now.

First I think we need to revert this API change cs3org/cs3apis#113.
The reason will hopefully be clear by the end of my comment but in short, by adding the password hash to the response we are enabling attacker to do offline brute-forcing on the share password hash. I somehow didn't think about this implication.

The process to authenticate a signed URL request roughly looks like this:

1. Take the signature and expiration from the requested URL
2. Get the share (including share password hash)
3. Calculate the signature using token, share password hash, expiration from request
4. Compare the signature from the request with the calculated one
5. If they match the request is authenticated

But step 2 is already problematic. With basic authentication we could just call GetPublicShareByToken and provide the share token and password. Pre-signed requests don't provide the password so we can't use that method. We would need to use GetPublicShare instead but currently this method requires an authenticated context. If we add that method to the unauthenticated methods then we enable everyone to request any public share as long as they know or guess the share token. They wouldn't be able to access the files yet but now with the changed API they would be able to brute-force to share password. Maybe not fast but that depends on the computing capabilities of the attacker.

What we would need instead is an 'internal' way to get the share + share password hash without an authenticated context.
That would only work if the webdav related code had direct access to the public share manager. Then the public cs3 API can lock down the access like before.

Another possible way would be if the webdav code (where the public share signing stuff would be implemented) could store data in a key/value store. Then we could just generate a signingkey per public share which is stored in this key value store and use this to sign and validate the signatures.
The only thing we have to solve here is to how to access the share then because we still don't have the share password to be able to call GetPublicShareByToken.

But I guess this also depends on how we revamp the public share authentication model.

[C0rby]

Users can misuse these anonymous users like they can in the current implementation. While they won't be able to access actual files as they would be protected by ACLs, [...] that these anonymous users should only be allowed access to the /remote.php/dav/public-files/ endpoint and those exposed by the public storage provider service.

But then they would be able to access the shared files, right? I mean wouldn't it be possible to set put this user into the request context and issue a InitiateDowload request or some other request for files in that share?

[C0rby]

Nevermind. With the share token and password it's of course possible to access the share files... 🤦

[labkode]

@C0rby for the brute-force attack a very simple solution is to add a delay on wrong password (like when you wrongly type your password when sudo-ing)

[ishank011]

Take the signature and expiration from the requested URL

Isn't expiration also part of the hashed signature, as per this cs3org/cs3apis#110? How do you retrieve that?

But I guess this also depends on how we revamp the public share authentication model.

What I propose above won't solve this. That is just to prevent exposing the creator's token to random users and is not really related to how we want to design the signed URLs

by adding the password hash to the response we are enabling attacker to do offline brute-forcing on the share password hash

I don't really see the problem here. Do you mean that if a user knows the password to a share, and if he accesses GetPublicShareByToken, he will get the hashed password, and so he can figure out the salt? Isn't that true for our whole jwt token design?

[ishank011]

A better way IMO, instead of having the public share service return the password would be to delegate the whole signing process to it. Add a method called GetShareBySignature which takes the 256 bit signature and returns the share. Although, this won't be the minimal approach which we want to adopt for CS3APIs @labkode

[ishank011]

Or better yet, make the password parameter in GetPublicShareByToken a oneof between password and signature.

[labkode]

@ishank011 if the logic is to just "sign" that can be added to the CS3APIS. If the logic is "craft a signature that allows fine-grained permissions to download this file" I think that doesn't fit there.

[labkode]

In the old Reva implementation (pre-CS3APIs) we had a dedicated module to craft tokens:
https://github.com/cernbox/revaold/blob/master/api/token_manager_jwt/token_manager_jwt.go

And it did created "tokens" for public links authentication:
https://github.com/cernbox/revaold/blob/master/api/token_manager_jwt/token_manager_jwt.go#L85

[ishank011]

@labkode if we follow this approach

Or better yet, make the password parameter in GetPublicShareByToken a oneof between password and signature.

then we just need to add a signature field to the PublicShare message which would be returned to the webdav service which can expose it.

[C0rby]

@C0rby for the brute-force attack a very simple solution is to add a delay on wrong password (like when you wrongly type your password when sudo-ing)

That doesn't work. If someone has the hash they can just calculate hashes and compare them with the share hash.

[C0rby]

Take the signature and expiration from the requested URL

Isn't expiration also part of the hashed signature, as per this cs3org/cs3apis#110? How do you retrieve that?

The expiration is included in the signing process. When someone is using the signed URL then the expiration needs to be included in the request in a query parameter.

by adding the password hash to the response we are enabling attacker to do offline brute-forcing on the share password hash

I don't really see the problem here. Do you mean that if a user knows the password to a share, and if he accesses GetPublicShareByToken, he will get the hashed password, and so he can figure out the salt? Isn't that true for our whole jwt token design?

No I mean currently the share can only be accessed by doing a GetPublicShareByToken request which includes the share token and password. If an attacker wants to bruteforce the password they have to repeatedly make that request.
To prevent bruteforcing the service could have some sort of cooldown/delay like @labkode above.

But if the attacker has the hash then the only limit to bruteforcing is the computational complexity of the hash and the computing power of the attacker.

[C0rby]

So based of your comments I propose these changes cs3org/cs3apis#118

[ishank011]

@butonic @refs @C0rby @labkode any comments on the design I proposed? Should I start with the implementation?

[refs]

👍 conceptually it makes sense. I'm not aware of the side effects compared to other approaches, such as using a dedicated "maintenance" user only for public shares.

On a performance note, I wonder how much of a toll for a system would be due to the overhead of looking for a given user (not getting into indices here 🙈). Wondering as well if such users have to be filtered out from the UI, but these are implementation details. The concept is clear to me.

My only remarks are:

• Lack of comparison (YMMV) with a production environment, i.e: there is an average of 2 public links per user, on a deployment with 100.000 users there would potentially be 200.000 extra users (these are totally made up numbers)
• Filter out public users from the UI (sounds trivial, but still to keep it in mind?)
• Reverse engineer-able: the username generation composed of public_publicSharetoken is guessable.

[ishank011]

@refs thanks a lot for the comments!

Sorry, I didn't mention this in the explanation. We won't actually be creating any users. These would just be used to generate the jwt token. So when the publicshare.Authenticate method will be called,

reva/pkg/auth/manager/publicshares/publicshares.go

Lines 121 to 128 in e8a00d9

	getUserResponse, err := gwConn.GetUser(ctx, &userprovider.GetUserRequest{
	UserId: publicShareResponse.GetShare().GetCreator(),
	})
	if err != nil {
	return nil, err
	}
	return getUserResponse.GetUser(), nil

instead of returning the creator of the share, we'll return

&userpb.User{
  Id: &userpb.UserId{
    OpaqueId: public_RMxHPGhqbRySVOH,
  },
}

The middleware will generate a token for this user which will be used to allow access to the shared file. When dismantling the token, we'll check that it's a special public-shares-only user and make sure that it has access only to the /remote.php/dav/public-files/ endpoint and those exposed by the public storage provider service.

using a dedicated "maintenance" user only for public shares

This is also an option, but the ACLs on all the shared resources will have the same principal then. So an attacker might use the token to access all such files.

Reverse engineer-able: the username generation composed of public_publicSharetoken is guessable.

True but the token won't be of much use as the middleware will block access for these special users. They'll only be able to access the file shared with them (which they can anyway since they have the token).

[refs]

Ace! thanks @ishank011 for taking the time to go through the point :) all clear

[ishank011]

This approach has been superseded by #1669