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

[security] smtplib multiple CRLF injection #87290

Closed
martinortner mannequin opened this issue Feb 4, 2021 · 17 comments
Closed

[security] smtplib multiple CRLF injection #87290

martinortner mannequin opened this issue Feb 4, 2021 · 17 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes deferred-blocker type-security A security issue

Comments

@martinortner
Copy link
Mannequin

martinortner mannequin commented Feb 4, 2021

BPO 43124
Nosy @warsaw, @tiran, @ned-deily, @bitdancer, @ambv, @miss-islington, @miguendes
PRs
  • bpo-43124: Fix smtplib multiple CRLF injection #25987
  • [3.10] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) #28034
  • [3.9] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) #28035
  • [3.8] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) #28036
  • [3.7] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) #28037
  • [3.6] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) #28038
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-08-29.15:09:19.891>
    created_at = <Date 2021-02-04.13:05:34.075>
    labels = ['type-security', 'deferred-blocker', '3.8', '3.9', '3.10', '3.11', '3.7']
    title = '[security] smtplib multiple CRLF injection'
    updated_at = <Date 2021-08-30.19:22:04.988>
    user = 'https://bugs.python.org/martinortner'

    bugs.python.org fields:

    activity = <Date 2021-08-30.19:22:04.988>
    actor = 'ned.deily'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-08-29.15:09:19.891>
    closer = 'lukasz.langa'
    components = []
    creation = <Date 2021-02-04.13:05:34.075>
    creator = 'martin.ortner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43124
    keywords = ['patch']
    message_count = 17.0
    messages = ['386481', '390048', '393206', '393208', '393209', '394160', '397421', '397423', '397433', '397806', '400535', '400540', '400541', '400543', '400546', '400661', '400663']
    nosy_count = 8.0
    nosy_names = ['barry', 'christian.heimes', 'ned.deily', 'r.david.murray', 'lukasz.langa', 'miss-islington', 'martin.ortner', 'miguendes']
    pr_nums = ['25987', '28034', '28035', '28036', '28037', '28038']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue43124'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @martinortner
    Copy link
    Mannequin Author

    martinortner mannequin commented Feb 4, 2021

    // reported via PSRT email (see timeline; last contact: Alex/PSRT)
    // external reference: http://consensys.net/diligence/vulnerabilities/private/z5kxjgfmja4offxbrw1miuxwezggajjfswlz9g2hfuh77we5dy727hqy5x9ii43e/

    cve:
    vendor: python
    vendorUrl: https://www.python.org/
    authors: tintinweb
    affectedVersions: [at least 3.8.3, <=3.7.8, <=3.6.11, <=3.5.9, <=2.7.18]
    vulnClass: CWE-93

    # Vulnerability Note

    ## Summary

    Python is a programming language that lets you work more quickly and integrate your systems more effectively.

    Two CR-LF injection points have been discovered in the Python standard library for SMTP interaction (client perspective) named smtplib that may allow a malicious user with direct access to smtplib.SMTP(..., local_hostname, ..) or smtplib.SMTP(...).mail(..., options) to inject a CR-LF control sequence to inject arbitrary SMTP commands into the protocol stream.

    The root cause of this is likely to be found in the design of the putcmd(cmd, args) method, that fails to validate that cmd nor args contains any protocol control sequences (i.e. CR-LF).

    It is recommended to reject or encode \r\n in putcmd() and enforce that potential multi-line commands call putcmd() multiple times to avoid that malicious input breaks the expected context of the method and hence cause unexpected behavior. For reference, the DATA command (multi-line) would not be affected by this change as it calls putcmd() only once and continues with directly interacting with the socket to submit the body.

    ## Details

    ### Description

    The root cause of this (and probably also some earlier reported CR-LF injections) is the method putcmd() in lib/smtplib.py[3]. The method is called by multiple commands and does not validate that neither cmd nor args contains any CRLF sequences.

    def putcmd(self, cmd, args=""):
        """Send a command to the server."""
        if args == "":
            str = '%s%s' % (cmd, CRLF)
        else:
            str = '%s %s%s' % (cmd, args, CRLF)
        self.send(str)

    However, the issue was initially found in mail(..., options) [4] which fails to ensure that none of the provided options contains CRLF characters. The method only ensures that provides mail addresses are quoted, optionslist is untouched:

    self.putcmd("mail", "FROM:%s%s" % (quoteaddr(sender), optionlist))

    A similar issue was found with smtplib.SMTP(...,local_hostname) (and helo(name), ehlo(name)) which may potentially contain CRLF sequences and, therefore, can be used to inject SMTP commands.

    Here's a snipped of helo [5]

    def helo(self, name=''):
        """SMTP 'helo' command.
        Hostname to send for this command defaults to the FQDN of the local
        host.
        """
        self.putcmd("helo", name or self.local_hostname)
        (code, msg) = self.getreply()
        self.helo_resp = msg
        return (code, msg)

    We highly recommend, fixing this issue once and for all directly in putcmd() and enforce that the interface can only send one command at a time, rejecting arguments that contain CRLF sequences or properly encoding them to avoid injection.

    ## Proof of Concept

    1. set-up a local tcp listener ⇒ nc -l 10001

    2. run the following PoC and replay the server part as outline in 3.

    import smtplib
    
    server = smtplib.SMTP('localhost', 10001, "hi\nX-INJECTED") # localhostname CRLF injection
    server.set_debuglevel(1)
    server.sendmail("hi@me.com", "you@me.com", "wazzuuup\nlinetwo")
    server.mail("hi@me.com",["X-OPTION\nX-INJECTED-1","X-OPTION2\nX-INJECTED-2"]) # options CRLF injection
    1. interact with smtplib, check for X-INJECTED
    ⇒  nc -l 10001                       
    nc -l 10001
    220 yo
    ehlo hi
    X-INJECTED
    250-AUTH PLAIN
    250
    mail FROM:<hi@me.com>
    250 ok
    rcpt TO:<you@me.com>
    250 ok
    data
    354 End data with <CR><LF>.<CR><LF> 
    wazzuuup
    linetwo
    .
    250 ok
    mail FROM:<hi@me.com> X-OPTION
    X-INJECTED-1 X-OPTION2
    X-INJECTED-2
    250 ok
    quit
    250 ok
    

    ### Proposed Fix

    • enforce that putcmd emits exactly one command at a time and encode \n -> \\n.
    diff --git a/Lib/smtplib.py b/Lib/smtplib.py
    index e2dbbbc..9c16e7d 100755
    --- a/Lib/smtplib.py
    +++ b/Lib/smtplib.py
    @@ -365,10 +365,10 @@ class SMTP:
         def putcmd(self, cmd, args=""):
             """Send a command to the server."""
             if args == "":
    -            str = '%s%s' % (cmd, CRLF)
    +            str = cmd
             else:
    -            str = '%s %s%s' % (cmd, args, CRLF)
    -        self.send(str)
    +            str = '%s %s' % (cmd, args)
    +        self.send('%s%s' % (str.replace('\n','\\n'), CRLF))

    ## Vendor Response

    Vendor response: gone silent

    ### Timeline

    JUL/02/2020 - contact psrt; provided details, PoC, proposed patch
    JUL/04/2020 - confirmed that vulnerability note was received
    SEP/10/2020 - requested status update.
    

    ## References

    • [1] https://www.python.org/
    • [2] https://www.python.org/downloads/
    • [3]

      cpython/Lib/smtplib.py

      Lines 365 to 371 in 1da648a

      def putcmd(self, cmd, args=""):
      """Send a command to the server."""
      if args == "":
      str = '%s%s' % (cmd, CRLF)
      else:
      str = '%s %s%s' % (cmd, args, CRLF)
      self.send(str)
    • [4]
      def mail(self, sender, options=()):
    • [5]

      cpython/Lib/smtplib.py

      Lines 428 to 445 in 1da648a

      def helo(self, name=''):
      """SMTP 'helo' command.
      Hostname to send for this command defaults to the FQDN of the local
      host.
      """
      self.putcmd("helo", name or self.local_hostname)
      (code, msg) = self.getreply()
      self.helo_resp = msg
      return (code, msg)
      def ehlo(self, name=''):
      """ SMTP 'ehlo' command.
      Hostname to send for this command defaults to the FQDN of the local
      host.
      """
      self.esmtp_features = {}
      self.putcmd(self.ehlo_msg, name or self.local_hostname)
      (code, msg) = self.getreply()

    @martinortner martinortner mannequin added type-security A security issue topic-email 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Feb 4, 2021
    @vstinner vstinner changed the title smtplib multiple CRLF injection [security] smtplib multiple CRLF injection Feb 4, 2021
    @vstinner vstinner changed the title smtplib multiple CRLF injection [security] smtplib multiple CRLF injection Feb 4, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Apr 2, 2021

    Deferred the blocker to a regular release due to lack of activity in time for the current expedited releases.

    @ned-deily
    Copy link
    Member

    Still in "deferred blocker" status awaiting a PR from someone

    @miguendes
    Copy link
    Mannequin

    miguendes mannequin commented May 7, 2021

    If there's no one working on it I'd be happy to prepare a fix.

    @ned-deily
    Copy link
    Member

    There is no sign of anyone currently working on it, so please feel free to dig in!

    @ned-deily
    Copy link
    Member

    Thanks for the PR! Can someone from the email team take a look at it, please?

    @pouralialireza2 pouralialireza2 mannequin removed topic-email 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Jul 10, 2021
    @pouralialireza2 pouralialireza2 mannequin added performance Performance or resource usage and removed type-security A security issue labels Jul 10, 2021
    @bitdancer
    Copy link
    Member

    This bug report starts with "a malicious user with direct access to `smtplib.SMTP(..., local_hostname, ..)", which is a senseless supposition. Anyone with "access to" the SMTP object could just as well be talking directly to the SMTP server and do anything they want that SMTP itself allows.

    The concern here is that data a program might obtain *from unsanitized user input* could be used to do header injection. The "proof of concept" does not address this at all. We'd need to see a scenario under which data that could reasonably be derived from user input ends up being passed as arguments to an smtplib method that calls putcmd with arguments.

    So, I would rate this as *very* low impact issue, unless someone has an *actual example* of code using smtplib that passes user input through to smtplib commands in an exploitable way.

    That said, it is perfectly reasonable to be proactive here and prevent scenarios we haven't yet thought of, by doing as recommended (and a bit more) by raising a ValueError if 'args' in the putcmd call contain either \n or \r characters. I don't think we need to check 'cmd', because I can't see any scenario in which the SMTP command would be derived from user input. If you want to be *really* paranoid you could check cmd too, and since it will always be a short string the additional performance impact will be minor.

    @bitdancer bitdancer added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes type-security A security issue and removed performance Performance or resource usage labels Jul 13, 2021
    @bitdancer
    Copy link
    Member

    s/header injection/command injection/

    @martinortner
    Copy link
    Mannequin Author

    martinortner mannequin commented Jul 13, 2021

    This bug report starts with "a malicious user with direct access to `smtplib.SMTP(..., local_hostname, ..)", which is a senseless supposition. Anyone with "access to" the SMTP object could just as well be talking directly to the SMTP server and do anything they want that SMTP itself allows.

    Let's not argue about the phrasing and settle on the fact that I am not a native English speaker which might be the root cause of the confusion. The core of the issue is that this *unexpected side-effect* may be security-relevant. Fixing it probably takes less time than arguing about phrasing, severity, or spending time describing exploitation scenarios for a general-purpose library that should protect the underlying protocol from injections.

    Be kind, I come in peace.

    @bitdancer
    Copy link
    Member

    My apologies, I did not think about the possibility of an English issue. I was reacting to the "security report speak", which I find often makes a security issue sound worse than it is :) Thank you for reporting this problem, and I do think we should fix it.

    My posting was directed at the severity of the issue, since it was potentially holding up a release. My point about the example is that without an example of code that could reasonably be expected to use user input in a call that could inject newlines, we can treat this as a low priority issue. If we had a proposed example of such code, then the priority would be higher. If it was an example of such code "in the wild", then it would be quite high :)

    The reason I'm saying we should have an example in order to consider it higher priority is that I cannot see *any* likelihood that this would be a problem in practice. Let me explain.

    putcmd is an *internal* interface. If we look at the commands that call putcmd or docmd, the only ones that pass extra data that aren't pretty obviously safe (ie: not clearly sanitized data) are rcpt and mail[]. In both cases the item of concern is optionslist. optionslist is a list of *SMTP server options. This is not data that is reasonably taken from user input, it is data provided *by the programmer*.

    [*] I did double check to make sure that email.utils.parseaddr sanitizes both \r and \r, just to be sure :)

    Therefore this is *not* a significant security issue. But as I said, we should take the "defense in depth" approach and apply the check in putcmd as you recommend. I just don't think it needs to hold up a release.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 29, 2021

    New changeset 0897253 by Miguel Brito in branch 'main':
    bpo-43124: Fix smtplib multiple CRLF injection (GH-25987)
    0897253

    @miss-islington
    Copy link
    Contributor

    New changeset 9e6c317 by Miss Islington (bot) in branch '3.10':
    bpo-43124: Fix smtplib multiple CRLF injection (GH-25987)
    9e6c317

    @ambv
    Copy link
    Contributor

    ambv commented Aug 29, 2021

    New changeset 24416e4 by Miss Islington (bot) in branch '3.9':
    bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) (GH-28035)
    24416e4

    @ambv
    Copy link
    Contributor

    ambv commented Aug 29, 2021

    New changeset b93aea4 by Miss Islington (bot) in branch '3.8':
    [3.8] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) (GH-28036)
    b93aea4

    @ambv
    Copy link
    Contributor

    ambv commented Aug 29, 2021

    Thanks, Martin! ✨ 🍰 ✨

    @ambv ambv closed this as completed Aug 29, 2021
    @ambv ambv closed this as completed Aug 29, 2021
    @ned-deily
    Copy link
    Member

    New changeset d2cc04c by Miss Islington (bot) in branch '3.7':
    [3.7] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) (GH-28037)
    d2cc04c

    @ned-deily
    Copy link
    Member

    New changeset 29d97d1 by Miss Islington (bot) in branch '3.6':
    [3.6] bpo-43124: Fix smtplib multiple CRLF injection (GH-25987) (GH-28038)
    29d97d1

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes deferred-blocker type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants