security #cve-2019-10913 [HttpFoundation] reject invalid method overr…

…ide (nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpFoundation] reject invalid method override

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

From https://www.intigriti.com/company/submission/CfDJ8Pja6NZvkpNCmx5vVyiGSn7LV-k0ZJ4JlDGSPAaBG1sG1aNinWbVYRos8ldmLPCMSPdHLrwLufz8lXoJ-UNS3XW1_Xkxc7u9rIaENVJ_-nQV_uic7D1tmRhB6PFiBkRgBA

About `Request::getMethod`:

> There will be developers, who expect the http method to be valid and therefore will use the return value unescaped in sql, html or other dangerous places.

this is what this PR improves, forcing only ASCII letters in overridden methods.

> It is possible to set the header to "GET", "HEAD", "OPTIONS" and "TRACE". Because of this, the method Request::isMethodSafe() returns true, although the actual http method is post.

I don't think this creates any issue: not fixed.

> Normally, if you try to provide a request body in a GET-Request, the web server discards the request body. This security functionality can be completely bypassed through this. [...] Recommendation: Remove the parsed body params from the request object, if a method without a body is set.

I don't think this is valid: actually we *do* populate `$request->request` with the body of GET requests when some is sent.

> Even if very rare, some users still use old browsers, where CORS is not available. Or a server admin allowed headers to be cross origin. In those cases this functionality enables CSRF-Attackes, if the developers trusts the http method. (E.g. Shopware does this).

I don't understand this, not addressed.

ping @michaelcullum if you want to answer the person.
And other to review :)

Commits
-------

6ce9991392 [HttpFoundation] reject invalid method override

Request.php

@@ -1290,19 +1290,37 @@ public function setMethod($method)
      */
     public function getMethod()
     {
-        if (null === $this->method) {
-            $this->method = strtoupper($this->server->get('REQUEST_METHOD', 'GET'));
-
-            if ('POST' === $this->method) {
-                if ($method = $this->headers->get('X-HTTP-METHOD-OVERRIDE')) {
-                    $this->method = strtoupper($method);
-                } elseif (self::$httpMethodParameterOverride) {
-                    $this->method = strtoupper($this->request->get('_method', $this->query->get('_method', 'POST')));
-                }
-            }
+        if (null !== $this->method) {
+            return $this->method;
+        }
+
+        $this->method = strtoupper($this->server->get('REQUEST_METHOD', 'GET'));
+
+        if ('POST' !== $this->method) {
+            return $this->method;
+        }
+
+        $method = $this->headers->get('X-HTTP-METHOD-OVERRIDE');
+
+        if (!$method && self::$httpMethodParameterOverride) {
+            $method = $this->request->get('_method', $this->query->get('_method', 'POST'));
+        }
+
+        if (!\is_string($method)) {
+            return $this->method;
+        }
+
+        $method = strtoupper($method);
+
+        if (\in_array($method, array('GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'CONNECT', 'OPTIONS', 'PATCH', 'PURGE', 'TRACE'), true)) {
+            return $this->method = $method;
+        }
+
+        if (!preg_match('/^[A-Z]++$/D', $method)) {
+            throw new \UnexpectedValueException(sprintf('Invalid method override "%s".', $method));
         }
 
-        return $this->method;
+        return $this->method = $method;
     }
 
     /**