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

xmlparse_setattro() Type Confusion #69207

Closed
JohnLeitch mannequin opened this issue Sep 7, 2015 · 3 comments
Closed

xmlparse_setattro() Type Confusion #69207

JohnLeitch mannequin opened this issue Sep 7, 2015 · 3 comments
Assignees
Labels
topic-XML type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@JohnLeitch
Copy link
Mannequin

JohnLeitch mannequin commented Sep 7, 2015

BPO 25019
Nosy @tiran, @serhiy-storchaka
Files
  • xmlparse_setattro_Type_Confusion.py: repro
  • xmlparse_setattro_Type_Confusion.patch: 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-09-07.20:09:57.923>
    created_at = <Date 2015-09-07.15:56:02.238>
    labels = ['expert-XML', 'type-crash']
    title = 'xmlparse_setattro() Type Confusion'
    updated_at = <Date 2015-09-18.12:23:21.777>
    user = 'https://bugs.python.org/JohnLeitch'

    bugs.python.org fields:

    activity = <Date 2015-09-18.12:23:21.777>
    actor = 'Arfrever'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-09-07.20:09:57.923>
    closer = 'serhiy.storchaka'
    components = ['XML']
    creation = <Date 2015-09-07.15:56:02.238>
    creator = 'JohnLeitch'
    dependencies = []
    files = ['40394', '40395']
    hgrepos = []
    issue_num = 25019
    keywords = ['patch']
    message_count = 3.0
    messages = ['250116', '250124', '250126']
    nosy_count = 6.0
    nosy_names = ['christian.heimes', 'Arfrever', 'python-dev', 'serhiy.storchaka', 'JohnLeitch', 'brycedarling']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue25019'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

    @JohnLeitch
    Copy link
    Mannequin Author

    JohnLeitch mannequin commented Sep 7, 2015

    Python 3.4 and 3.5 suffer from a vulnerability caused by the behavior of the xmlparse_setattro() function. When called, the function uses the provided name argument in several conditional statements which assume that the name argument is a string.

    However, if a name argument is provided that is not a string, this logic will make several calls to PyUnicode_CompareWithASCIIString that expect a string, yet receive some other type of object, leading to a type confusion vulnerability:

    static int
    xmlparse_setattro(xmlparseobject *self, PyObject *name, PyObject *v)
    {
        /* Set attribute 'name' to value 'v'. v==NULL means delete */
        if (v == NULL) {
            PyErr_SetString(PyExc_RuntimeError, "Cannot delete attribute");
            return -1;
        }
        assert(PyUnicode_Check(name));
        if (PyUnicode_CompareWithASCIIString(name, "buffer_text") == 0) {
        [...]
    }
    	

    In some applications, it may be possible to exploit this behavior to achieve arbitrary code execution. The type confusion can be observed by running the following script:

    from xml.parsers.expat import *
    p = ParserCreate()
    p.__setattr__(range(0xF), 0)

    Which, depending on the arrangement of memory, may produce an exception such as this:

    0:000> g
    (d84.ce0): Access violation - code c0000005 (first chance)
    First chance exceptions are reported before any exception handling.
    This exception may be expected and handled.
    eax=0086f904 ebx=0086f8fc ecx=0050005c edx=00b60138 esi=0050005e edi=00b60138
    eip=61e9a967 esp=0086f8c8 ebp=0086f8e0 iopl=0 nv up ei ng nz na pe cy
    cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00010287
    python35!find_maxchar_surrogates+0x37:
    61e9a967 0fb701 movzx eax,word ptr [ecx] ds:002b:0050005c=????
    0:000> k3
    ChildEBP RetAddr
    0086f8e0 61e9aa35 python35!find_maxchar_surrogates+0x37 [c:\build\cpython\objects\unicodeobject.c @ 1417]
    0086f908 61eabcf3 python35!_PyUnicode_Ready+0x35 [c:\build\cpython\objects\unicodeobject.c @ 1466]
    0086f918 61c547c3 python35!PyUnicode_CompareWithASCIIString+0x13
    [c:\build\cpython\objects\unicodeobject.c @ 10784]

    To fix this issue, it is recommended that xmlparse_setattro() be updated to validate that the name argument is a string and return out of the function early if it is not. A proposed patch is attached.

    Credit: John Leitch (johnleitch@outlook.com), Bryce Darling (darlingbryce@gmail.com)

    @JohnLeitch JohnLeitch mannequin added type-security A security issue topic-XML labels Sep 7, 2015
    @serhiy-storchaka serhiy-storchaka self-assigned this Sep 7, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 7, 2015

    New changeset ff2c4f281720 by Serhiy Storchaka in branch '3.4':
    Issue bpo-25019: Fixed a crash caused by setting non-string key of expat parser.
    https://hg.python.org/cpython/rev/ff2c4f281720

    New changeset 6006231dcaae by Serhiy Storchaka in branch '3.5':
    Issue bpo-25019: Fixed a crash caused by setting non-string key of expat parser.
    https://hg.python.org/cpython/rev/6006231dcaae

    New changeset edf25acae637 by Serhiy Storchaka in branch 'default':
    Issue bpo-25019: Fixed a crash caused by setting non-string key of expat parser.
    https://hg.python.org/cpython/rev/edf25acae637

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your contribution John.

    The committed patch slightly differs from the proposed patch. Error message now is the same as in setattr() and general __setattr__(). Tests are moved to existing test class for testing of attribute setting. Improved tests for valid attributes.

    @serhiy-storchaka serhiy-storchaka added type-crash A hard crash of the interpreter, possibly with a core dump and removed type-security A security issue labels Sep 7, 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
    topic-XML type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant