Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smtp_bridge allows to use an unauthorized connection pool #3022

Closed
kayrus opened this issue Feb 12, 2022 · 0 comments · Fixed by #3113
Closed

smtp_bridge allows to use an unauthorized connection pool #3022

kayrus opened this issue Feb 12, 2022 · 0 comments · Fixed by #3113

Comments

@kayrus
Copy link

kayrus commented Feb 12, 2022

Describe the bug

Prerequisites: backend postfix server with multiple users + haraka configured as a reverse proxy with smtp_bridge plugin

According to smtp_bridge docs:

This plugins simply post the data from the original connection to the remote SMTP server using the original AUTH details.

However there is a combination of bugs:

Expected behavior

  • try_auth_proxy should correctly handle auth
  • a corresponding connection pool must be available only to a particular authenticated user

Observed behavior

When smtp_bridge plugin is used, there is a high probability that unauthorized user with any credentials can send emails on behalf of other authed haraka users.

Steps To Reproduce

  1. Run haraka with smtp_bridge plugin configured
  2. Set pool_concurrency_max=1 to increase the bug hit probability
  3. send an email with valid username/password
  4. send an email with any (valid/invalid/anything) credentials, the email will be sent on behalf of recent user

System Info:

Haraka Haraka.js — Version: 2.8.28
Node v10.24.1
OS Linux kay-hp 5.4.0-99-generic #112-Ubuntu SMP Thu Feb 3 13:50:55 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
openssl OpenSSL 1.1.1f 31 Mar 2020

Additional context

  1. smtp_bridge must use the same pool sharing concept as smtp_forward (per username)
  2. there should be less duplicate logic, like auth logic in smtp_client.js and in plugins/auth/auth_proxy.js
  3. this may not be all the problems

short fix at least disallows the successful attempt to send emails by unauthenticated users:

diff --git a/plugins/auth/auth_proxy.js b/plugins/auth/auth_proxy.js
--- a/plugins/auth/auth_proxy.js
+++ b/plugins/auth/auth_proxy.js
@@ -131,6 +131,7 @@ exports.try_auth_proxy = function (connection, hosts, user, passwd, cb) {
                     cert = self.config.get(self.tls_cfg.main.cert || 'tls_cert.pem', 'binary');
                     if (key && cert) {
                         this.on('secure', () => {
+                            if (secure) return;
                             secure = true;
                             socket.send_command('EHLO', connection.local.host);
                         });

But even applying this short fix still allows other authenticated users to send emails on behalf of another account, when multiple accounts on the backend have different strict rules (good example is AWS SES). I think bigger code refactoring is the best long term aim.

msimerson added a commit to msimerson/Haraka that referenced this issue Mar 30, 2022
auth_proxy: run "secured" only once, improvement for haraka#3022
@msimerson msimerson mentioned this issue Mar 30, 2022
3 tasks
msimerson added a commit to msimerson/Haraka that referenced this issue Apr 1, 2022
- auth_proxy: run "secured" only once, improvement for haraka#3022
msimerson added a commit that referenced this issue Apr 4, 2022
* clean up get_pool call signature
* smtp_client: pass args as object (was positional)
* smtp_client: run "secured" once, fixes #3020
- auth_proxy: run "secured" only once, improvement for #3022
* update outbound for generic-pool v4 too
* force a newer node-gyp version
- fixes #3017
* reduce windows testing to working version
@msimerson msimerson mentioned this issue Jun 4, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant