codeql icon indicating copy to clipboard operation
codeql copied to clipboard

False positive

Open silent-sour opened this issue 1 year ago • 7 comments

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);

silent-sour avatar Feb 24 '24 02:02 silent-sour

Bump for review

silent-sour avatar Oct 18 '24 17:10 silent-sour

I will take a look.

michaelnebel avatar Oct 21 '24 09:10 michaelnebel

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 avatar Oct 21 '24 09:10 michaelnebel

The issue is fixed here: https://github.com/github/codeql/pull/17815

michaelnebel avatar Oct 21 '24 10:10 michaelnebel

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.

silent-sour avatar Oct 21 '24 10:10 silent-sour

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.

michaelnebel avatar Oct 21 '24 11:10 michaelnebel

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

silent-sour avatar Oct 21 '24 13:10 silent-sour

The improvement has been merged.

michaelnebel avatar Oct 23 '24 06:10 michaelnebel