Description

insistxc

opened on Jan 12, 2024

Issue body actions

Bug report

Bug description:

def _EndRecData(fpin):
    """Return data from the "End of Central Directory" record, or None.

    The data is a list of the nine items in the ZIP "End of central dir"
    record followed by a tenth item, the file seek offset of this record."""

    # Determine file size
    fpin.seek(0, 2)
    filesize = fpin.tell()

    # Check to see if this is ZIP file with no archive comment (the
    # "end of central directory" structure should be the last item in the
    # file if this is the case).
    try:
        fpin.seek(-sizeEndCentDir, 2)
    except OSError:
        return None
    data = fpin.read()
    if (len(data) == sizeEndCentDir and
        data[0:4] == stringEndArchive and
        data[-2:] == b"\000\000"):

When checking whether a file is a zip file, MemoryError was triggered, followed by OOM. After investigation, it was found that it was a read() read exception.

Through PDB debugging, it was found that a link file was read, which points to /proc/kcore, why does the existing zip file check not determine whether it is a zip file by reading the header byte (504B0304) of the file .

I think the existing judgment ZIP method does not limit the read reading. When reading a non -normal file, it may cause the system to collapse .

Hope to be resolved.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-113977, gh-120754: Remove unbounded reads from zipfile #122101
• [3.12] gh-113977, gh-120754: Remove unbounded reads from zipfile (GH-122101) #126347
• [3.13] gh-113977, gh-120754: Remove unbounded reads from zipfile (GH-122101) #126348

Activity

1. insistxc

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   on Jan 12, 2024
2. ronaldoussoren commented on Jan 12, 2024

   

   ronaldoussoren

   on Jan 12, 2024

   Contributor

   More actions

   That code already tries to check if the file is a zipfile by reading the header at the end of the file. The MemoryError seems to indicate that seeking to the end of the file doesn't work as expected for /proc/kcore.

   This function could be written a bit more defensively though, for example by using fpin.read(sizeEndCentDir+1) instead of fpin.read().

   Reacted by sunmy2019 and Cody Maloney
3. ronaldoussoren

   added

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   on Jan 12, 2024
4. serhiy-storchaka

   added this to Zipfile issues 📦on Jan 12, 2024
5. danifus commented on Aug 5, 2024

   

   danifus

   on Aug 5, 2024

   Contributor

   More actions

   This implements the suggested change: #122101

   Reacted by Cody Maloney
6. cmaloney commented on Aug 7, 2024

   

   cmaloney

   on Aug 7, 2024

   Contributor

   More actions

   There's a secondary thing here, that unbounded read defaults to the size at open with #120755. In this bug that's more than the amount of RAM remaining on the machine, hence the MemoryError / OOM. That stashed size is currently invalidated on .truncate() but not on .seek() which zipfile uses. I'd like to make seek() clear the estimated size, but that change conflicts a lot with #121593, so have been holding off.

   #122101 I think resolves this case for zipfile (and makes it more predictable, safer behavior generally) but I suspect the .seek() case will come up more in library code, so before python 3.14 would like to get that to invalidate the cached size generally.
7. picnixz

   changed the title [-]When checking whether a file is a zip file, a MemoryError was triggered. After investigation, it was found that it was a read() read exception.[/-] [+]Unbounded reads by `zipefile` may cause a `MemoryError`.[/+] on Nov 2, 2024
8. picnixz

   added

   3.12only security fixesonly security fixes

   3.13only security fixesonly security fixes

   3.14bugs and security fixesbugs and security fixes

   on Nov 2, 2024
9. picnixz commented on Nov 2, 2024

   

   picnixz

   on Nov 2, 2024

   Member

   More actions

   Are (potential) DDoS like that considered as security issues or not? @gpshead
10. gpshead

    changed the title [-]Unbounded reads by `zipefile` may cause a `MemoryError`.[/-] [+]Unbounded reads by `zipfile` may cause a `MemoryError`.[/+] on Nov 3, 2024
11. gpshead

    self-assigned this

    on Nov 3, 2024
12. gpshead

    added a commit that references this issue on Nov 3, 2024

    gh-113977, gh-120754: Remove unbounded reads from zipfile (GH-122101)

    556dc9b
13. miss-islington

    added 2 commits that reference this issue on Nov 3, 2024

    pythongh-113977, pythongh-120754: Remove unbounded reads from zipfile (…

    7ea6fd2

    pythongh-113977, pythongh-120754: Remove unbounded reads from zipfile (…

    f17af1e
14. gpshead commented on Nov 3, 2024

    

    gpshead

    on Nov 3, 2024

    Member

    More actions

    Are (potential) DDoS like that considered as security issues or not? @gpshead

    They can be, but not particularly severe. I don't think this one is worth considering a security problem because the circumstances in which it can occur are extremely limited: Someone has to have opened a file that either isn't seekable or provides an egregious amount of data upon read after seeking to the end.

    Just constructing that scenario in the first place is a sign of greater problems in the system.

    @sethmlarson as FYI

    Reacted by Cody Maloney
15. gpshead commented on Nov 3, 2024

    

    gpshead

    on Nov 3, 2024

    Member

    More actions

    Thanks for the nice bug report and PRs. The 3.12 and 3.13 back ports are also set to auto merge. Indeed, avoiding unbounded read assumptions is the right coding practice.
16. gpshead

    closed this as completedon Nov 3, 2024
17. github-project-automation

    moved this to Done in Zipfile issues 📦on Nov 3, 2024
18. gpshead

    added 2 commits that reference this issue on Nov 3, 2024

    [3.12] gh-113977, gh-120754: Remove unbounded reads from zipfile (GH-…

    6e3e91f

    [3.13] gh-113977, gh-120754: Remove unbounded reads from zipfile (GH-…

    7435127
19. picnixz

    added a commit that references this issue on Dec 8, 2024

    pythongh-113977, pythongh-120754: Remove unbounded reads from zipfile (…

    b7046ab
20. ebonnal

    added a commit that references this issue on Jan 12, 2025

    pythongh-113977, pythongh-120754: Remove unbounded reads from zipfile (…

    22db78f

Issue actions