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

Feature/finding null pointer #235

Merged
merged 9 commits into from Nov 24, 2022
Merged

Conversation

julianhille
Copy link
Owner

No description provided.

@@ -1018,7 +1018,13 @@ EStatusCode CFFFileInput::CalculateDependenciesForCharIndex(unsigned short inFon
if(status != PDFHummus::eFailure)
{
mCurrentDependencies = &ioDependenciesInfo;
return interpreter.Intepret(*GetGlyphCharString(inFontIndex,inCharStringIndex),this);
CharString* charString = GetGlyphCharString(inFontIndex,inCharStringIndex);
Copy link
Owner Author

Choose a reason for hiding this comment

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

needs indentation


if (!lowerLeftX || !lowerLeftY || !upperRightX || !upperRightY)
{
// not sure if just a return is a good idea here.
Copy link
Owner Author

Choose a reason for hiding this comment

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

not sure if this is a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

well its all in teh unexpected area so any decision is good.
Inspecting all usages there's a pattern of checking for an array, then if it's 4 and then either going for a default or calling SetPDFRectangleFromArray.

so that array check+ 4 check can be moved into SetPDFRectangleFromArray for better reuse and that it'd return status. then it's `if(SetPDFRectangleFromArray(....) == eFailure) do_the_default.

@@ -396,6 +396,12 @@ EStatusCode PDFParser::ParseLastXrefPosition()
while(!foundStartXref && mStream->NotEnded())
{
PDFObjectCastPtr<PDFSymbol> startxRef(mObjectParser.ParseNewObject());
if(!startxRef)
{
status = PDFHummus::eFailure;
Copy link
Owner Author

Choose a reason for hiding this comment

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

indentation

@julianhille
Copy link
Owner Author

@mhassan1 if you got some time i could need your pair of eyes here.

@mhassan1
Copy link
Contributor

mhassan1 commented Nov 23, 2022

At first glance, all these changes look fine to me. Questions:

  1. Should we add more test PDF files to the repository to ensure we are resolving these NPEs completely?
  2. Are these exploitable in the wild? If so, should we be handling this more discreetly, with a private pull request, or similar?
  3. Will we be reporting this as a vulnerability to Snyk?

@julianhille julianhille merged commit e990adc into develop Nov 24, 2022
251 checks passed
@julianhille julianhille deleted the feature/finding-null-pointer branch November 24, 2022 08:33
@julianhille
Copy link
Owner Author

@galkahana i took a deeper look and found again some situation where it might not end well

@galkahana
Copy link
Contributor

@galkahana i took a deeper look and found again some situation where it might not end well

Thanks. Will check on my end

@galkahana
Copy link
Contributor

@julianhille somebody submitted this issue once with the results of running fuzztest on a pdf text extractor i wrote on top of hummus, might be worthwhile input for more debuggging if you wish it:
galkahana/pdf-text-extraction#2

@julianhille
Copy link
Owner Author

Awesome. Thanks.

@julianhille
Copy link
Owner Author

@mhassan1 @galkahana i found more and created security private fork i would add you if you dont mind just do not accept if you dont want to take part, ok?

@@ -2176,7 +2176,7 @@ EStatusCode DocumentContext::SetupModifiedFile(PDFParser* inModifiedFileParser)
if(idArray.GetPtr() && idArray->GetLength() == 2)
{
PDFObjectCastPtr<PDFHexString> firstID = idArray->QueryObject(0);
if(firstID.GetPtr())
if(firstID != NULL && firstID.GetPtr())
Copy link
Contributor

Choose a reason for hiding this comment

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

verifying: is the null check really needed here? won't GetPtr() be falsy in this case?

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.

None yet

3 participants