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

False positive #15714

Closed
silent-sour opened this issue Feb 24, 2024 · 8 comments
Closed

False positive #15714

silent-sour opened this issue Feb 24, 2024 · 8 comments
Assignees

Comments

@silent-sour
Copy link

Description of the false positive

C# CWE-117 is incorrectly applied to user input sanitized with {string}.ReplaceLineEndings() instead of {string}.Replace(Environment.NewLine, string.Empty)

**Code sample

var username = authInfo.Username.ReplaceLineEndings();
_logger.LogError("Invalid login attempt: {username}", username);
@sidshank sidshank added the C# label Feb 26, 2024
@github github deleted a comment Feb 26, 2024
@silent-sour
Copy link
Author

Bump for review

@michaelnebel michaelnebel self-assigned this Oct 21, 2024
@michaelnebel
Copy link
Contributor

I will take a look.

@michaelnebel
Copy link
Contributor

michaelnebel commented Oct 21, 2024

I think the example might need to be changed a bit as the parameterless overload of ReplaceLineEndings replaces newlines with newlines (using the current environment). That is, we should probably only consider a string to be sanitized using the ReplaceLineEndings method in case the new lines are specifically replaced with something (eg. string.Empty)

var username = authInfo.Username.ReplaceLineEndings(string.Empty);
_logger.LogError("Invalid login attempt: {username}", username);

@michaelnebel
Copy link
Contributor

The issue is fixed here: #17815

@silent-sour
Copy link
Author

Thank you for working towards a resolution here.

You are incorrect in your assertion there. The documentation in the source shows the following:

    /// This method searches for all newline sequences within the string and canonicalizes them to the
    /// newline sequence provided by <paramref name="replacementText"/>. If <paramref name="replacementText"/>
    /// is <see cref="Empty"/>, all newline sequences within the string will be removed.
    ///

As such this method takes line endings (CR, LF, CR/LF) canonicalizes them to the local platform line ending, and replaces them with either the replacementText or the empty default value.

@michaelnebel
Copy link
Contributor

michaelnebel commented Oct 21, 2024

Thank you for the quick response.

The snippet of documentation you are referring to is from ReplaceLineEndings(string replacementText) and it states that if you provide string.Empty as an argument to ReplaceLineEndings(string replacementText) then all newline sequences will be removed.

The documentation for ReplaceLineEndings() can be seen here and it says

        /// <summary>
        /// Replaces all newline sequences in the current string with <see cref="Environment.NewLine"/>.
        /// </summary>
        /// <returns>
        /// A string whose contents match the current string, but with all newline sequences replaced
        /// with <see cref="Environment.NewLine"/>.
        /// </returns>

which does't remove newlines.

@silent-sour
Copy link
Author

Appreciate it, as I guess I didn't see that overrides' documentation.

@michaelnebel
Copy link
Contributor

The improvement has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants