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

Configure parser factories to prevent XXE attacks #10

Merged
merged 1 commit into from
Oct 5, 2018
Merged

Configure parser factories to prevent XXE attacks #10

merged 1 commit into from
Oct 5, 2018

Conversation

mpiggott
Copy link
Contributor

@mpiggott mpiggott commented Oct 3, 2018

Fix for #9 based on the OWASP Cheat Sheet

I attempted a test for DOMParser but even without the changes the parser would fail with a ParserException, its unclear to me whether it was an error in my hamfisted XHTML or the parser as it exists wouldn't have been vulnerable to an XXE.

@christianbauer christianbauer merged commit 74e6678 into 4thline:master Oct 5, 2018
@Nadahar
Copy link

Nadahar commented Oct 5, 2018

I'm not knowledgeable enough about how XML is used when it comes to DTDs, schemas etc., but I'm afraid that this is too simple. At least when it comes to UPnP my tests indicate that DTDs can't simply by disabled, that it breaks UPnP.

How well have the consequences of this been tested?

@mpiggott
Copy link
Contributor Author

mpiggott commented Oct 5, 2018

@Nadahar I haven't tested downstream, I stumbled upon the ticket. If DTDs are required for UPnP, then the example code on the cheat sheet gives some external resources in comments about other problems to look for.

@christianbauer
Copy link
Member

The tests should cover this well. However except running tests I've only started the Workbench and clicked on a few devices and executed a few actions to verify this fix, nothing more.

@Sami32
Copy link

Sami32 commented Nov 3, 2018

It has not fixed the XXE security issue:
UniversalMediaServer/UniversalMediaServer#1522 (comment)

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.

4 participants