csingh926541 (Community Member) asked a question.
public FileResult RenderImage(string imagePath)
{
var clientHandler = new HttpClientHandler { Credentials = this.NetworkCredentials };
using (var client = new HttpClient(clientHandler))
{
new FileExtensionContentTypeProvider().TryGetContentType(imagePath, out string contentType);
var imageData = client.GetByteArrayAsync(url).Result;
return new FileContentResult(imageData, contentType ?? "image/jpeg");
}
}
Issue was at highlighted one. The fix we tried is here below
if (!string.IsNullOrWhiteSpace(ReportServerUrl) && imagePath.Contains(ReportServerUrl, StringComparison.OrdinalIgnoreCase))
{
string url = $"{imagePath}";
var imageData = client.GetByteArrayAsync(url).Result;
return new FileContentResult(imageData, contentType ?? "image/jpeg");
}
But this fix didn't work , we followed this article https://community.veracode.com/s/question/0D52T00004i1UiSSAU/how-to-fix-cwe-918-veracode-flaw-on-webrequest-getresponce-method
Please help me out to fix this flaw.
.png){kind=logo}
Hi @csingh926541 (Community Member),
Veracode Static Analysis will report CWE 918 (Server-Side Request Forgery (SSRF)) if it can detect that data from outside of the application (such as the HTTP request, the database, or even the filesystem) is able to change the nature of a network request. The concern is that an attacker might be able to change the resource being requested and e.g. obtain resources he shouldn't have access to (for example from your local network).
In your particular scenario, you check that the `imagePath` to access contains `ReportServerUrl`. I generally don't feel comfortable with `contains()` checks applied on URLs. As an example, say `ReportServerUrl` would be `my-server.com`. URLs of the form `some-interesting-resource-on-your-network#my-server.com`, `attacker.my-server.com` (this one would be more interesting for Open Redirects though), etc. would pass this check and might still allow an attacker to forge a server-side request. If there is only one particular host you want your application to access here, please validate that this host is `ReportServerUrl`.
Moreover, if e.g. the image name would be user-controlled, I would recommend applying some dynamic validation on this, e.g. make sure it is alphanumeric (e.g.: `^[a-zA-Z0-9]{1,50}$`, without the ticks) and validate that the file extension provided belongs to an image.
Please note that Veracode Static Analysis will not automatically close flaws based on dynamic validation. This means that you would have to propose a mitigation and discuss it with your Security Team. The following article describes how to propose a mitigation: https://help.veracode.com/r/improve_mitigation .
Thank you,
Florian Walter
public async Task<byte[]> GetByteArrayAsync(string imagePath, string format, string imageID)
{
var requestUrl = $"{imagePath}&rs:{nameof(format)}={format}&rs:{nameof(imageID)}={imageID}";
return await _client.GetByteArrayAsync(requestUrl);
}
What's wrong here. Not passing any URL. Building url from parameters.
How can i fix this ?
Yes, that's exactly what Veracode Static Analysis reports here. Whenever you compose a URL of parameters that may come from outside the application, our static engine would report CWE 918 whenever an HTTP request to this URL is triggered.
Please make sure to apply validation on all dynamic parameters as per my first comment. At this point in the code, you have certain expectations regarding the contents of each parameter (as an example, an `imageID` might be numeric or alphanumeric?). With input validation, you ensure that the parameters actually conform to your expectations. This is your main wall of defense against this flaw class.
If you would like someone to look over this with you, please feel free to schedule a consultation. The following Knowledge Article describes how to do this: https://community.veracode.com/s/article/How-to-schedule-a-consultation-call
Thank you,
Florian Walter
As per your above suggestion , we tried to put some validation for each of the parameter . But still they have the same issue . Here is the code
public async Task<FileResult> RenderImage(string imagePath, string sessionId, string format, string imageId)
{
new FileExtensionContentTypeProvider().TryGetContentType(imagePath, out string contentType);
if (new System.Text.RegularExpressions.Regex(@"\.\.").IsMatch(imagePath ?? throw new ArgumentNullException(nameof(imagePath))))
{
throw new ExceptionValidation("Invalid imagePath.");
}
if (!(sessionId ?? throw new ArgumentNullException(nameof(sessionId))).All(char.IsLetterOrDigit))
{
throw new ExceptionValidation("Invalid sessionId.");
}
if (!Guid.TryParse(imageId ?? throw new ArgumentNullException(nameof(imageId)), out _))
{
throw new ExceptionValidation("Invalid imageId.");
}
if (!(format ?? throw new ArgumentNullException(nameof(format))).Equals(("HTML5"), StringComparison.OrdinalIgnoreCase))
{
throw new ExceptionValidation("Invalid report format.");
}
var requestUrl = $"{imagePath}&rs:{nameof(sessionId)}={sessionId}&rs:{nameof(format)}={format}&rs:{nameof(imageId)}={imageId}";
var imageData = await _reportingService.GetByteArrayAsync(requestUrl);
}
What else we can try to fix this issue ?
Please note that Veracode Static Analysis will not automatically close flaws based on dynamic validation. This means that you would have to propose a mitigation and discuss it with your Security Team. The following article describes how to propose a mitigation: https://help.veracode.com/r/improve_mitigation .