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

Use encoded URLs for searchSuggestionHtml #721

Merged
merged 2 commits into from Mar 9, 2022
Merged

Use encoded URLs for searchSuggestionHtml #721

merged 2 commits into from Mar 9, 2022

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Mar 1, 2022

Previously, the seachURL was not encoded.
This resulted in an XSS vulnerability, a concept of proof is:

start kiwix-serve
visit - http://192.168.18.1:8081/"><svg onload=alert(1)>
This would display an alert message.

This encodes the searchURL before passing it to searchSuggestionHtml

@juuz0 juuz0 requested a review from kelson42 March 1, 2022 11:46
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Unit tests are broken and it seems you PR encode & URL sepearator and it should not.

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #721 (04d6824) into master (e48b550) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #721   +/-   ##
=======================================
  Coverage   57.81%   57.81%           
=======================================
  Files          56       56           
  Lines        3651     3651           
  Branches     2047     2047           
=======================================
  Hits         2111     2111           
  Misses       1539     1539           
  Partials        1        1           
Impacted Files Coverage Δ
src/server/internalServer.cpp 81.25% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e48b550...04d6824. Read the comment docs.

@juuz0 juuz0 requested a review from kelson42 March 1, 2022 16:35
@kelson42 kelson42 added this to the 10.1.0 milestone Mar 1, 2022
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

I wonder if this is the right place to secure the url. At least we already have a function to url encode a string (urlEncode), we should use it.

But nice catch anyway.

src/server/internalServer.cpp Outdated Show resolved Hide resolved
@juuz0 juuz0 requested a review from mgautierfr March 2, 2022 17:14
@juuz0 juuz0 changed the title Do escape searchURL in full text search suggestion use encoded URLs for searchSuggestionHtml Mar 2, 2022
src/server/internalServer.cpp Outdated Show resolved Hide resolved
@kelson42 kelson42 changed the title use encoded URLs for searchSuggestionHtml Use encoded URLs for searchSuggestionHtml Mar 5, 2022
@juuz0 juuz0 force-pushed the xssVul branch 2 times, most recently from 48d6916 to 6dbd859 Compare March 6, 2022 13:07
@juuz0 juuz0 requested a review from mgautierfr March 6, 2022 13:07
Previously, the seachURL was not encoded.
This resulted in an XSS vulnerability, a concept of proof is:

start kiwix-serve
visit - http://192.168.18.1:8081/"><svg onload="alert(1)">
This would display an alert message.

This encodes the searchURL before passing it to searchSuggestionHtml
@kelson42 kelson42 merged commit f893777 into master Mar 9, 2022
20 checks passed
@kelson42 kelson42 deleted the xssVul branch March 9, 2022 13:33
@legoktm
Copy link
Member

legoktm commented Mar 18, 2022

@juuz0 @kelson42 do we know what version the XSS vulnerability was introduced in?

@kelson42
Copy link
Collaborator

@legoktm I don't know, but I would guess for an old weakness

@juuz0
Copy link
Collaborator Author

juuz0 commented Mar 18, 2022

#465
Probably this
the method added into this explicitly defines no escaping {{{searchURL}}}

@legoktm
Copy link
Member

legoktm commented Mar 18, 2022

Thanks, so a597870 was only included in 10.0.0 (no released Debian versions are affected, just unstable).

Could we do a 10.0.2 release with just this cherry-picked? I note that even library.kiwix.org is vulnerable to this. Or if 10.1.0 is coming pretty soon then waiting wouldn't be too bad.

And we should also get a CVE ID assigned for this vulnerability, @kelson42 if you haven't gone through this process before I'm happy to help out.

@kelson42
Copy link
Collaborator

@legoktm Thx for your last comment. For the moment I don’t have a strong opinion on this but understand the rationals. It seems as well your have a clearer opinion on the next release than I do. But, considering this PR to be closed, could you please open a new ticket with this arguments and why you woukd like to request early release of 10.1.0?

@kelson42 kelson42 mentioned this pull request Mar 23, 2022
10 tasks
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

4 participants