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

Arbitrary code execution vulnerability due to unchecked eval() call in dumbdbm module #67074

Closed
StephenFarris mannequin opened this issue Nov 16, 2014 · 11 comments
Closed
Assignees
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@StephenFarris
Copy link
Mannequin

StephenFarris mannequin commented Nov 16, 2014

BPO 22885
Nosy @malemburg, @gvanrossum, @rhettinger, @vstinner, @bitdancer, @PCManticore, @serhiy-storchaka, @gvanrossum
Files
  • issue22885.patch
  • issue22885_1.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-02-15.22:40:44.914>
    created_at = <Date 2014-11-16.18:39:00.954>
    labels = ['type-security', 'library']
    title = 'Arbitrary code execution vulnerability due to unchecked eval() call in dumbdbm module'
    updated_at = <Date 2015-02-22.16:12:27.930>
    user = 'https://bugs.python.org/stephenfarris'

    bugs.python.org fields:

    activity = <Date 2015-02-22.16:12:27.930>
    actor = 'Arfrever'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-02-15.22:40:44.914>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-11-16.18:39:00.954>
    creator = 'stephen.farris'
    dependencies = []
    files = ['37812', '37813']
    hgrepos = []
    issue_num = 22885
    keywords = ['patch']
    message_count = 11.0
    messages = ['231255', '234446', '234447', '234450', '234547', '234551', '234598', '234599', '234600', '236073', '236075']
    nosy_count = 11.0
    nosy_names = ['lemburg', 'gvanrossum', 'rhettinger', 'vstinner', 'Arfrever', 'r.david.murray', 'Claudiu.Popa', 'python-dev', 'serhiy.storchaka', 'Guido.van.Rossum', 'stephen.farris']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue22885'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @StephenFarris
    Copy link
    Mannequin Author

    StephenFarris mannequin commented Nov 16, 2014

    The dumbdbm module uses an unchecked call to eval() in the _update method, which is called in response to a call to dumbdbm.open(), and is used to load the index from the directory file.  This poses a security vulnerability because it allows an attacker to execute arbitrary code on the victim's machine by inserting python code into the DBM directory file.  This vulnerability could allow an attacker to execute arbitrary commands on the victim machine, potentially allowing them to deploy malware, gain system access, destroy files and data, expose sensitive information, etc.

    @StephenFarris StephenFarris mannequin added stdlib Python modules in the Lib dir type-security A security issue labels Nov 16, 2014
    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Jan 21, 2015

    Here's a patch which uses ast.literal_eval instead. This doesn't get code executed, since literal_eval will fail loudly for anything other than a literal. There are some issues to consider:

    • let the current ast.literal_eval call bubble out with a lot of different exceptions
    • normalize the exception to dbm.dumb.error.

    I'm leaning towards the first, since it clearly shows that something bad happened in the module and it's a first indicator that someone tampered with the data file.

    @gvanrossum
    Copy link
    Member

    Python 3's exception chaining allows us to do the second (easier to catch without resorting to "except Exception:" or even "except:") while still showing the original exception in the traceback.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Jan 21, 2015

    Thanks for the tip, Guido. The new patch uses exception chaining. If this needs backporting, most probably the first patch can be used.

    @vstinner
    Copy link
    Member

    + with open(_fname + ".dir", 'w') as stream:
    + stream.write(content)

    I don't see where the created file is deleted. Add something like:

    self.addCleanup(support.unlink, _fname + ".dir")

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Jan 23, 2015

    Thanks, Victor. I thought the same thing, but the file is deleted here already, here: https://hg.python.org/cpython/file/981ba93bcbde/Lib/test/test_dbm_dumb.py#l228

    @serhiy-storchaka
    Copy link
    Member

    Raising dbm.dumb.error is behavior change. It would be safer not apply this part in maintained releases.

    If add sanity checks in 3.5, note that following line "key = key.encode('Latin-1')" can raise an exception too (AttributeError or UnicodeEncodeError). And incorrect data can cause an error later in __getitem__ if pos_and_siz_pair is not a pair of two integers.

    I think it is worth to split this issue on two issues and fix only security issue here.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Jan 24, 2015

    Thanks, Serhiy. Only the security issue is fixed in this patch, since eval is replaced by ast.literal_eval and nothing else. Indeed, if the data is something else than what dbm expects after ast.literal_eval, then it would fail, but it would have failed previously too.

    @serhiy-storchaka
    Copy link
    Member

    I mean that raising dbm.dumb.error is different issue unrelated to changing eval to ast.literal_eval. See also Raymond's objections in bpo-21708.

    bpo-22885.patch LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 15, 2015

    New changeset 02865d22a98d by Serhiy Storchaka in branch '2.7':
    Issue bpo-22885: Fixed arbitrary code execution vulnerability in the dumbdbm
    https://hg.python.org/cpython/rev/02865d22a98d

    New changeset 693bf15b4314 by Serhiy Storchaka in branch '3.4':
    Issue bpo-22885: Fixed arbitrary code execution vulnerability in the dbm.dumb
    https://hg.python.org/cpython/rev/693bf15b4314

    New changeset cf6a62b0ef3b by Serhiy Storchaka in branch 'default':
    Issue bpo-22885: Fixed arbitrary code execution vulnerability in the dbm.dumb
    https://hg.python.org/cpython/rev/cf6a62b0ef3b

    @serhiy-storchaka
    Copy link
    Member

    Committed bpo-22885.patch with modified test which demonstrates vulnerability of unpatched dbm.dumb. If you want to change exception type raised by dbm.dumb, you can open new issue.

    @serhiy-storchaka serhiy-storchaka self-assigned this Feb 15, 2015
    @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
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants