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

[Playlist] dont use istream directly to a tinyxml structure #20306

Merged
merged 1 commit into from Oct 20, 2021

Conversation

fuzzard
Copy link
Contributor

@fuzzard fuzzard commented Oct 12, 2021

Description

Turn istream into a std::string to handle large buffers #20305

Motivation and context

Fixes #20305

the >> operator of istream does no bounds checking and hangs/eventually crashes with large inputs as seen with the POC provided. Instead look to bring the istream into a std::string and then parse the string with tinyxml into the CXBMCTinyXML structure.

How has this been tested?

OSX with the provided POC in #20305, and another "good" sample asx file.
The POC errors out with the xmldoc.Error check. Good sample is processed correctly.

What is the effect on users?

No crash with a malicious asx file

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Turn istream into a std::string to handle large buffers (xbmc#20305)
@fuzzard fuzzard added Type: Fix non-breaking change which fixes an issue Backport: Needed v20 Nexus labels Oct 12, 2021
@fuzzard fuzzard added this to the Nexus 20.0 Alpha 1 milestone Oct 12, 2021
@basilgello
Copy link
Collaborator

I think this might be needed to be ported to 18.x and 17.x as well... At least, Debian still ships 17.6 in buster.

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 18, 2021

Anyone any insights on whether this is an appropriate fix. Any other options/methods anyone can come up with?

@basilgello
Copy link
Collaborator

I think it's proper one, if the root cause is no bounds check on istream. Did the reporter share details on second issue?

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 18, 2021

not that i could ascertain. From what i gathered, its just the same root cause being triggered via different events (run direct asx file, or browse folder with asx file).

@fuzzard fuzzard merged commit 48730b6 into xbmc:master Oct 20, 2021
2 checks passed
@fuzzard fuzzard deleted the fix_20305 branch October 20, 2021 02:54
smp79 pushed a commit to smp79/xbmc that referenced this pull request Oct 20, 2021
[Playlist] dont use istream directly to a tinyxml structure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Type: Fix non-breaking change which fixes an issue v20 Nexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kodi v19.0 Buffer Overflow
2 participants