CCG Flow Insecure (any client password accepted)

[gerneio]

This stems back to #104, which was ultimately an explanation for how the interactive delegated flow was indeed secure.

So I understand the inherent security with using "any" email client password when using a delegated flow, since that requires immediate user interaction to complete the auth process, and therefore the resulting tokens are encrypted with the original email client password sent, so subsequent requests would need to use the same password to avoid a user interactive re-authentication. So that inherently keeps the delegated flow secure, HOWEVER if using client credential grant flow, that is simply not the case. With CCG, if I send a different password, sure the error retrying login due to exception while decrypting OAuth 2.0 credentials will be thrown, but the program will attempt to re-authenticate and generate new access tokens automatically, and since CCG doesn't require any user interaction at all and relies on the client/app id, tenant id, and client secret already stored in config, the new tokens are generated and stored w/o complaint.

This effectively allows a CCG flow to be used as an open relay for any system that can reach it with any specified password, as long as they specify the correct set of usernames.

One solution might be to allow specifying some sort of "app password" that we can add in config somehow to allow only certain email client passwords to be used. I'd imagine that adds some complexity and security risks since the same password would be used to encrypt the tokens in the same file later on, so more thought and consideration would need to be given for how to resolve that properly.

Another solution might be preventing CCG flows from automatically re-generating encryption keys based on current password when decryption fails, and maybe force manual user intervention to modify the config file in place to wipe out the bad keys (i.e. a way to get some user interaction when things go wrong).

Just some food for thought, but I do think this is a pretty serious issue that makes using the CCG flow insecure, especially if you're running other third party software on your server which could potentially exploit this (or perhaps they themselves were exploited), or god forbid you open this up on the public internet 😨...

Let me know if I'm just blatantly missing an easy way to make this secure. Since Microsoft's support for SMTP OAUTH2 w/ CCG flow was finally released earlier this year (ref this & that), setting up some sort of proxy like this would be pretty critical for finally retrofitting older SMTP clients that have no other ways of supporting this.

Thanks for the work you have put into this library! For the most part (minus some docker networking issues I'm investigating and this potential security flaw issue) it worked pretty well right out of the gate.

[gerneio]

Per your comment in #213, you're right, I completely overlooked the encrypt_client_secret_on_first_use config option. I misread it as something completely different.

Using this feature (with the error loop fix), as well as utilizing your pre-encryption script, are great substitutes for making the CCG flow secure.

I suppose my only complaint would be that CCG is insecure by default, at least based on the default settings and example config file. So first time users (such as myself) might leave it unsecure in risky situations on their first installs.

encrypt_client_secret_on_first_use also doesn't play too well with catch all accounts, as you elude to in the documentation. I understand the limitations of using the same password (which makes total sense), so assuming that is always the case, there's still a problem where the client_secret config is still left unencrypted under the catch all account config (after first use). I tried adding the output of the pre-encryption script under the catch all account config, but then it runs into decryption errors. I suspect it's because the Cryptographer class doesn't take into account the existence of catch all accounts when getting the token_salt or token_iterations (compare here vs here).

However, using just single account configs, does seem to work quite well, so thanks for pointing out that config option!

[simonrob]

Great – glad you were able to resolve this. And yes, unfortunately, CCG is simply insecure by default without any preparation step (such as the encryption script) – it's an admin-type method that intentionally gives access to accounts with little interaction required. I don't think the proxy could really do much more here to secure this flow. But I do think anyone using the CCG flow is likely to be a relatively advanced user, and the proxy's readme is also pretty clear about the potential insecurity. I'll look at strengthening that warning, though.

Re: catch-all accounts and secret encryption – this is already on my list to fix. The intent is as outlined in the readme: this usage should be supported, but only when using the same password (which is potentially likely given the typical use of CCG).

It's also worth explaining why the proxy purposefully doesn't remove the unencrypted secret from any top-level inherited configuration. The reason is that you might be using this intentionally at first setup time – for example: you have a large number of accounts to configure via CCG, and you'd like each one to have a different password. After logging in once to each account, they would have their own encrypted versions of the secret, and when you've finished you can manually remove the unencrypted secret from the top-level section, which will prevent any new accounts from being created.

If, on the other hand, you do want to use the same password for all accounts, once you've logged into an account, just replace the unencrypted secret at the top level with the encrypted one (and, once I've made the fix mentioned above, also the token_salt and token_iterations values) from an individual account, and this will be used for any new accounts. Note: it'll also be duplicated in the account itself, so if at any point you want to prevent the creation of new accounts you can remove it from the top level section (but keep token_salt and token_iterations at the top level).

[gerneio]

All makes sense to me. Thanks for the explanation!

To make CCG flow secure by default, the only quick fix I can think of would be to maybe change encrypt_client_secret_on_first_use to be true by default and update readme/docs accordingly. Of course, that could be a breaking change for some when updating to the latest version. Having a changeme file which lists breaking changes per version would be helpful here.

Another solution (suggested in OP), but probably a bit more code changes required would be to only allow token generation once for a given account w/ CCG flow. How exactly, while allowing expired tokens to be re-generated, not 100% sure, but maybe by enforcing that the same token_salt must be used (once initially generated). I noticed that if I disable encrypt_client_secret_on_first_use (i.e. restore to default settings), then if I submit two different requests with different passwords, the two requests will have different token_salts. If I instead, use the same password, but force the access_token_expiry to be outdated, the access_token is regenerated, but the token_salt remains the same. So that is why I think we could probably try to enforce the same token_salt usage. I also don't think this would cause too many breaking changes as compared to the above. Anyway, just food for thought...

Either way, I think more direct warnings as you suggested would go a long way too, although not sure what the correct verbiage sweet spot would be. Perhaps at minimum a blurb that starts with ⚠️WARNING⚠️ in all caps 🤷 (since I completely missed it).

[simonrob]

It's worth remembering that none of this is really an issue unless you're using the proxy in a shared environment, or on a publicly accessible server. If that's the case, then personally I think I'd advise against the more powerful methods entirely, and stick to normal per-user authentication.

The token_salt is not relevant – it's created afresh whenever a new password is used, and is used from that point forward as part of the encryption/decryption process. I also wouldn't want to change the default for encrypt_client_secret_on_first_use because that would affect all usage of the proxy, and would be very disruptive outside of the CCG case.

What actually may be worth considering is the default value for delete_account_token_on_password_error in the CCG situation. The normal default is True because of issues with clients storing IMAP and SMTP passwords separately, and people being unaware of this, then finding themselves seemingly (but not actually) locked out of their account. I don't want to change that behaviour because it led to lots of unnecessary issues that I don't want to restart. However, if this value was changed so that it was ignored when using the CCG method, it would mean that once the account had been authenticated, only the correct password would work; other attempts would be rejected (rather than resetting the account's credentials and allowing any password to be used for the next login).

This wouldn't help if you're using catch-all accounts in a situation where the client secret that's being used has access to many accounts, some of which are not set up with the proxy – new accounts are always going to be allowed in catch-all mode (that's the whole point!). So again it's a trade-off. But probably this change in conjunction with a bit more highlighting in the readme might improve things in general.

[simonrob]

Commit 8b2c0ad improves the CCG documentation and also adds the new restriction on removal of account access tokens that I outlined above.

In the process of investigating this, I discovered a flaw with the existing CCG implementation that, in specific circumstances, could allow the situation you outlined even when accounts had been created already. In the interest of full responsible disclosure, I released a new version of the proxy (2023-12-19) and also published a security advisory just in case.

Thanks for the discussion about this.