[XERCESC-2188] Fix potential double-free in usage of ReaderMgr::pushReader() #47
Conversation
The fix consists in adding a new argument to pushReader() to specify if ReaderMgr must own the passed entity, and adapt callers to specify the right value of this ownership flag depending on the calling context. SPDX-FileCopyrightText: Portions Copyright 2021 Siemens Modified on 15-Jul-2021 by Siemens and/or its affiliates to fix CVE-2018-1311: Apache Xerces-C use-after-free vulnerability scanning external DTD. Copyright 2021 Siemens. Co-authored-by: Even Rouault <even.rouault@spatialys.com>
|
Seeing now https://issues.apache.org/jira/projects/XERCESC/issues/XERCESC-2188 , I see my approach is close to a suggestion of https://issues.apache.org/jira/browse/XERCESC-2188?focusedCommentId=17055399&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17055399 |
|
Hi Rouault,
I cant see how my changes could cause a test regression, all my change does is delete 4 smart pointers that are not used in the code base.
Kind regards
John
From: Even Rouault ***@***.***>
Sent: 23 January 2022 15:34
To: apache/xerces-c ***@***.***>
Cc: McCann, John (DI SW PE OT IO PP) ***@***.***>; Mention ***@***.***>
Subject: Re: [apache/xerces-c] [XERCESC-2188] Fix potential double-free in usage of ReaderMgr::pushReader() (PR #47)
Seeing now https://issues.apache.org/jira/projects/XERCESC/issues/XERCESC-2188 , I see my approach is close to a suggestion of https://issues.apache.org/jira/browse/XERCESC-2188?focusedCommentId=17055399&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17055399
—
Reply to this email directly, view it on GitHub<#47 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXMN5WD5SDILV22VLP5YNFLUXQNXNANCNFSM5MTPEBCQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
…-----------------
Siemens Industry Software Limited is a limited company registered in England and Wales.
Registered number: 3476850.
Registered office: Pinehurst 2, Pinehurst Road, Farnborough, Hampshire, GU14 7BF.
|
yes, but that causes a memory leak since nobody would take care of freeing the declDTD object. Hence my extra changes |
|
Ah yes I can see that now, thanks for picking up on that. Do I need to add that change to my fork or has it been added in your fork which is later than mine?
Thanks and kind regards
John
From: Even Rouault ***@***.***>
Sent: 25 January 2022 14:15
To: apache/xerces-c ***@***.***>
Cc: McCann, John (DI SW PE OT IO PP) ***@***.***>; Mention ***@***.***>
Subject: Re: [apache/xerces-c] [XERCESC-2188] Fix potential double-free in usage of ReaderMgr::pushReader() (PR #47)
I cant see how my changes could cause a test regression, all my change does is delete 4 smart pointers that are not used in the code base.
yes, but that causes a memory leak since nobody would take care of freeing the declDTD object. Hence my extra changes
—
Reply to this email directly, view it on GitHub<#47 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXMN5WGUPIVCZD6K2NQ2IYTUX2V5XANCNFSM5MTPEBCQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
…-----------------
Siemens Industry Software Limited is a limited company registered in England and Wales.
Registered number: 3476850.
Registered office: Pinehurst 2, Pinehurst Road, Farnborough, Hampshire, GU14 7BF.
|
yes |
|
So just to confirm there is nothing you need me to do to get this fix in the code base?
From: Even Rouault ***@***.***>
Sent: 01 February 2022 14:02
To: apache/xerces-c ***@***.***>
Cc: McCann, John (DI SW PE OT IO PP) ***@***.***>; Mention ***@***.***>
Subject: Re: [apache/xerces-c] [XERCESC-2188] Fix potential double-free in usage of ReaderMgr::pushReader() (PR #47)
or has it been added in your fork which is later than mine?
yes
—
Reply to this email directly, view it on GitHub<#47 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXMN5WBMXAP4Q4D7FUWXLZLUY7RWZANCNFSM5MTPEBCQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
…-----------------
Siemens Industry Software Limited is a limited company registered in England and Wales.
Registered number: 3476850.
Registered office: Pinehurst 2, Pinehurst Road, Farnborough, Hampshire, GU14 7BF.
|
no, we just need someone with commit rights in this repository to review & merge it |
|
@rleigh-codelibre can you merge this PR and make a new release? |
|
@scantor this vulnerability was reported almost 4 years ago. It has to be finally fixed. |
|
Since you addressed me personally, I can simply reiterate as I have in Jira (which is where this proposal should be, this is not a GitHub project) that I don't have any exposure to, and thus no source of resources with which to work on, anything in the DTD code unless it's a trivial fix that doesn't change the ABI and I'm already doing some other work on the code. Nothing else has arisen with the code that necessitated a release for my project, so there hasn't been any opportunity for me to look at anything else. I am not stopping anybody else from doing the work, and nobody is stopping others from joining the project as committers, which is certainly needed for obvious reasons. One issue that's perhaps less obvious is that a fix that requires a 4.0 rev may not get uptake by the few distributors of the current version. I imagine that's why Red Hat took the approach they took with it and just made it leak memory instead. Perhaps that's the best option in the end after all. I really have not looked at the issue at all in any depth to understand the trade-offs or possible fixes. |
|
I am not sufficiently familiar with this part of the codebase to review it meaningfully, but the changes look good and the unit tests are passing and not reporting any leaks, so I think merging this should be fairly risk-free. Regarding making a new release, all of the recent bugfixes will need backporting to the 3.2 branch if we want to have a new 3.2 point release with all of these changes included. There are quite a few to backport thanks to all the work done recently, primarily by @rouault. @scantor Would you be able to make the release? I can probably find some time to do the backporting, unless you want to do this. |
|
I looked at the fix last night at least in cursory fashion. It can't be backported to 3.2 because it's an API and therefore ABI change. Given some method defaulting it could probably be a 3.3 since it would be backwardly-compatible. As with you, I have absolutely no idea if the fix is either sufficient or doesn't break anything. I don't really care for my own purposes so would defer to others on whether to accept the patch given that lack of insight into its correctness. As for doing a release, not really, no. It would be very unlikely for me to find any time to do so until some time later this year, possibly in the Spring. And I can't promise that. |
|
Hello Scott Cantor, Did this hot fix make it into the xerces code base? I think previously you alluded to the possibility of having this in the code base for a release in spring this year (although you never committed to that)? Did that happen or is this PR still in the review stage? Thanks and kind regards John |
|
No, and no, I have no expectation of any releases. If a security issue that actually affects my code comes up I would probably apply this and bump to 3.3. This cannot be part of a patch to 3.2, as I said. This project needs active committers that have the time allocated to work on it. Until it gets some, it's going to stay moribund. If you need this fix, I would definitely suggest that you consider becoming one or find somebody else who is able to. If that happens, I am willing to help that person or persons get through the release process. |
| fCurReader = fReaderStack->pop(); | ||
| fCurEntity = fEntityStack->pop(); | ||
|
|
||
| popReaderAndEntity(); |
There was a problem hiding this comment.
The call to popReaderAndEntity may delete fCurEntity. This will result in the deleted pointer being passed to the EndOfEntityException through prevEntity resulting in potential issues when this pointer is dereferenced in the code that handles the exception.
|
Hello Rouault, Has this fix made its way into the xerces code base? Kind regards John |
|
It has not, and I don't think it's even known that the fix is correct. |
|
Thanks for your response Scott, How does it get to be known that the "fix is correct."? It appears that the tests are passing and there are no regressions. This hotfix is really important for one of our customers, so we would like to work with you to get it into the codebase. Looking forward to your response John |
|
There are nowhere near adequate tests to treat that as meaningful, it's merely a data point. It's known when somebody who knows the code reviews the change for correctness. There are no active committers left who know the code, QED. There's a comment above that makes a case that this fix introduces a bug, i.e. a regression. Maybe it does, maybe it doesn't. That's plenty good enough for me to treat it as suspect. As for your customer, if you need the project to be viable, then you need to provide ongoing committers to make it viable. |
|
Apache-496067-disclosure-report.pdf Hello Scott here is the vulnerability report as reported by the UK National Cyber Security Center, which outlines the vulnerability and even mentions the problematic lines which are part of the #47 thread I have noted that @rleigh-codelibre comment on Feb 2, 2022 which states "the changes look good and the unit tests are passing and not reporting any leaks, so I think merging this should be fairly risk-free." I will consider becoming a committer to this project to fix this vulnerability Kind regards John |
Only if you're in it for the long haul, it's a commitment (pun intended) to actually sustain the code base, not just a means of getting one fix applied. |
|
FYI: #54 |
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
These are the instructions for observing the bug (before this commit): $ git clone https://github.com/apache/xerces-c.git $ cd xerces-c $ mkdir build $ cd build $ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8 $ cp ../samples/data/personal.xml . $ cat <<EOF >personal.dtd <?xml encoding="ISO-8859-1"?> <!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity; EOF $ gdb samples/StdInParse (gdb) b IGXMLScanner.cpp:1544 (gdb) run <personal.xml 1544 fReaderMgr.pushReader(reader, declDTD); (gdb) p declDTD $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) n 1547 dtdScanner.scanExtSubsetDecl(false, true); (gdb) n 1548 } (gdb) s ... (gdb) s # The Janitor is about to delete the above declDTD. 90 delete fData; (gdb) p fData $1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68 (gdb) b ReaderMgr.cpp:1024 (gdb) n ... (gdb) n # Now we about to dereference the deleted declDTD. 1024 if (curEntity && !curEntity->isExternal()) (gdb) p curEntity $2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68 Origin: apache/xerces-c@e002426 Bug: apache/xerces-c#47 Bug: apache/xerces-c#54 Bug: https://issues.apache.org/jira/browse/XERCESC-2188 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2018-1311 Bug-Debian: https://bugs.debian.org/947431 Gbp-Pq: Name CVE-2018-1311.patch
The fix consists in adding a new argument to pushReader() to specify if
ReaderMgr must own the passed entity, and adapt callers to specify the
right value of this ownership flag depending on the calling context.
SPDX-FileCopyrightText: Portions Copyright 2021 Siemens
Modified on 15-Jul-2021 by Siemens and/or its affiliates to fix CVE-2018-1311: Apache Xerces-C use-after-free vulnerability scanning external DTD. Copyright 2021 Siemens.
Co-authored-by: Even Rouault even.rouault@spatialys.com
Supersedes #46 (avoids the memory leak in the unit tests)
@johnjamesmccann Do you have access to a reproducer to confirm it fixes the issue ? I couldn't easily find a reproducer