Proposal: warn about fmt.Sprintf on HTTP request body building

[jimen0]

Hi,

First of all, thanks for creating and maintaining this repository. The set of rules is very useful. I'd like to propose a new rule for detecting HTTP requests where the body is built using fmt.Sprintf() instead of url.Values{}.Encode(). I was bitten by this some time ago and I think a rule could be created for warning about it.

Scenario

An HTTP request is built using one or more user-controlled parameters for the body value.

TL;DR: I crafted this playground to show the different results.

Detected behavior

Body is manually crafted via fmt.Sprintf or string concatenation (+).

// name is a parameter, user supplied
data := fmt.Sprintf("name=%s&order=1", name)
req, err := http.NewRequest(http.MethodPost, "https://example.com/", strings.NewReader(data))

An attacker could supply a value like test&forge=pwn resulting in the following HTTP request being sent.

POST / HTTP/1.1
Host: example.com
User-Agent: Go-http-client/1.1
Content-Length: 27
Content-Type: application/x-www-form-urlencoded
Cccept-Encoding: gzip, deflate
Connection: close
  
name=test&forge=pwn&order=1

Desired behavior

Body is created via url.Values{}.Encode():

// name is a parameter, user supplied
data := url.Values{
    "name":  {name},
    "order": {"1"},
}
req, err := http.NewRequest(http.MethodPost, "https://example.com/", strings.NewReader(data.Encode()))

Then, the request is securely encoded:

POST / HTTP/1.1
Host: example.com
User-Agent: Go-http-client/1.1
Content-Length: 31
Content-Type: application/x-www-form-urlencoded
Accept-Encoding: gzip, deflate
Connection: close

name=test%26forge%3Dpwn&order=1

Please, let me know if a rule like that would fit into this repository. If so, I could try to write it and contribute it. If you think it doesn't fit into this repository, please feel free to close this issue and sorry about the noise I caused.

I'm not 100% sure this being possible using semgrep, though.

Best,
Miguel

[dgryski]

Writing patterns to detect this kind of dataflow might be tricky, but maybe experiment with semgrep's taint tracking support: https://github.com/returntocorp/semgrep/blob/develop/docs/experimental.md#taint-tracking

[jimen0]

Hi @dgryski,

Thank you for taking the time to review my proposal and sharing some docs with me. I highly appreciate that. I'll keep an eye on semgrep's repository in case they ever add something that makes it easier to implement this. I'm closing this issue now to avoid making unneeded noise. Regards! 😄