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

Crash in brave_news::parse_feed_string #26604

Closed
rillian opened this issue Nov 7, 2022 · 4 comments · Fixed by brave/brave-core#15895
Closed

Crash in brave_news::parse_feed_string #26604

rillian opened this issue Nov 7, 2022 · 4 comments · Fixed by brave/brave-core#15895

Comments

@rillian
Copy link

rillian commented Nov 7, 2022

Description

User reported a crash in brave_news::parse_feed_string. Investigation traced it to the presence of the character sequence <! at the end of of description body in this rss feed.

The voca_rs library used by our feed parser uses unchecked lookahead when stripping tags and fails a bounds-check when passed this text after entity substitution.

Steps to Reproduce

  1. Enable Brave News
  2. Add a custom feed with <! at the end of the title or description field
  3. Start browser

Actual result:

Crash is observed

Expected result:

Data should be handled gracefully.

Reproduces how often:

100% reproducible with the reduced unit testcase I wrote. The live feed from the original report will likely change soon and may stop triggering the issue.

Desktop Brave version:

Initially reported in Nightly 1.47.42

Version/Channel Information:

  • Can you reproduce this issue with the current release? Yes
  • Can you reproduce this issue with the beta channel?
  • Can you reproduce this issue with the nightly channel? Yes

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields?
  • Does the issue resolve itself when disabling Brave Rewards?
  • Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

Filed a-merezhanyi/voca_rs#21 for the upstream issue.

@rillian rillian added OS/Android Fixes related to Android browser functionality OS/Desktop QA/Yes labels Nov 7, 2022
@rillian
Copy link
Author

rillian commented Nov 8, 2022

I filed an upstream PR with a quick fix. If upstream can't respond quickly, we should switch to a fork.

Other mitigations:

  • We could try to strip the offending text in our cxx bridge code
  • voca_rs and feed_rs clearly need fuzzing
  • Run the feed parser in a utility process for better crash recovery

rillian added a commit to brave/brave-core that referenced this issue Nov 10, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 11, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 14, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 14, 2022
Make sure we always build with at least the version with the
fix for the partial-tag array index panic.

Fixup for brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 18, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 21, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
rillian added a commit to brave/brave-core that referenced this issue Nov 21, 2022
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
@brave-builds brave-builds added this to the 1.48.x - Nightly milestone Dec 7, 2022
@rillian rillian self-assigned this Dec 8, 2022
@kjozwiak
Copy link
Member

kjozwiak commented Dec 9, 2022

The above requires 1.47.121 or higher for 1.47.x verification 👍

@stephendonner
Copy link

Verified PASSED using

Brave 1.47.123 Chromium: 108.0.5359.99 (Official Build) beta (x86_64)
Revision 410951fc34bb4b2cbf182231f9f779efaafaf682-refs/branch-heads/5359_71@{#9}
OS macOS Version 13.1 (Build 22C65)

Steps:

  1. installed 1.47.123
  2. launched Brave
  3. opened a new-tab window
  4. clicked Customize
  5. clicked Brave News
  6. clicked Turn on Brave News
  7. pasted https://gist.github.com/rillian/4848ee54e73890a3cbd37545c7517fd1/raw/3c0994323049b7578e5362c7a60608cb7b19202a/gistfile1.txt into the search textfield and pressed return
  8. clicked Get feeds from ...
  9. under Sources, clicked on Follow on The Hacker News
  10. exited the dialog
  11. checked my Brave News feed

Confirmed no crash, I was able to add The Hacker News feed to Brave News, and viewed an article

example example example example example
Screenshot 2022-12-11 at 10 12 05 AM Screenshot 2022-12-11 at 10 12 09 AM Screenshot 2022-12-11 at 10 12 12 AM Screenshot 2022-12-11 at 10 12 20 AM Screenshot 2022-12-11 at 10 15 31 AM

@Uni-verse
Copy link
Contributor

Uni-verse commented Jan 5, 2023

Verified on Samsung Galaxy S21 & Samsung Galaxy Tab S7 using the following version(s):

Brave	1.47.164 Chromium: 109.0.5414.74 (Official Build) (64-bit) 
Revision	e7c5703604daa9cc128ccf5a5d3e993513758913-refs/branch-heads/5414@{#1172}
OS	Android 12; Build/SP1A.210812.016

Test Plan:

  1. Launch Brave
  2. Scroll on new tab page and enable Brave News
  3. Click 'Customize' in the lower right
  4. In the search box add feed url https://gist.github.com/rillian/4848ee54e73890a3cbd37545c7517fd1/raw/3c0994323049b7578e5362c7a60608cb7b19202a/gistfile1.txt
  5. Click 'Add Source' or 'Find feeds at...' followed by 'Follow' for the listed rss feed, depending on the Brave News version.
  6. Check updated feed
  7. Remove custom rss feed
  • Verified custom source mentioned in test plan can be add to feed
  • Verified refreshing feed/relaunch browser app does not cause any crash
  • Verified custom source can be removed in the settings
Example Example Example Example
screenshot-1672959043088 screenshot-1672959145135 screenshot-1672959153818 screenshot-1672959628415

sangwoo108 pushed a commit to brave/brave-core that referenced this issue Apr 4, 2023
Bump this dependency of the feed parser used by Brave News to
avoid an array index out of bounds panic when an rss field had
an angle bracket near the end. Some transitive dependencies are
also updated.

Resolves brave/brave-browser#26604
This was referenced Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants