Menu

#52 Accept SMTP commands with bare line feeds

v1.0 (example)
wont-fix
nobody
None
5
2021-02-28
2021-02-10
Akash
No

Hi there..

8 years on and I am still in love with this nifty app. Helps me to consolidate emails from all my different servers w/o eating up too many resources. Thanks again for writing it.

I have realized that emailrelay in trying to be RFC conformant doesn't respond to SMTP commands terminating with bare line feeds. For example HELO me\r\n to a emailrelay server works but HELO me\n does not. The remote SMTP client just keeps waiting for a response in the latter case. Is it possible to make emailrelay accept SMTP commands ending either with \n or \r\n.

Thanks.

Discussion

  • Graeme Walker

    Graeme Walker - 2021-02-11
    • status: open --> wont-fix
     
  • Graeme Walker

    Graeme Walker - 2021-02-11

    Unfortunately your client is broken, and it is not in anyone's interest for conforming software to try to work round the non-conformance of others. Remember IE6 if nothing else.

    Having said that, the good news is you can easily change the source code yourself and rebuild: look for GNet::LineBufferConfig::smtp() and remove the "\r\n",2U .

    I can do you a special one-off build if you like, but I can't put that sort of work-round in for real. Sorry.

     
  • Akash

    Akash - 2021-02-12

    Hi Graeme,

    I don't have a use case or a broken client but what I have observed is that most mainstream MTAs like Postfix, Exim, Nginx (as mail proxy) and may be others too support this by default. Another non-RFC thing that they support are the missing angle brackets in MAIL & RCPT (which I shall try to figure out of my own). But this makes me wonder that if they are supporting it, there must be clients existing in the wild world of web who send \n instead of \r\n. Before posting this I had reached the following code in glinebuffer.cpp but don't know what to replace it with:

    GNet::LineBufferConfig GNet::LineBufferConfig::smtp()
    {
        return LineBufferConfig( std::string("\r\n",2U) , 998U + 2U , /*fmin=*/2U ) ; // RFC-2822
    }
    

    I understand that the first parameter specifies the EOL to look for, second is the max line length over which a warning is thrown but unsure of the third parameter (fmin). I tried replacing it with:

    return LineBufferConfig( std::string("\n",1U) , 999U + 1U , /*fmin=*/1U ) ;
    

    But emailrelay crashed with this error in the log after receiving a HELO me\n:

     assertion error: gsmtpserverprotocol.cpp(193): eolsize == 2U || ( inDataState() && eolsize == 0U )
    

    I then changed the line 193 in gsmtpserverprotocol.cpp from:

    G_ASSERT( eolsize == 2U || ( inDataState() && eolsize == 0U ) ) ;
    

    to:

    G_ASSERT( eolsize == 1U || ( inDataState() && eolsize == 0U ) ) ;
    

    Some progress as HELO, MAIL, RCPT & DATA are accepted but the transaction gets stuck at:

    354 start mail input -- end with <CRLF>.<CRLF>

    irrespective of whether a \n.\n is sent or a \r\n.\r\n after the DATA command . Restored gsmtpserverprotocol.cpp to its original state and as per your direction replaced the line in glinebuffer.cpp (removed: "\r\n",2U) :

    return LineBufferConfig( std::string("\r\n",2U) , 998U + 2U , /*fmin=*/2U ) ; // RFC-2822
    

    with:

       return LineBufferConfig( std::string() , 998U + 2U , /*fmin=*/2U ) ; // RFC-2822
    

    This build also gives the same assertion error as mentioned above on receiving a HELO\n. Will really appreciate some pointers. Although I am currently just looking for emailrelay to support either of \n or \r\n, my ultimate goal is to make a emailrelay build which is fault tolerant like other MTAs and lax on line endings and should work with mixed EOLs in the commands & data. For example this should work:

    HELO me\n
    MAIL FROM: <>\r\n
    RCPT TO: <test@test.com>\n
    DATA\n
    ....
    A \n B \r\n C \r D
    ....
    \r\n.\r\n
    QUIT\n
    

    This too should work:

    HELO me\n
    MAIL FROM: <>\n
    RCPT TO: <test@test.com>\r\n
    DATA\r\n
    ....
    A \n B \r\n C \r D
    ....
    \n.\n
    QUIT\r\n
    

    Thanks.

     

    Last edit: Akash 2021-02-12
  • Graeme Walker

    Graeme Walker - 2021-02-12

    I hope and expect that the laxities of other MTAs are for historical reasons; there were broken clients in the wild for a time and the server code has not been tightened up after those clients were fixed or went away. That's what I have observed with the angle-bracket thing -- I got complaints for a while, but those complaints have dried up in recent years.

    The obvious problem with allowing newline line endings is what do you do with "\n.\n" within the body text? There might be smart ways of dealing with it, but I would be very reluctant to add complexity to the body data transfer just to accommodate clients that don't follow the RFCs, especially if those clients no longer exist.

    But it's great that you take a different view and are willing to get stuck into the code.

    The line buffering code in emailrelay is optimised to minimise data copying, so it is more complicated than you might expect. The key to avoiding data copying is to allow the line buffer to serve up line fragments, so it identifies line breaks without actually assembling complete lines. The 'fmin' parameter is to avoid the situation where a line fragment is so small that it becomes difficult for the higher-level protocol to process. The motivating case is when the SMTP server receives a line fragment of "." during body text data transfer -- it might be the first part of a ".." escape, or it might be the start of the EOT mark. If you tell the line buffer to serve up fragments of at least two characters (fmin=2), and also tell you how far into the current line you are (linesize) and what the first character of the line was (c0), then the ambiguity goes away.

    The documentation is in the header files. The relevant comment for fmin is "The fmin value can be used to prevent trivially small line fragments from being returned. This is useful for SMTP where a fragment containing a single dot character and no end-of-line can cause confusion with respect to the end-of-text marker."

    If you allow autodetection of newline endings then the assertion check should be changed to "eolsize <= 2".

    The other fix should be in GSmtp::ServerProtocol::isEndOfText(). I think changing e.eolsize==2U to just e.eolsize (ie. a non-zero test) should suffice.

     
  • Akash

    Akash - 2021-02-18

    Hi,

    Thanks for the explanation and the tips, following which I was able to achieve somewhat satisfactory support for bare \n. I think server determines the eol by the first line sent by client and acts accordingly during the entire session thereafter. So if a HELO\n is sent, all further commands must end with \n only: DATA\n QUIT\n etc and end of data is also determined by a \n.\n only (\r\n.\r\n doesn't work). Likewise if first command is sent with \r\n, entire session must use \r\n only. To emulate the behavior of other MTAs which work just fine with mixed line endings, I made another change by adding:

    G::Str::trimRight( line , " \r" ) ;
    

    after:

    G::Str::trimLeft( line , " \t" ) ;
    

    in: std::string GSmtp::ServerProtocol::commandWord(). This allowed me to have mixed line endings in the commands at least. The following sequences which weren't working before work now after this change:

    HELO\n
    ...
    ...
    DATA\r\n
    \n.\n
    QUIT\n
    
    HELO\r\n
    ...
    ...
    DATA\n
    \r\n.\r\n
    QUIT\n
    

    But end of data is still getting determined by the line ending of first line from client. Postfix & Exim are very flexible with this and ignore the presence of \r in the \r\n.\r\n combination and thus will accept anything like: \r\n.\r\n , \n.\n , \n.\r\n , \r\n.\n

    Is it too complicated to achieve the same in emailrelay?

    Thanks.

     
  • Graeme Walker

    Graeme Walker - 2021-02-18

    So how about you use newline as the line-buffer terminator, without auto-detection, set fmin to 3, strip carriage-returns early on in ServerProtocol::apply(), and make isEndOfText() more accommodating?

    std::string line( line_data , line_data_size ) ;
    G::Str::trimRight( line , "\r" ) ; // new
    event = commandEvent( ... etc ... )

    return ( e.linesize == 1U && e.eolsize == 1U && e.c0 == '.' ) ||
    ( e.linesize == 2U && e.eolsize == 1U && e.c0 == '.' && e.ptr[1] == '\r' ) ;

     
  • Graeme Walker

    Graeme Walker - 2021-02-18

    ... and right-trim similarly in ServerProtocol::doAuthData()

     
  • Akash

    Akash - 2021-02-18

    Perfect :) Its now working just like I wanted. Can't thank you enough for the above instructions. :)

    BTW emailrelay has been into existence since many years now. Didn't you ever try to get it into OS repositories? I really feel that this nice, well crafted and maintained software deserves much more exposure.

    Another thing. A verbose log of emailrelay shows something like this:

    [22119]: emailrelay: info: tx>>: "220 server -- E-MailRelay V2.2rc3 -- Service ready"
    [22119]: emailrelay: info: rx<<: "HELO me"
    [22119]: emailrelay: info: tx>>: "250 OK"
    [22119]: emailrelay: info: rx<<: "MAIL FROM: <>"
    [22119]: emailrelay: warning: empty MAIL-FROM return path
    [22119]: emailrelay: info: content file: /tmp/emailrelay.22119.1613677542.2.content
    [22119]: emailrelay: info: tx>>: "250 OK"
    [22119]: emailrelay: info: rx<<: "RCPT TO: <test@test.com>"
    [22119]: emailrelay: info: address verifier: executing [/verify.sh] [test@test.com] [] [127.0.0.1:36772] [server] [] []
    [22119]: emailrelay: info: address verifier: exit code 1 [] [test@test.com]
    [22119]: emailrelay: info: tx>>: "250 OK"
    [22119]: emailrelay: info: rx<<: "DATA"
    [22119]: emailrelay: info: tx>>: "354 start mail input -- end with <CRLF>.<CRLF>"
    [22119]: emailrelay: info: rx<<: [message content not logged]
    [22119]: emailrelay: info: rx<<: "."
    [22119]: emailrelay: info: envelope file: /tmp/emailrelay.22119.1613677542.2.envelope.new
    [22119]: emailrelay: info: tx>>: "250 OK"
    [22119]: emailrelay: info: rx<<: "QUIT"
    [22119]: emailrelay: info: tx>>: "221 OK"
    [22119]: emailrelay: info: smtp connection closed: smtp protocol done: 127.0.0.1:36772
    

    Which indicates that the spool file name is generated before address verifier script is run. Have you ever considered making it available to the script as an additional parameter or environment etc.? "filter" script which gets the content & envelope file names doesn't get the meta information (especially the remote IP) which the verifier script gets (and vice versa). If the filter script needs this information to make a decision, it has to parse the envelope file. If the verifier script can gets the spool file name, it can store this meta information in some easily re-readable form (/path-to-spool-file.meta) which can be later utilized by filter script. It would be nice to have emailrelay also send the value of client "HELO/EHLO" to the verifier script.

    Regards.

     
  • Akash

    Akash - 2021-02-19

    Just noticed that the HELO/EHLO value doesn't get recorded in the envelope file also nor is sent as a parameter to the verifier script. I just want the script (filter/verifier) to perform some basic spam tests before accepting the email for which HELO value is required.

     
  • Akash

    Akash - 2021-02-19

    Hi.

    Figured out how to send the HELO value to verifier script. Declaration in a lot many files needed to changed.

    gsmtpserverprotocol.cpp:

    m_verifier.verify( to , from , m_peer_address , mechanism , id ) ;
    

    to:

    m_verifier.verify( to , from , m_peer_address , mechanism , id , m_session_peer_name ) ;
    

    gexecutableverifier.cpp :

    void GSmtp::ExecutableVerifier::verify( const std::string & to_address ,
        const std::string & from_address , const GNet::Address & ip ,
        const std::string & auth_mechanism , const std::string & auth_extra )
    {
        G_DEBUG( "GSmtp::ExecutableVerifier::verify: to \"" << to_address << "\": from \"" << from_address << "\": "
            << "ip \"" << ip.hostPartString() << "\": auth-mechanism \"" << auth_mechanism << "\": "
            << "auth-extra \"" << auth_extra << "\"" ) ;
    
        G::ExecutableCommand commandline( m_path.str() , G::StringArray() ) ;
        commandline.add( to_address ) ;
        commandline.add( from_address ) ;
        commandline.add( ip.displayString() ) ;
        commandline.add( GNet::Local::canonicalName() ) ;
        commandline.add( G::Str::lower(auth_mechanism) ) ;
        commandline.add( auth_extra ) ;
    
        G_LOG( "GSmtp::ExecutableVerifier: address verifier: executing " << commandline.displayString() ) ;
        m_to_address = to_address ;
        m_task.start( commandline ) ;
    }
    

    to:

    void GSmtp::ExecutableVerifier::verify( const std::string & to_address ,
        const std::string & from_address , const GNet::Address & ip ,
        const std::string & auth_mechanism , const std::string & auth_extra , const std::string & helo )
    {
        G_DEBUG( "GSmtp::ExecutableVerifier::verify: to \"" << to_address << "\": from \"" << from_address << "\": "
            << "ip \"" << ip.hostPartString() << "\": auth-mechanism \"" << auth_mechanism << "\": "
            << "auth-extra \"" << auth_extra << "\": "
            << "helo \"" << helo << "\"" ) ;
    
        G::ExecutableCommand commandline( m_path.str() , G::StringArray() ) ;
        commandline.add( to_address ) ;
        commandline.add( from_address ) ;
        commandline.add( ip.displayString() ) ;
        commandline.add( GNet::Local::canonicalName() ) ;
        commandline.add( G::Str::lower(auth_mechanism) ) ;
        commandline.add( auth_extra ) ;
        commandline.add( helo) ;
    
        G_LOG( "GSmtp::ExecutableVerifier: address verifier: executing " << commandline.displayString() ) ;
        m_to_address = to_address ;
        m_task.start( commandline ) ;
    }
    

    ginternalverifier.cpp :

    void GSmtp::InternalVerifier::verify( const std::string & to , const std::string & , const GNet::Address & ,
        const std::string & , const std::string & )
    

    to:

    void GSmtp::InternalVerifier::verify( const std::string & to , const std::string & , const GNet::Address & ,
        const std::string & , const std::string & , const std::string & )
    

    gnetworkverifier.cpp :

    void GSmtp::NetworkVerifier::verify( const std::string & mail_to_address ,
            const std::string & mail_from_address , const GNet::Address & client_ip ,
            const std::string & auth_mechanism , const std::string & auth_extra )
    {
        if( m_client_ptr.get() == nullptr )
        {
            m_client_ptr.reset( new RequestClient(GNet::ExceptionSink(m_client_ptr,m_es.esrc()),"verify","",m_location,m_connection_timeout,m_response_timeout) ) ;
        }
    
        G::StringArray args ;
        args.push_back( mail_to_address ) ;
        args.push_back( mail_from_address ) ;
        args.push_back( client_ip.displayString() ) ;
        args.push_back( GNet::Local::canonicalName() ) ;
        args.push_back( G::Str::lower(auth_mechanism) ) ;
        args.push_back( auth_extra ) ;
    
        m_to_address = mail_to_address ;
        m_client_ptr->request( G::Str::join("|",args) ) ;
    }
    

    to:

    void GSmtp::NetworkVerifier::verify( const std::string & mail_to_address ,
            const std::string & mail_from_address , const GNet::Address & client_ip ,
            const std::string & auth_mechanism , const std::string & auth_extra , const std::string & helo  )
    {
        if( m_client_ptr.get() == nullptr )
        {
            m_client_ptr.reset( new RequestClient(GNet::ExceptionSink(m_client_ptr,m_es.esrc()),"verify","",m_location,m_connection_timeout,m_response_timeout) ) ;
        }
    
        G::StringArray args ;
        args.push_back( mail_to_address ) ;
        args.push_back( mail_from_address ) ;
        args.push_back( client_ip.displayString() ) ;
        args.push_back( GNet::Local::canonicalName() ) ;
        args.push_back( G::Str::lower(auth_mechanism) ) ;
        args.push_back( auth_extra ) ;
        args.push_back( helo ) ;
    
        m_to_address = mail_to_address ;
        m_client_ptr->request( G::Str::join("|",args) ) ;
    }
    

    declaration changes from:

    const std::string & auth_mechanism , const std::string & auth_extra )
    

    to:

    const std::string & auth_mechanism , const std::string & auth_extra , const std::string & helo  )
    

    in files:

    gexecutableverifier.h
    gnetworkverifier.h
    ginternalverifier.h

    But I wanted to ask whether its safe to send the HELO value (which is provided by the client) as a command line parameter to a script. Is any sanitization performed before the script is run with parameters with arbitrarily values. Additionally I can't seem to figure out a similar way to pass the spool file name to the verifier script. Travelling back from what is displayed in the verbose log, I figured that spool file is actually m_content_path from gnewfile.cpp but how do I utilize this value in: m_verifier.verify( .... ) inside gsmtpserverprotocol.cpp.

    Thanks.

     

    Last edit: Akash 2021-02-19
  • Graeme Walker

    Graeme Walker - 2021-02-19

    You're right that there is no consistency in the information available to verifier scripts compared to filter scripts.

    I have considered using environment variables in the past, but they are a bit more tricksy that they first appear, at least historically. But using a single environment variable to contain the path of the envelope file would be a reasonable approach, then additional meta-data can be stuffed into the envelope file without breaking existing scripts.

    The code structure does not make it easy, though: the ServerProtocol is given a ProtocolMessage reference, which might be a ProtocolMessageStore object; the ProtocolMessageStore object contains a NewMessage reference, which might be a NewFile object; and it is the NewFile class that knows about content and envelope files.

    I suggest changing NewMessage::id() so that it returns a string, which in NewFile should be the envelope path, and add a pure-virtual id() method to ProtocolMessage that just returns the NewMessage::id(). Then the envelope path will be available to ServerProtocol when it
    calls Verifier::verify().

    If you add the EHLO value to the envelope file note that the relevant code was re-written for the recent v2.2 release.

    You are also right that anything coming in over the wire should be treated as suspect, and especially if it is used on a command-line. I use xtext encoding when writing into the envelope file, although if I were starting again I would use url-encoding. G::Str::printable() and G::Base64::encode() can also be useful. You will see that I only use exec() to run scripts, so shell meta-character trickery is less of a risk.

     
  • Graeme Walker

    Graeme Walker - 2021-02-21

    When it comes to distros, I am only aware of OpenWrt carrying emailrelay at the moment, but I would be delighted if an emailrelay user pushed for it to be included in some repository or other. It's not something I would do myself though; I publish emailrelay here and I hope it is useful, but I have never advertised or advocated for it.

     
  • Akash

    Akash - 2021-02-26

    Thanks Graeme for the hints.

    I suggest changing NewMessage::id() so that it returns a string, which in NewFile should be the envelope path, and add a pure-virtual id() method to ProtocolMessage that just returns the NewMessage::id(). Then the envelope path will be available to ServerProtocol when it
    calls Verifier::verify().

    Shall try but it surely feels like something which is beyond my C++ programing and tweaking abilities. In the meantime I am trying to implement PROXY protocol & XCLIENT in Emailrelay. Not that difficult as their format is such that they can be interpreted as any other SMTP command. For XCLIENT:

    m_fsm.addTransition( Event::eXclient , State::s_Any , State::s_Same , &GSmtp::ServerProtocol::doXclient ) ;

    if( command == "XCLIENT" ) return Event::eXclient ;

    void GSmtp::ServerProtocol::doXclient( EventData event_data  , bool & )
    {
        std::string line( event_data.ptr , event_data.size ) ;
        std::string real_peer_address;
        std::size_t pos = line.find( " ADDR=" ) ;
        if( pos != std::string::npos )
        {
        real_peer_address = line.substr( pos + 6U ) ;
        m_peer_address = ?? // How to update it with real_peer_address ??
        }
        sendOk() ;
    }
    

    That is assuming XCLIENT: ADDR=1.2.3.4 which isn't the case normally. Ignore that part as I will add more parsing code later. But once the real IP address is extracted into: real_peer_address, I need to update m_peer_address with it. There exists a method m_peer_address.hostPartString() to get the string representation of the address but I need to do the reverse. Can't seem to figure that out.

     

    Last edit: Akash 2021-02-26
  • Graeme Walker

    Graeme Walker - 2021-02-26

    Just construct an Address object with the network address string and a zero port number, and assign it. m_peer_address = Address( line.substr(...) , 0U ) ;

     
  • Akash

    Akash - 2021-02-27

    Hi,

    Using your help and pointers I have been able to make much of the desired changes. Just finished implementing PROXY & XCLIENT for running emailrelay behind other proxying servers like nginx & haproxy. I will upload the code in git and share the link here when I have debugged the code changes I have made. May be someday someone else will find them useful.

    I see that emailrelay can listen on multiple interfaces (but not on multiple ports). Is there any way for gsmtpserverprotocol.cpp to fetch the IP address, the connection was accepted on. This information isn't even displayed in the debug logs so I couldn't trace back from there. And the code structure is far too complex for me to find where it does getsockname to retrieve this.

     
  • Graeme Walker

    Graeme Walker - 2021-02-28

    You should add an additional Address parameter to the ServerProtocol constructor; the Smtp::ServerPeer constructor can obtain the address at the local end of the connection by calling the base-class's localAddress() method, or it can obtain the listening address as peer_info.m_server->address().

    GSmtp::ServerPeer::ServerPeer( ... ) :
    ...
    m_protocol( ... peer_info.m_address , localAddress().second , ... )
    // or m_protocol( ... peer_info.m_address , peer_info.m_server->address() , ... )
    {
    ...
    }

    Note that a Server listens for incoming connections and spins off a ServerPeer object of the appropriate dynamic type to represent each new connection, passing it a PeerInfo object containing the new socket and a pointer back to the Server. For SMTP the Smtp::Server overrides newPeer() to create a Smtp::ServerPeer object, which is-a Net::ServerPeer. The Net::ServerPeer is-a Net::Connection so it has localAddress() and peerAddress() methods.

     

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.