You are viewing a plain text version of this content. The canonical link for it is here.
Posted to c-dev@xerces.apache.org by GitBox <gi...@apache.org> on 2022/10/05 19:56:42 UTC
[GitHub] [xerces-c] rouault opened a new pull request, #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
rouault opened a new pull request, #51:
URL: https://github.com/apache/xerces-c/pull/51
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>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] scantor commented on pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
scantor commented on PR #51:
URL: https://github.com/apache/xerces-c/pull/51#issuecomment-1269983028
(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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] rouault commented on a diff in pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
rouault commented on code in PR #51:
URL: https://github.com/apache/xerces-c/pull/51#discussion_r986039665
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
Review Comment:
fLeafCount is actually declared as unsigned int at line 236 of DFAContentModel.hpp (which is already much bigger than what is reasonable in practice :-))
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
+ if (fLeafCount > std::numeric_limits<size_t>::max() / sizeof(CMLeaf*))
Review Comment:
> Would it be possible to add brackets around the division for clarity?
this is the best way I can think of that doesn't rely on doing overflow arithmetic... As it is unsigned type, we could do overflow arithmetic as it is well defined in C/C++, but I tend to fuzz software with -fsanitize=unsigned-integer-overflow because in practice > 90% unsigned overflows that occur are actually bugs
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
Review Comment:
fixed
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -1364,14 +1372,27 @@ unsigned int DFAContentModel::countLeafNodes(ContentSpecNode* const curNode)
if(nLoopCount!=0)
{
count += countLeafNodes(cursor);
- for(unsigned int i=0;i<nLoopCount;i++)
- count += countLeafNodes(rightNode);
+ const unsigned int countRight = countLeafNodes(rightNode);
+ // Avoid integer overflow in below multiplication
+ if (countRight > std::numeric_limits<unsigned int>::max() / nLoopCount)
Review Comment:
cf above comments regarding data type
brackets added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] rouault commented on pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
rouault commented on PR #51:
URL: https://github.com/apache/xerces-c/pull/51#issuecomment-1265749924
CC @rleigh-codelibre This should be relatively safe to apply
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] rleigh-codelibre commented on a diff in pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
rleigh-codelibre commented on code in PR #51:
URL: https://github.com/apache/xerces-c/pull/51#discussion_r986030234
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
Review Comment:
Should this be `XMLSize_t` (a.k.a. `size_t`) rather than `unsigned int` to match the type of `fLeafCount`?
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
Review Comment:
typo: allocation
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -1364,14 +1372,27 @@ unsigned int DFAContentModel::countLeafNodes(ContentSpecNode* const curNode)
if(nLoopCount!=0)
{
count += countLeafNodes(cursor);
- for(unsigned int i=0;i<nLoopCount;i++)
- count += countLeafNodes(rightNode);
+ const unsigned int countRight = countLeafNodes(rightNode);
+ // Avoid integer overflow in below multiplication
+ if (countRight > std::numeric_limits<unsigned int>::max() / nLoopCount)
Review Comment:
In all of the following changes, just want to check as above that the limit check should use `XMLSize_t` rather than `unsigned int`?
And would it also be possible to put brackets around all expressions being compared for clarity?
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
+ if (fLeafCount > std::numeric_limits<size_t>::max() / sizeof(CMLeaf*))
Review Comment:
Should this also be `XMLSize_t`?
Would it be possible to add brackets around the division for clarity?
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
Review Comment:
Oops, I was looking at the wrong definition in src/xercesc/validators/common/ContentLeafNameTypeVector.hpp when I was scanning over the `git grep` output. In that case, that all looks fine!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] scantor commented on pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
scantor commented on PR #51:
URL: https://github.com/apache/xerces-c/pull/51#issuecomment-1273514080
I applied this to both branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] scantor commented on pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
scantor commented on PR #51:
URL: https://github.com/apache/xerces-c/pull/51#issuecomment-1269982071
@rleigh-codelibre If you can apply this to master that will make it easier for me to cherry-pick back to the branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] rleigh-codelibre commented on a diff in pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
rleigh-codelibre commented on code in PR #51:
URL: https://github.com/apache/xerces-c/pull/51#discussion_r986046406
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
+ if (fLeafCount > std::numeric_limits<size_t>::max() / sizeof(CMLeaf*))
Review Comment:
I just meant `fLeafCount > (std::numeric_limits<size_t>::max() / sizeof(CMLeaf*))` so that the operator precedence is explicit (and the same for below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org
[GitHub] [xerces-c] rouault commented on a diff in pull request #51: [XERCESC-2241] Fix integer overflows in DFAContentModel class
Posted by GitBox <gi...@apache.org>.
rouault commented on code in PR #51:
URL: https://github.com/apache/xerces-c/pull/51#discussion_r986050269
##########
src/xercesc/validators/common/DFAContentModel.cpp:
##########
@@ -661,8 +662,15 @@ void DFAContentModel::buildDFA(ContentSpecNode* const curNode)
// in the fLeafCount member.
//
fLeafCount=countLeafNodes(curNode);
+ // Avoid integer overflow in below fLeafCount++ increment
+ if (fLeafCount > std::numeric_limits<unsigned int>::max() - 1)
+ throw OutOfMemoryException();
fEOCPos = fLeafCount++;
+ // Avoid integer overflow in below memory allocatoin
+ if (fLeafCount > std::numeric_limits<size_t>::max() / sizeof(CMLeaf*))
Review Comment:
ah sorry for some reason I read your sentence as "would it be possible to *avoid* the division"... Brackets added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org