RLindsey475282 (Community Member) asked a question.
Why would this code sample not mitigate CWE 117?
public async Task<IActionResult> GetAttributesAsync( string identifier , [FromQuery] string user )
{
try
{
var result = await _attributeManager.GetAppAttributesAsync( identifier , user );
return Ok( result );
}
catch ( ConfigManagerException ex )
{
return BadRequest( new ErrorResponse( ex.Message , ex.ErrorCode ) );
}
catch ( Exception ex )
{
_logger.LogCritical( ex , ex.Message , HttpUtility.UrlEncode( identifier ) , HttpUtility.UrlEncode( user ) );
return StatusCode( StatusCodes.Status500InternalServerError );
}
}
.png){kind=logo}
Hi @RLindsey475282 (Community Member) ,
Veracode Static Analysis reports flaws of CWE 117 when we can see dynamic data going into a logging statement. Depending on the logger this may also include the exception message and exception, however I would need more details to verify this is the case here.
I would recommend you schedule a consultation call to discuss.
You can check out this knowledge article (https://community.veracode.com/s/article/How-to-schedule-a-consultation-call) on how to schedule a consultation call with us.
Thank you,
Boy Baukema
I don't believe a consultation is in order. Your documentation on how to address CWE 117 explicitly states that one of the approved cleansing functions is HttpUtility.UrlEncode(), which is clearly being leverage in the example. I'm still unclear as to why this does not satisfactorily address the flaw. Please advise.
@RLindsey475282 (Community Member)
As we see there are 4 arguments to this function, the 3rd & 4th are cleansed using HttpUtility.UrlEncode(), but the 1st and 2nd argument which are 'ex', 'ex.getMessage' is not cleaned. That is what Boy has indicated earlier.
Regards,
Kashif.