JArredondo115396 (Community Member) asked a question.

November 5, 2020 at 9:04 PM
Highly inconsistent CWE-117 flaws : Improper Output Neutralization for Logs

I am new to Veracode mitigation and doing initial scans on several applications. One application which has hundreds of debugging lines had a number flagged as CWE-117 yet I am not able to determine a consistent basis for this. In the very same class I see this line of code generate the flaw (Note, _workModel.ProcessID is a GUID type):

 

_logger.Debug($"{_workModel.ProcessId}|Starting Certify Process. ");

 

yet in a similar method this line of code did not generate a flaw:

 

_logger.Debug($"{_workModel.ProcessId}|Attempting to clean Certify logs. ");

 

Many of these flaws do not write any sort of user information to logs, including types like GUID's and integers which cannot contain CRLF's or other unsafe characters and should be safer to write to logs but yet CWE-177 flaws occur. Please help me understand this inconsistency. Thanks.

  • 4 answers
  • 1.52K views
VCode likes this.

  • Boy, Security Consultant (Veracode)

    Hi @JArredondo115396 (Community Member)​ ,

     

    Veracode Static Analysis detects CWE 117 by inspecting the binary (or source code for source languages) for

    1. A logging statement
    2. ... that we can detect is being used
    3. ... that we can detect is logging data originating from outside of the application (such as data from an HTTP request, but also from a file, database, etc.)
    4. ... where the logging data is not passed through a Supported Cleansing Function ( https://help.veracode.com/go/review_cleansers )

     

    Only if all 3 conditions are true will we report a flaw. In general though we recommend having a *consistent* logging strategy. For example using a Supported Cleansing Function ( https://help.veracode.com/go/review_cleansers ) on all dynamic data. If you implement this, Veracode Static Analysis will be able to automatically close the flaws.

     

    There are also other solutions that can not be automatically detected by Veracode Static Analysis, for example not logging to a log file but logging to a logging service that does not delimit with newlines (\n and \r).

    If you're using this you can document this in a mitigation proposal ( https://help.veracode.com/go/improve_mitigation ) and contact your security team for approval.

     

    Thank you,

    Boy Baukema

    Expand Post
    Selected as Best

  • Boy, Security Consultant (Veracode)

    Hi @JArredondo115396 (Community Member)​ ,

     

    Veracode Static Analysis detects CWE 117 by inspecting the binary (or source code for source languages) for

    1. A logging statement
    2. ... that we can detect is being used
    3. ... that we can detect is logging data originating from outside of the application (such as data from an HTTP request, but also from a file, database, etc.)
    4. ... where the logging data is not passed through a Supported Cleansing Function ( https://help.veracode.com/go/review_cleansers )

     

    Only if all 3 conditions are true will we report a flaw. In general though we recommend having a *consistent* logging strategy. For example using a Supported Cleansing Function ( https://help.veracode.com/go/review_cleansers ) on all dynamic data. If you implement this, Veracode Static Analysis will be able to automatically close the flaws.

     

    There are also other solutions that can not be automatically detected by Veracode Static Analysis, for example not logging to a log file but logging to a logging service that does not delimit with newlines (\n and \r).

    If you're using this you can document this in a mitigation proposal ( https://help.veracode.com/go/improve_mitigation ) and contact your security team for approval.

     

    Thank you,

    Boy Baukema

    Expand Post
    Selected as Best

  • chubbsondubs (Community Member)

    I recently just wrestled with 117 flaw. When I started I had raw logger references being used everywhere and concatenation happening everywhere. So I decided to centralize all of the logger usages into a utility class LogUtil. Then I changed all of my references to use this utility. Since concatenating strings was an issue I created a method that let me change from things like this:

     

    logger.debug( workerId + " | Starting Job")

     

    To:

     

    logger.debug("{0} | Starting Job", workerId)

     

    After changing everything over to this new placeholder pattern when I scanned I still got 117, but only inside this one class. Then inside my util I escaped all parameters being passed in. But, Veracode still said I had issues. I was using a Supported Cleansing Function, but it complained still. Eventually I decided that escaping just the parameters wasn't good enough because developers could still concatenate onto the message being passed and I wasn't escaping that. So I escaped that too. And it still complained. Finally, I decided it there was too much escaping going on, and just for performance reasons I decided to escape right before I passed the message to the underlying logger So for example from this:

     

    logger.debug( escape(message), escapeParams( params ) )

     

    to:

     

    logger.debug( escape( message.format( params ) ) )

     

    And it's that last change that fixed the flaw completely. So the lessons I took away from this were:

     

    1. Centralizing my usage of the logger helped to solve the problem and prevent other developers from making mistakes
    2. I didn't have to make my fancy format functions to fix it, but it does help with performance.
    3. Escaping at the very bottom was the only solution. Escape at the last possible place before the message is sent to the logger.
    4. Why the hell haven't log libraries already done this for us?! 😁
    Expand Post

      • chubbsondubs (Community Member)

        @EGertis462759 (Community Member)​ In this particular case commons-lang StringEscapeUtils.escapeJava did the trick. It'll escape the new lines and replace those with \n. This makes messages that use new lines a little harder to read, but it doesn't affect stacktraces as those are logged through a different mechanism [ie logger.info( escape( message ), exception)]

        Expand Post

Topics (4)

No articles found
Loading

Ask the Community

Get answers, share a use case, discuss your favorite features, or get input from the community.