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

OF-1885: SSRF guard favicon #1497

Merged
merged 2 commits into from Oct 27, 2019

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Oct 3, 2019

No description provided.

The Openfire servlet that is supposed to be used to retrieve favicons from remote servers could be used to obtain anything. To prevent unauthorized use, this commit adds a check that verifies if the returned data is an image. If that's not the case, the returned data is ignored.
I've noticed that many favicons are not shown. Increasing retrieval timeouts in the hope that this yields better results.
@GregDThomas
Copy link
Contributor

This is good as far as it goes; however I wonder if it could be locked down further? From memory, the favicon is used when displaying S2S sessions. How about restricting the FavIcon servlet to only fetch icons from servers from which a S2S session exists? And the favicon.ico at that?

@guusdk
Copy link
Member Author

guusdk commented Oct 4, 2019

I've considered that. I'm wondering if the added complexity outweighs the added security benefits. What do you think?

@GregDThomas
Copy link
Contributor

Having pondered, I worry that even if we're not displaying it, the admin console will happily fetch any file off any HTTP server it has access to. If it were me, I'd probably lean towards changing the way this works such that the servlet
(a) Only fetches favicon's from S2S connected servers, and
(b) Only fetches the favicon

@guusdk
Copy link
Member Author

guusdk commented Oct 4, 2019 via email

@akrherz
Copy link
Member

akrherz commented Oct 27, 2019

I might raise a ticket as a future improvement to this.

Filed OF-1902

@akrherz akrherz merged commit e6a9db9 into igniterealtime:master Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants