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

[XERCESC-2241] Fix integer overflows in DFAContentModel class #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Oct 2, 2022

On .xsd files like the following ones (generated by ossfuzz, so broken), integer overflows can happen in DFAContentModel::countLeafNodes() and DFAContentModel::buildDFA() which can later cause out-of-bounds access.

Found in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52025

<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema"
           xmlns:myns="http://myns"
           targetNamespace="http://myns"
           elementFormDefault="qualified" attributeFormDefault="unqualified">

<xs:element name="main_elt">
  <xs:complexType>
     <xs:sequence>
        <xs:group ref="myns:mygroup" minOccurs="32767" maxOccurs="1"/>
      </xs:sequence>
  </xs:complexType>
</xs:element>

<xs:group name="mygroup">
  <xs:sequence>
      <!-- related to https://issues.apache.org/jira/browse/XERCESC-1051 -->
      <xs:element name="elt" maxOccurs="33333">
        <xs:complexType>
            <xs:sequence>
 ame="x" type="xs:int" maxOccurs="1"/>
            </xs:sequence>
        </xs:complexType>
      </xs:element>
  </xs:sequence>
</xs:group>

</xs:schema>

@rouault
Copy link
Contributor Author

rouault commented Oct 3, 2022

CC @rleigh-codelibre This should be relatively safe to apply

Copy link
Contributor

@rleigh-codelibre rleigh-codelibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of the change all looks fine, but a query regarding the type used for numeric_limits::max().

Thanks,
Roger

src/xercesc/validators/common/DFAContentModel.cpp Outdated Show resolved Hide resolved
src/xercesc/validators/common/DFAContentModel.cpp Outdated Show resolved Hide resolved
src/xercesc/validators/common/DFAContentModel.cpp Outdated Show resolved Hide resolved
src/xercesc/validators/common/DFAContentModel.cpp Outdated Show resolved Hide resolved
On .xsd files like the following ones (generated by ossfuzz, so broken),
integer overflows can happen in DFAContentModel::countLeafNodes() and
DFAContentModel::buildDFA() which can later cause out-of-bounds access.

Found in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52025

```
<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema"
           xmlns:myns="http://myns"
           targetNamespace="http://myns"
           elementFormDefault="qualified" attributeFormDefault="unqualified">

<xs:element name="main_elt">
  <xs:complexType>
     <xs:sequence>
        <xs:group ref="myns:mygroup" minOccurs="32767" maxOccurs="1"/>
      </xs:sequence>
  </xs:complexType>
</xs:element>

<xs:group name="mygroup">
  <xs:sequence>
      <!-- related to https://issues.apache.org/jira/browse/XERCESC-1051 -->
      <xs:element name="elt" maxOccurs="33333">
        <xs:complexType>
            <xs:sequence>
 ame="x" type="xs:int" maxOccurs="1"/>
            </xs:sequence>
        </xs:complexType>
      </xs:element>
  </xs:sequence>
</xs:group>

</xs:schema>
```
@rleigh-codelibre rleigh-codelibre self-requested a review October 3, 2022 18:54
Copy link
Contributor

@rleigh-codelibre rleigh-codelibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, this all looks to be correct to me. I'll merge it once the CI has passed (for the CI build which isn't broken).

@scantor
Copy link
Contributor

scantor commented Oct 6, 2022

@rleigh-codelibre If you can apply this to master that will make it easier for me to cherry-pick back to the branch.

@scantor
Copy link
Contributor

scantor commented Oct 6, 2022

(My only concern re: compatibility was the reference to size_it in the max function possibly causing compatibility issues, but I'll take the risk.)

@scantor
Copy link
Contributor

scantor commented Oct 10, 2022

I applied this to both branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants