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

Added encoding explicitly when XmlReader is created for non-utf-8 cases #919

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

Aleksanderis
Copy link
Contributor

A fix for #918, when encoding should be specified explicitly if web service should work with encodings other than utf-8 (or any other utf encodings). It's an improvement for a previous fix for ISO-8859-1 support (#429).

It creates a new StreamReader on top of existing stream, which might look like a hack. However in fact initial stream is coming from some .NET internals like HttpContext.Request.Body and as much as I understand - by default it's always UTF-8 nowadays. I didn't find any better way to switch the encoding for the underlying stream.
In addition I added CloseInput = true property on XmlReaderSettings, which should handle and close additionally created StreamReader.

else
{
var streamReaderWithEncoding = new StreamReader(stream, _writeEncoding);
reader = XmlReader.Create(streamReaderWithEncoding, new XmlReaderSettings() { IgnoreWhitespace = true, DtdProcessing = DtdProcessing.Prohibit, CloseInput = true });

Check failure

Code scanning / CodeQL

Untrusted XML is read insecurely

This insecure XML processing depends on a [user-provided value](1) (DTD processing is enabled by default in versions before 4.0, default settings resolver is insecure in versions before 4.5). This insecure XML processing depends on a [user-provided value](2) (DTD processing is enabled by default in versions before 4.0, default settings resolver is insecure in versions before 4.5).
Copy link
Contributor Author

@Aleksanderis Aleksanderis Nov 5, 2022

Choose a reason for hiding this comment

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

Tried to inline settings like it was before - but it still doesn't recognize that DtdProcessing = DtdProcessing.Prohibit is set.

According to https://codeql.github.com/codeql-query-help/csharp/cs-xml-insecure-dtd-handling/:

The solution is to set the DtdProcessing property to DtdProcessing.Prohibit.

..so it still looks like a false positive to me.

@kotovaleksandr kotovaleksandr merged commit af8d9fb into DigDes:develop Nov 7, 2022
@Aleksanderis Aleksanderis deleted the fix-non-utf-encodings branch November 7, 2022 06:22
@Aleksanderis
Copy link
Contributor Author

@kotovaleksandr thanks for approving. Any ETA for NuGet package release with the fix?
It's quite urgent for us - we need a working solution to test other business features.
So would be good to know if we should wait a bit, or revert to a stable branch without SoapCore and return to it later.

@kotovaleksandr
Copy link
Member

@Aleksanderis done
https://www.nuget.org/packages/SoapCore/1.1.0.34

@Aleksanderis
Copy link
Contributor Author

Nice, thanks a lot. 👍

@Aleksanderis
Copy link
Contributor Author

Seems like this fix wasn't fully enough, more flexibility is needed to control encodings in non-utf world..
A new follow-up pull-request - #921

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.

2 participants