Etherpad password only secured pads stores the specific pad password in plain-text cookies

[jaseg]

From a security point of view this is bad. A cookie should not contain more than a session id or a (limited-time) token. The current behavior e.g. leaks an user's password with each XSS flaw.

[JohnMcLear]

We think this is something about avoid being prompted on second visit to a pad, the best solution is to use sessions instead so we should do that..

We did it this way initially because we didn't have support for sessions..

[JohnMcLear]

@eldiddio you might want to look into this one because it affects you directly. To replicate create a new pad on primarypad, click to set a password.

Visit the pad, type in that password, look at your cookies.

[JohnMcLear]

Steps to replicate (tm):

Change your APIKEY.txt to EtherpadFTW and restart Etherpad.

All URLS should be visited

• Unsure if required: http://hostname:9001/api/1/createGroup?apikey=EtherpadFTW
• Unsure if required: http://hostname:9001/api/1/createAuthorIfNotExistsFor?apikey=EtherpadFTW&name=yolo
• Unsure if required: http://hostname:9001/api/1/createSession?apikey=EtherpadFTW&groupID=%%%%REPLACE%%%%&authorID=%%%%REPLACE%%%%&validUntil=1561973004
  • Note that the validUntil is set here, if this is in the past for you, change it.

In Javascript Console
document.cookie = "sessionID = %%%%REPLACE%%%%"; // replace with sessionID from above

• http://hostname:9001/api/1/createGroupPad?apikey=EtherpadFTW&groupID=%%%%REPLACE%%%%&padName=testjohn&text=test%20text
• http://hostname:9001/api/1/setPublicStatus?apikey=EtherpadFTW&padID=%%%%REPLACE%%%%$testjohn&publicStatus=true
• http://hostname:9001/api/1/setPassword?apikey=EtherpadFTW&padID=%%%%REPLACE%%%%$testjohn&password=test
• http://hostname:9001/p/g.E2DsuKXqDIDY9n94$testjohn
  • Enter the password
  • Hit F12, click resources, look at your cookies, you will see a cookie called "password" with the password stored in plain text.

[JohnMcLear]

The reason we store the password in plain text is so we can check to see if the password has changed.

To resolve this I will use our sessionStore and destroy any stored sessions for a pad(except the pad owner) when a password is changed, that will force the user to reauth to the new password ergo creating a new session :)

[JohnMcLear]

So I thought about this and tbh I think it will get addressed better in #1211 when we rewrite auth. Our current model is kinda broken and this is pretty edge case. Agreed it's important but we really need to address auth in general cc @Pita

[JohnMcLear]

I introduced Salting a while back so this could be used to encrypt the password. I'm not sure why I didn't propose that when I introduced salting, but yeah, basically tell the server to salt the password based on the sessionKey from settings.json.. Simples!

[JohnMcLear]

bump @eldiddio

[JohnMcLear]

Another one to ensure @rhelmer is aware of.

[JohnMcLear]

FWIW you shouldn't use pad level passwords, your access should be done by session management. That's why this hasn't been actioned..

[JedMeister]

@muxator - sorry to be a pain, but browsing the issue tracker I found this one which IMO should be tagged security. FWIW I haven't tested to confirm it's still valid, so perhaps it's been addressed already and just needs to be closed?

[muxator]

Yeah, needs confirmation. Labeling for now.

[JohnMcLear]

The logic is still in to set a password and to ask for a password and you set a password using the API.

I'm +1 just dropping the "setPassword" API because we shouldn't be using anything but a mature authentication method for access to pads.

@muxator thoughts on dropping setPassword in the API then we can close this off?

Lots of other plugins / alternatives exist for access control so I'm totally fine with that..

[muxator]

Preamble: I still did not took the time to replicate this. I may be wrong.

@muxator thoughts on dropping setPassword in the API then we can close this off?

Two alternatives. I am 55% for number 1.

1. That's a breaking change for sure. In general I would be against it.

   If something is out in the wild and used this (borked?) functionality, we should try to un-bork it.
   From what I read in the thread a way must exist. It's about using an ephemeral session instead of a plaintext cookie after all...

2. if we really decide to drop this off it is because it's less work. It may make sense because we don't have such a big user base after all.
   At that point I ask: is removing the API endpoint the only thing we should do? Would exist any other redundant logic at that point that should be removed, too? If we want to remove something, let's get rid of everything related.

[JohnMcLear]

+1 #2

I doubt anyone is using the setPassword. 99% of consuming API will be creating sessions and handling auth off pad. I am not worried about just ripping it out, it could be a plugin if people want it?

+1 remove other redundant conversations.

[JohnMcLear]

I'm gonna document what needs doing and then make a bounty as I won't have time to complete this work before I sign off.

[JohnMcLear]

Well that was easy, took 5mins. #4178