Original flawed code:

  public Task<HttpResponseMessage> DoSomething(string param1, string param2, string isParam3, string param4, string param5, string param6, HttpRequestMessage request)

  {

   return SendRequest(HttpMethod.Get, $"/api/endpoint/action{request.RequestUri.Query}", request);

  }

... which in turn calls:

  private Task<HttpResponseMessage> SendRequest(HttpMethod method, string queryString, HttpRequestMessage request)

  {

   var forwardUri = $"{_baseAddress}{queryString}";

   request.Headers.Remove("Host");

   var cookies = request.Headers.GetCookies();

   var specificCookies = cookies?

    .SelectMany(header => header.Cookies

     .Where(c => c.Name == "matching-header")

     .Select(x => $"{x.Name}={x.Value};")

    ).ToList() ?? new List<string>();

   if (specificCookies.Any())

   {

    request.Headers.Remove("Cookie");

    request.Headers.Add("Cookie", specificCookies);

   }

   request.RequestUri = new Uri(forwardUri);

   if (request.Method == HttpMethod.Get)

   {

    request.Content = null;

   }

   return _client.SendAsync(request);

  }

... where _client is an HttpClient:

  public SomeService()

  {

   _client = new HttpClient();

   _baseAddress = ConfigurationManager.AppSettings["SomeEndpoint"];

   if (!_baseAddress.EndsWith("/"))

    _baseAddress += "/";

  }

Now, I see why this generated the CWE 918... but I don't understand why my code change did not remediate it.

In another Community post by spadhi561149 on December 17, 2019, @Boy, Security Consultant (Veracode)​ responded:

The only thing that Veracode Static Analysis will automatically detect as a remediation for this flaw category is to change the input to be hardcoded or to cast it to an integer. If this is not possible we recommend that you apply dynamic validation (for example with a regex) but this will not be automatically detected by Veracode Static Analysis and must then be documented in a Mitigation by Design mitigation proposal and reviewed by your security team.

So, I did exactly that -- I ensured everything made at minimum a round-trip to a scalar variable, which should prevent forgery injections:

public Task<HttpResponseMessage> DoSomething(string param1, string param2, string isParam3, string param4, string param5, string param6, HttpRequestMessage request)

{

 // JLW CWE 918 fix for Veracode 2625 -- validate URL inputs

 UriBuilder builder = new UriBuilder($"{_baseAddress}" + "api/controller/action");

 var query = HttpUtility.ParseQueryString(builder.Query);

 if (!long.TryParse(param1, NumberStyles.None, CultureInfo.CurrentCulture, out long param1AsLong))

 {

  _logger.Error($"Error message specific to invalid long integer param1.");

  throw new Exception($"Error message specific to invalid long integer param1.");

 }

 if (!decimal.TryParse(param2, NumberStyles.None, CultureInfo.CurrentCulture, out decimal param2AsDecimal))

 {

  _logger.Error($"Error message specific to invalid decimal param2.");

  throw new Exception($"Error message specific to invalid decimal param2.");

 }

 if (!int.TryParse(param4, NumberStyles.None, CultureInfo.CurrentCulture, out int param3AsInt))

 {

  _logger.Error($"Error message specific to invalid integer param4.");

  throw new Exception($"Error message specific to invalid integer param4.");

 }

 if (!int.TryParse(param4, NumberStyles.None, CultureInfo.CurrentCulture, out int param5AsInt))

 {

  _logger.Error($"Error message specific to invalid integer param5.");

  throw new Exception($"Error message specific to invalid integer param5.");

 }

 query["param1"] = param1AsLong.ToString();

 query["staticParam"] = "1";

 query["param2"] = param2AsDecimal.ToString(CultureInfo.CurrentCulture);

 query["isParam3"] = isParam3.ToLower() == "true" ? "true" : "false";

 query["param4"] = param4AsInt.ToString();

 query["param5"] = param5AsInt.ToString();

 query["param6"] = Uri.EscapeDataString(param6);

 return SendRequest(HttpMethod.Get, "api/controller/endpoint", query, request);

}

... with a new overload to SendRequest, that constructs what SHOULD be a safe request, and passes it along to the original SendRequest:

  private Task<HttpResponseMessage> SendRequest(HttpMethod method, string endpoint, NameValueCollection query, HttpRequestMessage request)

  {

   UriBuilder builder = new UriBuilder($"{_baseAddress}{endpoint}") {Query = query.ToString()};

   return SendRequest(method, $"{endpoint}{builder.Query}", request);

  }

Is the static scan not using heuristics to see that the Query segments have all been validated to be scalars (except the final one, which is an unvalidatable string that is essentially a security token)?

I still get four paths of violation through the above code, and I don't understand why. I PERHAPS expected a single path, due to param6 not being scalar and thus uncastable, but I thought that EscapeDataString stood an even shot at success. I didn't expect NO change to the scan, however. My NameValueCollection was completely validated without RegEx by casting to scalars, as instructed.

Any help would be appreciated.

Best Regards,

Jeff W

Reading, PA