From 7e2904763170597795d8c117d938cb6e4b86b3b7 Mon Sep 17 00:00:00 2001 From: MegaBrutal Date: Thu, 12 Feb 2015 00:04:50 +0100 Subject: [PATCH] (Relay) Handle unexpected replies and disconnections adequately - Previously it was possible that MgSMTP erroneously administered an e-mail as delivered, when the server accepted the recipient in the RCPT stage, but disconnected before or in the DATA stage. This patch mainly addresses this issue, by making sure that the transaction is completed, before administering a final status. - MgSMTP is now more tolerant with non-compliant replies, e.g., accepts 250 for connection opening, not only 220. - Increased relay send buffer size from 32 lines to 64. modified: Common.pas modified: MgSMTP.pas modified: Relay.pas modified: Spool.pas --- Common.pas | 15 ++++++++++++--- MgSMTP.pas | 2 +- Relay.pas | 47 ++++++++++++++++++++++++++++++++++++++--------- Spool.pas | 29 +++++++++++++++++------------ 4 files changed, 68 insertions(+), 25 deletions(-) diff --git a/Common.pas b/Common.pas index 8e0b7fb..7692672 100644 --- a/Common.pas +++ b/Common.pas @@ -138,6 +138,7 @@ type procedure AddRecipient(Recipient: TRecipient); overload; procedure SetReturnPath(Address: string); procedure SetRecipientData(Index, Data: integer; RMsg: string = ''); + procedure SetAllRecipientData(Data: integer; RMsg: string = ''); procedure SetRelayHost(HostName: string); property ReturnPath: string read FReturnPath write SetReturnPath; property RelayHost: string read FRelayHost write SetRelayHost; @@ -154,7 +155,7 @@ type function EMailTimeStamp(DateTime: TDateTime): string; function EMailTimeStampCorrected(DateTime: TDateTime): string; function StatusToStr(Status: integer): string; - procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope); + procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope; TransactionComplete: boolean); function CleanEOLN(S: string): string; function GenerateRandomString(Length: integer): string; @@ -303,7 +304,7 @@ begin + '+' + IntToStr(Status and DS_SMTPREPLYMASK); end; -procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope); +procedure AssignDeliveryStatusToSMTPCodes(Envelope: TEnvelope; TransactionComplete: boolean); var i, code, cond, status: integer; Recipient: TRecipient; begin for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin @@ -312,7 +313,8 @@ begin cond:= code div 100; case cond of 0: status:= DS_DELAYED or DS_UNEXPECTEDFAIL; - 2: status:= DS_DELIVERED; + 2: if TransactionComplete then status:= DS_DELIVERED + else status:= DS_DELAYED or DS_UNEXPECTEDFAIL; 4: status:= DS_DELAYED; 5: status:= DS_PERMANENT; else status:= DS_PERMANENT or DS_UNEXPECTEDFAIL; @@ -658,6 +660,13 @@ begin FRecipients[Index].Data:= Data; end; +procedure TEnvelope.SetAllRecipientData(Data: integer; RMsg: string = ''); +var i: integer; +begin + for i:= 0 to Length(FRecipients) - 1 do + SetRecipientData(i, Data, RMsg); +end; + procedure TEnvelope.SetReturnPath(Address: string); begin FReturnPath:= Address; diff --git a/MgSMTP.pas b/MgSMTP.pas index 181d969..b172a10 100644 --- a/MgSMTP.pas +++ b/MgSMTP.pas @@ -49,7 +49,7 @@ const document what bugfix/feature are you testing with the actual build. This will be logged to help you differentiate outputs of subsequent builds in your logs. If left empty, it won't be added to the logs. } - DEVCOMMENT = 'EHLO->HELO fallback'; + DEVCOMMENT = 'Detect unexpected disconnection'; var diff --git a/Relay.pas b/Relay.pas index cea792d..713db8d 100644 --- a/Relay.pas +++ b/Relay.pas @@ -1,6 +1,6 @@ { MegaBrutal's SMTP Server (MgSMTP) - Copyright (C) 2010 MegaBrutal + Copyright (C) 2010-2015 MegaBrutal This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General Public License as published by @@ -88,6 +88,7 @@ type protected FEnvelope: TEnvelope; FEMailProperties: TEMailProperties; + FTransactionComplete: boolean; FRoutingTarget: TRoutingTarget; RoutingTable: TRoutingTable; TCP: TTCPRFCConnection; @@ -99,6 +100,7 @@ type public property Envelope: TEnvelope read FEnvelope; property EMailProperties: TEMailProperties read FEMailProperties; + property IsTransactionComplete: boolean read FTransactionComplete; property RelayServerName: string read GetRelayServerName; property RelayServerPort: integer read GetRelayServerPort; function OpenConnection: boolean; @@ -171,6 +173,7 @@ begin Self.RoutingTable:= RoutingTable; FEnvelope:= Envelope; FEMailProperties:= EMailProperties; + FTransactionComplete:= false; FRoutingTarget:= RoutingTable.GetRouteInfo(Envelope.RelayHost); Response:= TRFCReply.Create; FillChar(SMTPExtensions, SizeOf(TSMTPExtensions), #0); @@ -302,10 +305,8 @@ end; procedure TRelayer.AdministerMassFailure(var Result: boolean); -var i: integer; begin - for i:= 0 to Envelope.GetNumberOfRecipients - 1 do - Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text); + Envelope.SetAllRecipientData(Response.GetNumericCode, Response.ReplyText.Text); Result:= false; end; @@ -341,12 +342,13 @@ begin end else Result:= false; MXList.Free; + FTransactionComplete:= false; end; function TRelayer.Greet: boolean; { This function reads and checks the relay server's greeting. + Then identifies this server with an EHLO. Then, if necessary, authenticates at the connected relay server. - Then identifies this server with a HELO. The function returns true, if the authentication and the EHLO command were successful. } var @@ -360,7 +362,9 @@ begin Response.Clear; AdministerMassFailure(Result); TCP.ReadResponse(Response); - if Response.GetNumericCode = SMTP_R_READY then begin + + { Expect 2xx reply. } + if (Response.GetNumericCode div 100) = 2 then begin TCP.SendCommand(SMTP_C_EHLO, MainServerConfig.Name); TCP.ReadResponse(Response); @@ -426,20 +430,32 @@ function TRelayer.SendEnvelope: boolean; { Sends the envelope (that is the return-path and the recipient addresses). The function returns true, if the MAIL command were successful, and the relay server has accepted at least one of the recipient addresses. + This function returns false if a null reply is read, which is considered + as protocol violation. This function is aware of the SMTP extension, named PIPELINING. If it's supported by the server, we send RCPT commands stuffed, without waiting for a response. After all RCPTs are sent, we check all responses. } var - i, c: integer; Prms: string; + i, c: integer; UltimateFail: boolean; Prms: string; procedure ProcessRCPTResponse; begin TCP.ReadResponse(Response); - if Response.GetNumericCode = SMTP_R_OK then Inc(c); + { If we get an "OK" reply code, we increase the count of sucessful + recipients. } + if Response.GetNumericCode = SMTP_R_OK then Inc(c) + { Response code 0 is non-existent in the SMTP protocol. + If we receive something we _perceive_ as 0, then it is likely + that something seriously went wrong. MgSMTP shouldn't treat + this condition as permanent. } + else if Response.GetNumericCode = 0 then UltimateFail:= true; Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text); end; begin + { The MAIL command is considered the beginning of the transaction. } + FTransactionComplete:= false; + UltimateFail:= false; Response.Clear; Prms:= 'FROM:<' + Envelope.ReturnPath + '>'; @@ -463,8 +479,16 @@ begin for i:= 0 to Envelope.GetNumberOfRecipients - 1 do ProcessRCPTResponse; - Result:= c <> 0; + Result:= (c <> 0) and (not UltimateFail); + + { If there are no accepted recipients, and no protocol failure is + discovered, we can't send the DATA command, and practically we + can do nothing more for this transaction. Therefore it is + considered complete by protocol. } + FTransactionComplete:= (c = 0) and (not UltimateFail); + if not Result then begin + { Either way, try to reset SMTP state in case of failure. } TCP.SendCommand(SMTP_C_RSET); TCP.ReadResponse(Response); end; @@ -499,7 +523,12 @@ var i: integer; begin TCP.WriteLn('.'); TCP.ReadResponse(Response); + + { Mark the transaction complete, if we have a valid response. } + FTransactionComplete:= Response.GetNumericCode <> 0; + for i:= 0 to Envelope.GetNumberOfRecipients - 1 do begin + { Set status code for recipients those were accepted in the envelope stage: } if Envelope.GetRecipient(i).Data = SMTP_R_OK then Envelope.SetRecipientData(i, Response.GetNumericCode, Response.ReplyText.Text); end; diff --git a/Spool.pas b/Spool.pas index 558205d..1e45101 100644 --- a/Spool.pas +++ b/Spool.pas @@ -1,6 +1,6 @@ { MegaBrutal's SMTP Server (MgSMTP) - Copyright (C) 2010-2014 MegaBrutal + Copyright (C) 2010-2015 MegaBrutal This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General Public License as published by @@ -133,7 +133,7 @@ type SpoolConfig: TSpoolConfig; SpoolFilters: TSpoolFilters; function DeliverLocalMessage(SpoolObject: TSpoolObjectReader; MailboxPtr: pointer; ReturnPath, Recipient: string): integer; - procedure DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer); + function DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer): boolean; procedure HandleFailure(SpoolObject: TSpoolObjectReader; IsLocal: boolean; FailEnvelope: TEnvelope; FailedRecipient: TRecipient; AddStatus: integer; FailMsg: string); procedure HandleDeliveryResults(SpoolObject: TSpoolObjectReader; IsLocal: boolean; Envelope, FailEnvelope: TEnvelope; AddStatus: integer; FailMsg: string); procedure CreateBounceMessage(SourceSpoolObject: TSpoolObjectReader; FailEnvelope: TEnvelope); @@ -662,11 +662,8 @@ begin R:= Mailbox^.DeliverMessagePart(LockID, Chunk); end; if R then begin - if Mailbox^.FinishDeliverMessage(LockID) then begin - Result:= 0; - { It's better to set in Execute. } - {SpoolObject.QuickSetDeliveryStatus(Recipient, DS_DELIVERED);} - end + if Mailbox^.FinishDeliverMessage(LockID) then + Result:= 0 else Result:= 4; end @@ -682,7 +679,10 @@ begin Result:= 1; end; -procedure TDeliveryThread.DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer); +function TDeliveryThread.DeliverRelayMessage(SpoolObject: TSpoolObjectReader; Relayer: TRelayer): boolean; +{ Relay message to remote server. + Returns true if the transaction went through (doesn't necessarily mean + that the message was actually accepted). } var Headers, Chunk: TStrings; R: boolean; begin if Relayer.PrepareSendMessage then begin @@ -690,11 +690,12 @@ begin Chunk:= TStringList.Create; { Leave a line between the headers and the body. } Headers.Add(''); + R:= Relayer.DeliverMessagePart(Headers); while (not SpoolObject.IsEOF) and R do begin - { Maybe constant "32" should be configurable? } + { Maybe constant "64" should be configurable? } Chunk.Clear; - SpoolObject.ReadChunk(Chunk, 32); + SpoolObject.ReadChunk(Chunk, 64); R:= Relayer.DeliverMessagePart(Chunk); end; if R then begin @@ -702,9 +703,12 @@ begin end else SpoolObject.SetDeliveryStatus(false, Relayer.Envelope, DS_DELAYED or DS_CONNECTIONFAIL); + + Result:= R; Chunk.Free; Headers.Free; - end; + end + else Result:= false; end; function TDeliveryThread.NeedSendReport(SpoolObject: TSpoolObject): boolean; @@ -866,9 +870,10 @@ begin if Relayer.Greet then if Relayer.SendEnvelope then DeliverRelayMessage(SpoolObject, Relayer); + + AssignDeliveryStatusToSMTPCodes(CurrEnv, Relayer.IsTransactionComplete); Relayer.CloseConnection; Relayer.Free; - AssignDeliveryStatusToSMTPCodes(CurrEnv); HandleDeliveryResults(SpoolObject, false, CurrEnv, FailEnv, 0, ''); end else begin -- 2.34.1