Empty cookie security risk bug fix 610

[ShahanaFarooqui]

Empty cookie security risk bug fix 610

[Kixunil]

Unfortunately, this fix is insufficient.

• It could happen that there's only few bytes of space available or writing fails midway for some other reason. This would still lead to token being dangerously short and trivial to brute-force. At least 16 bytes of entropy should be required, which means 32 hex chars. (so it should be common.cookie.trim().length >= 32)
• I believe implementing atomic writes would still be useful to avoid problems.
• You missed some additional concerns that I shared with @saubyk privately. You don't have a Keybase account nor publicly known PGP key, so I couldn't send a copy to you. Please ask @saubyk for details privately or ask me using a secure communications channel. Keybase is really good for this, I believe - please pair your GitHub account.

[ShahanaFarooqui]

Unfortunately, this fix is insufficient.

• It could happen that there's only few bytes of space available or writing fails midway for some other reason. This would still lead to token being dangerously short and trivial to brute-force. At least 16 bytes of entropy should be required, which means 32 hex chars. (so it should be common.cookie.trim().length >= 32)

[Shahana]: If the writting fails midway or fails completly, the RTL server will throw an error and will not start. Still, we added another check for minimum length of the cookie as 32 characters (PR #648).

• I believe implementing atomic writes would still be useful to avoid problems.

[Shahana]: We believe that atomic writing is an overkill in this case. Specially, when SSO is only encouraged through a secure third party application. We can revisit the option in future if absolutely needed.

• You missed some additional concerns that I shared with @saubyk privately. You don't have a Keybase account nor publicly known PGP key, so I couldn't send a copy to you. Please ask @saubyk for details privately or ask me using a secure communications channel. Keybase is really good for this, I believe - please pair your GitHub account.

[Shahana]: I discussed the issue with @saubyk and implemented the suggestion you provided in the private chat with him (PR #648). Regarding PGP keys, sharing any suggestion/concerns with @saubyk is my prefered way and would like to continue with that for now.

Thanks once again for the issue and suggestions.

[Kixunil]

If the writting fails midway or fails completly, the RTL server will throw an error and will not start.

Note that this was the reason why the issue occurred in the first place - RTL failed but subsequently it read the cookie file because it was auto-restarted by systemd.

Anyway, I believe/hope that this fix is sufficient now.

We believe that atomic writing is an overkill in this case

Yeah, maybe it is. The cost is just one more syscall and a few more lines of code so that was my reason behind it. I ran version of RTL from my PR for a while and no problem ever occurred.

Specially, when SSO is only encouraged through a secure third party application.

Yeah, I believe this is not related to the topic. Or am I missing something important?

Thanks once again for the issue and suggestions.

You are welcome, I'm happy I have helped!