Skip to content

Commit

Permalink
feat: only accept RFC-compliant End-of-DATA sequence
Browse files Browse the repository at this point in the history
Previously Postal was fairly forgiving about line endings. This requires that the end of data sequence is `<CR><LF>.<CR><LF>`.
  • Loading branch information
adamcooke committed Mar 1, 2024
1 parent a9ade3c commit 0140dc4
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 17 deletions.
19 changes: 16 additions & 3 deletions app/lib/smtp_server/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ class Client
def initialize(ip_address)
@logging_enabled = true
@ip_address = ip_address

@cr_present = false
@previous_cr_present = nil

if @ip_address
check_ip_address
@state = :welcome
Expand Down Expand Up @@ -51,6 +55,14 @@ def id
end

def handle(data)
if data[-1] == "\r"
@cr_present = true
data = data.chop # remove last character (\r)
else
Postal.logger.debug("\e[33m WARN: Detected line with invalid line ending (missing <CR>)\e[0m", id: id)
@cr_present = false
end

Postal.logger.tagged(id: id) do
if @state == :preauth
return proxy(data)
Expand All @@ -59,11 +71,12 @@ def handle(data)
log "\e[32m<= #{sanitize_input_for_log(data.strip)}\e[0m"
if @proc
@proc.call(data)

else
handle_command(data)
end
end
ensure
@previous_cr_present = @cr_present
end

def finished?
Expand Down Expand Up @@ -409,7 +422,7 @@ def data(_data)
@headers["received"] = [received_header]

handler = proc do |idata|
if idata == "."
if idata == "." && @cr_present && @previous_cr_present
@logging_enabled = true
@proc = nil
finished
Expand All @@ -424,7 +437,7 @@ def data(_data)
end

if @receiving_headers
if idata.blank?
if idata&.length&.zero?
@receiving_headers = false
elsif idata.to_s =~ /^\s/
# This is a continuation of a header
Expand Down
9 changes: 2 additions & 7 deletions app/lib/smtp_server/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -194,16 +194,11 @@ def run_event_loop
eof = true
end

# Normalize all \r\n and \n to \r\n, but ignore only \r.
# A \r\n may be split in 2 buffers (\n in one buffer and \r in the other)
buffers[io] = buffers[io].gsub(/\r/, "").encode(buffers[io].encoding, crlf_newline: true)

# We line buffer, so look to see if we have received a newline
# and keep doing so until all buffered lines have been processed.
while buffers[io].index("\r\n")
while buffers[io].index("\n")
# Extract the line
line, buffers[io] = buffers[io].split("\r\n", 2)

line, buffers[io] = buffers[io].split("\n", 2)
# Send the received line to the client object for processing
result = client.handle(line)
# If the client object returned some data, write it back to the client
Expand Down
40 changes: 33 additions & 7 deletions spec/lib/smtp_server/client/finished_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,32 @@ module SMTPServer
end

describe "when finished sending data" do
context "when the . character does not end with a <CR>" do
it "does nothing" do
allow(Postal::Config.smtp_server).to receive(:max_message_size).and_return(1)
client.handle("DATA")
client.handle("Subject: Hello")
client.handle("\r")
expect(client.handle(".")).to be nil
end
end

context "when the data before the . character does not end with a <CR>" do
it "does nothing" do
allow(Postal::Config.smtp_server).to receive(:max_message_size).and_return(1)
client.handle("DATA")
client.handle("Subject: Hello")
expect(client.handle(".\r")).to be nil
end
end

context "when the data is larger than the maximum message size" do
it "returns an error and resets the state" do
allow(Postal::Config.smtp_server).to receive(:max_message_size).and_return(1)
client.handle("DATA")
client.handle("a" * 1024 * 1024 * 10)
expect(client.handle(".")).to eq "552 Message too large (maximum size 1MB)"
client.handle("\r")
expect(client.handle(".\r")).to eq "552 Message too large (maximum size 1MB)"
end
end

Expand All @@ -43,7 +63,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "550 Loop detected"
client.handle("\r")
expect(client.handle(".\r")).to eq "550 Loop detected"
end
end

Expand All @@ -55,7 +76,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "530 From/Sender name is not valid"
client.handle("\r")
expect(client.handle(".\r")).to eq "530 From/Sender name is not valid"
end
end

Expand All @@ -71,7 +93,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "250 OK"
client.handle("\r")
expect(client.handle(".\r")).to eq "250 OK"
queued_message = QueuedMessage.first
expect(queued_message).to have_attributes(
domain: "example.com",
Expand Down Expand Up @@ -110,7 +133,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "250 OK"
client.handle("\r")
expect(client.handle(".\r")).to eq "250 OK"

queued_message = QueuedMessage.first
expect(queued_message).to have_attributes(
Expand Down Expand Up @@ -141,7 +165,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "250 OK"
client.handle("\r")
expect(client.handle(".\r")).to eq "250 OK"

queued_message = QueuedMessage.first
expect(queued_message).to have_attributes(
Expand Down Expand Up @@ -179,7 +204,8 @@ module SMTPServer
client.handle("To: #{rcpt_to}")
client.handle("")
client.handle("This is a test message")
expect(client.handle(".")).to eq "250 OK"
client.handle("\r")
expect(client.handle(".\r")).to eq "250 OK"

queued_message = QueuedMessage.first
expect(queued_message).to have_attributes(
Expand Down

0 comments on commit 0140dc4

Please sign in to comment.