S5146 false positive?

SonarCloud
,
1

SonarCloud is marking the following as a vulnerability ( javasecurity:S5146 )

What am i doing wrong ?


private static final String[] WHITELISTED_DOMAINS = {
        "domain1.com",
        "domain2.net"
    };

private static final String WHITELIST_ERROR_FORMAT = "%s is not a whitelisted redirect domain";

@Path("/myPath")
public Response myRedirectFunc(ContainerRequestContext ctx) {

  try {
    String thisHost = ctx.getHeaders().get("host").get(0);
    if(Arrays.stream(WHITELISTED_DOMAINS).noneMatch(entry -> thisHost.endsWith(entry)))
    {
      throw new MyException(String.format(WHITELIST_ERROR_FORMAT, thisHost));
    }

    return Response.temporaryRedirect(redirectURI).build();

  } catch (Exception e) {
    return Response.serverError().build();
  }
}
2

Hello @Rajesh_Goswami, thank you for reaching us!

We tried to reproduce your issue, but this was not possible from the portion of code that you posted. Can you provide us an usable reproducer?
In particular the code above does not compile and, in order to help you, it would be interesting for us to see the declaration and the assignments of the redirectURI variable.

Roberto

3

Hello @Roberto_Orlandi

Does the below help ?

Regards…

private static final String[] WHITELISTED_DOMAINS = {
        "domain1.com",
        "domain2.net"
    };

private static final String WHITELIST_ERROR_FORMAT = "%s is not a whitelisted redirect domain";

@Path("/myPath")
public Response myRedirectFunc(ContainerRequestContext ctx) {

  try {
    String thisHost = ctx.getHeaders().get("host").get(0);
    if(Arrays.stream(WHITELISTED_DOMAINS).noneMatch(entry -> thisHost.endsWith(entry)))
    {
      throw new MyException(String.format(WHITELIST_ERROR_FORMAT, thisHost));
    }

    String thisURLFormat = "https://%s%s";
    URI redirectURI = new URI(String.format(thisURLFormat, thisHost,"/some/path.html"));

    return Response.temporaryRedirect(redirectURI).build();

  } catch (Exception e) {
    return Response.serverError().build();
  }
}
4

Hello @Rajesh_Goswami,

Thank you for providing more details.

I can say that this is a true positive, because thisHost could be user-controlled, and we don’t consider endsWith() as a Validator, because using that could still allow to have, as example, something like attackerdomain1.com.
On a side note: even if it was a Validator, SonarCloud would not currently be able to recognize it as it is part of a lambda expression, not supported yet by our engine.

Roberto

5

Thanks for looking into it @Roberto_Orlandi

Do you have a recommendation to fix it ? We would like to have a list of whitelisted domains as opposed to hosts and keep sonar happy. The current suggestion offered by sonar is like below.

What does sonar look at for hosts ?

protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
    String whiteList = "http://localhost/safe";
    String location = req.getParameter("url");

    if (!location.startsWith(whiteList))
      throw new IOException();

    resp.sendRedirect(location); // Compliant
}
7

hi @Roberto_Orlandi do you have any recommendation ? What are the validators that sonar looks for in case of hosts ? i can change the lambda expression, just want to keep sonar happy and still support domain whitelisting. Regards…

8

Hi @Rajesh_Goswami
Thank you for the feedback

We updated the rule description (will be visible in the next release) with this new compliant solution based on an allow list:

protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
    String location = req.getParameter("url");

    List<String> allowedUrls = new ArrayList<String>();  
    allowedUrls.add("https://www.domain1.com/");
    allowedUrls.add("https://www.domain2.com/");

    if (allowedUrls.contains(location))
      resp.sendRedirect(location); // Compliant
}

List.Contains is recognized by SonarCloud as a way to build an allow list of hosts/urls/whatever. As the risk of bypasses is low with this solution we strongly recommend it. But if I understand well, in your case you want to allow all subdomains of some trusted main domains:

  • You can use a regular expression, as soon as you use Pattern.matches SonarCloud validates the input of the regex, our engine doesn’t look for the moment at the pattern, it’s up to you to specify one that is secure / not permissive, this one below for instance seems good to me:
    if (Pattern.matches(".*\\.domain1\\.com$", location))
      resp.sendRedirect(location); // Compliant
  • Or you can parse the URL, retrieve the host with url.getHost() (by doing that our engine validates the url instance) and perform safely a redirection:
    URL url = new URL(location);
    String[] domains = url.getHost().split("\\.");
    if (domains.length > 1 && domains[domains.length - 2].equals("domain1")) {
      resp.sendRedirect(url.toString()); // Compliant
    }

If you have still difficulties, let us know.

Eric

1 Like