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] CVE-2021-29921: ipaddress Should not reject IPv4 addresses with leading zeroes as ambiguously octal #80565

Closed
TV4Fun mannequin opened this issue Mar 20, 2019 · 40 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes deferred-blocker stdlib Python modules in the Lib dir type-security A security issue

Comments

@TV4Fun
Copy link
Mannequin

TV4Fun mannequin commented Mar 20, 2019

BPO 36384
Nosy @ncoghlan, @vstinner, @ericvsmith, @tiran, @ned-deily, @ambv, @mgorny, @Julian, @serhiy-storchaka, @zooba, @pablogsal, @miss-islington, @TV4Fun, @gcbirzan, @achraf-mer
PRs
  • bpo-36384: Remove check for leading zeroes in IPv4 addresses #12577
  • bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated #25099
  • [3.9] bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated (GH-25099) #25815
  • [3.8] bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated (GH-25099) #27801
  • bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 #27824
  • [3.8] bpo-36384: [doc] Correct typos in CVE-2021-29921 fix description #27825
  • [3.10] bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 (GH-27824) #27826
  • [3.9] bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 (GH-27824) #27827
  • 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-05-25.10:26:41.541>
    created_at = <Date 2019-03-20.23:40:09.514>
    labels = ['type-security', 'deferred-blocker', '3.8', '3.9', '3.10', 'library']
    title = '[security] CVE-2021-29921: ipaddress Should not reject IPv4 addresses with leading zeroes as ambiguously octal'
    updated_at = <Date 2021-08-19.09:20:50.963>
    user = 'https://github.com/TV4Fun'

    bugs.python.org fields:

    activity = <Date 2021-08-19.09:20:50.963>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-25.10:26:41.541>
    closer = 'pablogsal'
    components = ['Library (Lib)']
    creation = <Date 2019-03-20.23:40:09.514>
    creator = 'Joel Croteau'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36384
    keywords = ['patch', '3.8regression', '3.9regression']
    message_count = 40.0
    messages = ['338514', '338694', '339204', '339205', '339206', '339210', '339211', '339572', '389826', '389847', '390046', '390128', '390331', '390340', '390353', '390361', '392423', '392572', '392684', '392692', '392696', '392697', '394162', '394316', '394322', '394327', '394379', '394380', '394390', '399784', '399798', '399801', '399803', '399804', '399805', '399809', '399893', '399895', '399898', '399899']
    nosy_count = 18.0
    nosy_names = ['ncoghlan', 'vstinner', 'eric.smith', 'christian.heimes', 'ned.deily', 'pmoody', 'docs@python', 'lukasz.langa', 'mgorny', 'Julian', 'python-dev', 'serhiy.storchaka', 'steve.dower', 'pablogsal', 'miss-islington', 'Joel Croteau', 'gc2', 'achraf-mer']
    pr_nums = ['12577', '25099', '25815', '27801', '27824', '27825', '27826', '27827']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue36384'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @TV4Fun
    Copy link
    Mannequin Author

    TV4Fun mannequin commented Mar 20, 2019

    I understand to a certain extent the logic in not allowing IPv4 octets that might ambiguously be octal, but in practice, it just seems like it creates additional parsing hassle needlessly. I have never in many years of working on many networked systems seen anyone use dotted octal format, it is actually specifically forbidden by RFC 3986 (https://tools.ietf.org/html/rfc3986#section-7.4), and it means that the ipaddress module throws exceptions on many perfectly valid IP addresses just because they have leading zeroes. Since the module doesn't support dotted octal or dotted hex anyway, this check seems a little pointless. If nothing else, there should be a way to disable this check by specifying that your IPs are in fact dotted decimal, otherwise it seems like it's just making you have to do extra parsing work or just write your own implementation.

    @TV4Fun TV4Fun mannequin added 3.7 (EOL) end of life 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error 3.8 only security fixes labels Mar 20, 2019
    @SilentGhost SilentGhost mannequin removed 3.7 (EOL) end of life 3.9 only security fixes labels Mar 21, 2019
    @ericvsmith
    Copy link
    Member

    I agree that this is not a useful check.

    @ncoghlan
    Copy link
    Contributor

    New changeset e653d4d by Nick Coghlan (Joel Croteau) in branch 'master':
    bpo-36384: Remove check for leading zeroes in IPv4 addresses (GH-12577)
    e653d4d

    @ncoghlan
    Copy link
    Contributor

    I've merged the change for Python 3.8 (thanks Joel!).

    I'm not sure whether to classify it as an enhancement or as an interoperability bug fix, though, so I've put the status to pending and added Ned to the nosy list to get his thoughts as the Python 3.7 RM.

    @ned-deily
    Copy link
    Member

    ipaddress is behaving as documented:

    "The following constitutes a valid IPv4 address:

    A string in decimal-dot notation, consisting of four decimal integers in the inclusive range 0–255, separated by dots (e.g. 192.168.0.1). Each integer represents an octet (byte) in the address. Leading zeroes are tolerated only for values less than 8 (as there is no ambiguity between the decimal and octal interpretations of such strings). [...]"

    https://docs.python.org/3/library/ipaddress.html

    I can sort of understand imposing that restriction in a Python 2 world where leading zeros implied octal and Python 3 outright rejects such forms of integers to avoid the ambiguity. That said, there's no particular reason why the components of an IPv4 string acceptable to ipaddress *have* to follow the same rules so I'm +0 on making the change at all. It's a bit of a stretch to consider it a bug when it appears to be behaving as documented but I would expect such a change to fix more problems than causing them so I'm OK if you want to backport it.

    But, in any case, the documentation for 3.8 and/or 3.7 needs to be updated.

    @serhiy-storchaka
    Copy link
    Member

    See also the article "Ping and FTP Resolve IP Address with Leading Zero as Octal" (https://web.archive.org/web/20061206211851/http://support.microsoft.com/kb/115388).

    This is still true in Windows 10.

    So it is safer to reject IPv4 addresses with leading zeros that can be ambiguously interpreted. Otherwise this can create a security hole.

    @ericvsmith
    Copy link
    Member

    I think it should be 3.8 only, and the docs should be updated. Apologies for not catching that earlier: I searched via Google, which was a mistake.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 7, 2019

    The recommended handling in the article that Serhiy mentions is to strip the leading zeroes, which the ipaddress module will still do - it's only being made more tolerant on input. That means it will become usable as a prefilter step (pass string with potentially leading zeroes to ipaddress, get string with no leading zeroes out).

    So that means the one part we missed is the docs update (together with a versionchanged note in the module docs themselves)

    @ncoghlan ncoghlan added docs Documentation in the Doc dir and removed stdlib Python modules in the Lib dir labels Apr 7, 2019
    @tiran
    Copy link
    Member

    tiran commented Mar 30, 2021

    Serhiy was right, this is a security issue.

    The patch should not have landed in 3.8. At a bare minimum the patch should have been postponed until documentation was updated. Since 3.8 the ipaddresss does not behave as documented. A similar security issue in NPM was published two days ago, CVE-2021-28918.

    I proposed to not only revert the change, but also tighten the check for leading zeros so it behaves like glibc's inet_pton(). It refuses any IPv4 string with a leading zero.

    >>> socket.inet_pton(socket.AF_INET, "01.1.1.1")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: illegal IP address string passed to inet_pton
    >>> socket.inet_pton(socket.AF_INET, "1.1.1.01")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: illegal IP address string passed to inet_pton

    @tiran tiran added stdlib Python modules in the Lib dir 3.9 only security fixes 3.10 only security fixes type-security A security issue and removed type-bug An unexpected behavior, bug, or error labels Mar 30, 2021
    @vstinner
    Copy link
    Member

    The patch should not have landed in 3.8. At a bare minimum the patch should have been postponed until documentation was updated. Since 3.8 the ipaddresss does not behave as documented. A similar security issue in NPM was published two days ago, CVE-2021-28918.

    Link: https://sick.codes/sick-2021-011

    @ambv
    Copy link
    Contributor

    ambv commented Apr 2, 2021

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

    @zooba
    Copy link
    Member

    zooba commented Apr 3, 2021

    (Copied from my comment on the PR, following the one where I said this was ready to go.)

    Withdrawing the readiness - @ambv and I would prefer to see this behind a flag (probably "strict" parsing), on by default for 3.10, and maybe on by default for 3.9/earlier.

    The main reasoning being that this isn't our vulnerability, but an inconsistency with other vulnerable libraries. The current fix is the best it can be, but it doesn't prevent the vulnerability, it just causes Python to break first. So it ought to be relatively easy to retain the flexible (though admittedly non-sensical) behaviour for those who currently rely on it.

    @ambv ambv removed the 3.8 only security fixes label May 2, 2021
    @ambv
    Copy link
    Contributor

    ambv commented May 2, 2021

    New changeset 60ce8f0 by Christian Heimes in branch 'master':
    bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated (GH-25099)
    60ce8f0

    @ambv
    Copy link
    Contributor

    ambv commented May 2, 2021

    New changeset 5374fbc by Miss Islington (bot) in branch '3.9':
    bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated (GH-25099) (GH-25815)
    5374fbc

    @tiran
    Copy link
    Member

    tiran commented May 2, 2021

    Łukasz, thanks for pushing the PR over the finish line!

    @ned-deily
    Copy link
    Member

    Is there anything more to be done for this issue or can it be closed?

    @pablogsal
    Copy link
    Member

    I'm closing this, if someone thinks something is missing, please, reopen

    @vstinner
    Copy link
    Member

    I created https://python-security.readthedocs.io/vuln/ipaddress-ipv4-leading-zeros.html to track this vulnerability.

    Python 3.8 is left unchanged (accept leading zeros). Python 3.7 and older are not affected.

    @gcbirzan
    Copy link
    Mannequin

    gcbirzan mannequin commented May 25, 2021

    The timeline there is wrong. This issue's creation time isn't the disclosure time, it's when the bug was introduced. The disclosure was on 30th of May, when I emailed security@python.org and Christian Heimes commented here and made #25099. Even though Serhiy Storchaka commented that this could be a security issue back when the issue was new, the date would be 30th of March 2019, not 20th.

    @vstinner
    Copy link
    Member

    George-Cristian Bîrzan: "The timeline there is wrong."

    Fixed: https://python-security.readthedocs.io/vuln/ipaddress-ipv4-leading-zeros.html#timeline

    The strange part is "2019-03-20 (-741 days): Python issue bpo-36384 reported by Joel Croteau".

    The problem is that this issue was "reused" for two different things: the initial change and the vulnerability.

    Maybe I can removed the reference to the bpo to remove it from the timeline (and put it in links).

    @gcbirzan
    Copy link
    Mannequin

    gcbirzan mannequin commented May 25, 2021

    I think the only thing I'd improve would be to mention that this issue is the one that introduced the bug, otherwise it looks a bit weird.

    @vstinner
    Copy link
    Member

    I think the only thing I'd improve would be to mention that this issue is the one that introduced the bug, otherwise it looks a bit weird.

    Ok, done: https://python-security.readthedocs.io/vuln/ipaddress-ipv4-leading-zeros.html#timeline

    @achraf-mer
    Copy link
    Mannequin

    achraf-mer mannequin commented Aug 17, 2021

    Can we backport the security fix from this issue https://bugs.python.org/issue36384#msg392684 to version 3.8
    The comment explicitly says that it was decided to not include in 3.8, I am not sure this is best, since it prevents using 3.8 because of this open vulnerability, and it does not seem to be a breaking change or too hard to port.

    @achraf-mer achraf-mer mannequin added 3.8 only security fixes and removed docs Documentation in the Doc dir 3.9 only security fixes 3.10 only security fixes labels Aug 17, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Aug 17, 2021

    it prevents using 3.8 because of this open vulnerability

    What do you mean by this?

    Our understanding is that this is a low-severity CVE because in order for this to be a vulnerability, you'd have to have both:

    1. user access to IP address input; and
    2. control over two addresses sharing numerical representation with leading zeroes: the first resolving when leading zeroes are treated as octal numbers; the second resolving when leading zeroes are treated as decimal numbers.

    Access to both then allows you at best to circumvent IP address-based access control or denial of service. However, access to just 1. allows you to input any IP address to achieve the same goals.

    Hence low-severity.

    it does not seem to be a breaking change

    It is a bona fide breaking change. Any IP address configuration saved in files or databases which might have used leading zeroes would be rejected by 3.8.12. The same was true for 3.9.5 but since this release series has much higher exposure (still receiving binary installers and regular-cadence bugfixes), it was less controversial to include it.

    If you still feel this ought to be fixed in 3.8, please elaborate.

    @achraf-mer
    Copy link
    Mannequin

    achraf-mer mannequin commented Aug 17, 2021

    > it prevents using 3.8 because of this open vulnerability

    What do you mean by this?

    Our understanding is that this is a low-severity CVE because in order for this to be a vulnerability, you'd have to have both:

    1. user access to IP address input; and
    2. control over two addresses sharing numerical representation with leading zeroes: the first resolving when leading zeroes are treated as octal numbers; the second resolving when leading zeroes are treated as decimal numbers.

    Access to both then allows you at best to circumvent IP address-based access control or denial of service. However, access to just 1. allows you to input any IP address to achieve the same goals.

    Hence low-severity.

    Even though I agree with you assessment on the root cause of the issue itself, it is listed as critical in https://nvd.nist.gov/vuln/detail/CVE-2021-29921, which means most commercial scan tools will also flag python 3.8 as critical, and this could prevent users from going with python 3.8 on production. (our case too)

    > it does not seem to be a breaking change

    It is a bona fide breaking change. Any IP address configuration saved in files or databases which might have used leading zeroes would be rejected by 3.8.12. The same was true for 3.9.5 but since this release series has much higher exposure (still receiving binary installers and regular-cadence bugfixes), it was less controversial to include it.

    If you still feel this ought to be fixed in 3.8, please elaborate.

    IMHO I still think this should be solved in 3.8, otherwise there is really no other alternative but to upgrade to python 3.9 which is a hassle, since all 3.8.x are "critically vulnerable", had the CVE in https://nvd.nist.gov/vuln/detail/CVE-2021-29921 not been marked as critical, then we could have used python 3.8 knowing the two conditions you mentioned earlier.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 17, 2021

    I was unaware of the "CRITICAL" base score assigned by NIST to this. Alright, let's port this back then. There are a few things the PR will need.

    @tiran
    Copy link
    Member

    tiran commented Aug 17, 2021

    "CRITICAL" is a ridiculous high assessment for this bug. Somebody ticked all the scary boxes in the CVSS form like "total loss of control".

    @tiran
    Copy link
    Member

    tiran commented Aug 17, 2021

    @ambv
    Copy link
    Contributor

    ambv commented Aug 17, 2021

    New changeset 03dd89d by achraf-mer in branch '3.8':
    [3.8] bpo-36384: Leading zeros in IPv4 addresses are no longer tolerated (GH-25099) (GH-27801)
    03dd89d

    @ambv ambv added 3.9 only security fixes 3.10 only security fixes labels Aug 19, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Aug 19, 2021

    New changeset 6ebfe8d by Łukasz Langa in branch '3.8':
    [3.8] bpo-36384: [doc] Correct typos in CVE-2021-29921 fix description (GH-27825)
    6ebfe8d

    @ambv
    Copy link
    Contributor

    ambv commented Aug 19, 2021

    New changeset 0fd66e4 by Łukasz Langa in branch 'main':
    bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 (GH-27824)
    0fd66e4

    @miss-islington
    Copy link
    Contributor

    New changeset 1204dfc by Miss Islington (bot) in branch '3.10':
    bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 (GH-27824)
    1204dfc

    @ambv
    Copy link
    Contributor

    ambv commented Aug 19, 2021

    New changeset 9882025 by Miss Islington (bot) in branch '3.9':
    bpo-36384: [doc] Mention CVE-2021-29921 fix in 3.8.12 (GH-27824) (GH-27827)
    9882025

    @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.8 only security fixes 3.9 only security fixes 3.10 only security fixes deferred-blocker stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests