glewio / glew-magento2-service

GNU General Public License v3.0
3 stars 3 forks source link

glew/module/version controller exposes data and is a security issue #12

Open kirkmadera opened 4 years ago

kirkmadera commented 4 years ago

glew/module/version publicly exposes data which could be used to target known vulnerabilities in specific versions of software. This data should be behind the admin login, have an enable/disable flag with a security warning (defaulted to disabled), or should not exist at all.

    {
        "glewPluginVersion": "2.1.4",
        "magentoVersion": "2.3.5-p1",
        "magentoEdition": "Enterprise",
        "phpVersion": "7.3.19",
        "moduleEnabled": "1",
        "apiVersion": "2.0",
        "memoryLimit": "756M",
        "maxExecutionTime": "18000"
    }
verus-agency commented 3 years ago

This is a concern for us as well. Is there an update on this? @nastroth @chrisbyronc @drewlshoaf

kirkmadera commented 3 years ago

I think that this is what the intent of \Glew\Service\Controller\Module::isAuthorized.

protected function isAuthorized() {

    $token = $this->helper->getConfig()['security_token'];
    $authToken = (isset($_SERVER['HTTP_X_GLEW_TOKEN']) ? $_SERVER['HTTP_X_GLEW_TOKEN'] : $_SERVER['X_GLEW_TOKEN']);

    if (empty($authToken)) {
        return false;
    }

    if (trim($token) != trim($authToken)) {
        $this->helper->log('Glew feed request with invalid security token: '.$authToken.' compared to stored token: '.$token);
        return false;
    }

    return true;
}

You now have to pass an "X-Glew_Token" HTTP header on all requests with your glow token (from the admin).

kirkmadera commented 3 years ago

Can you update this code to use an injected request object or, at the very least, PHP's header() function?

The main reason for this is that, I read the code and saw that it was looking for a $_SERVER var and immediately went to Nginx and added this as a fastcgi param so that $_SERVER['X_GLEW_TOKEN'] would be available. I think this is counter to the intent of this code and is dangerous. Use of $_SERVER['HTTP_WHATEVER'] to get an HTTP header is so uncommon that I had to look up the feature and I think a lot of people are going to do what I did, leaving their site data wide open.

A non-breaking way to implement this, conforming to Magento standards would be something like this:

protected $request;

public function __construct(
    RequestInterface $request
) {
    $this->request = $request;
}

protected function isAuthorized()
{
    $authToken = $this->request->getHeader('x-glew-token');
    if (!$authToken) {
        $authToken = $_SERVER['X_GLEW_TOKEN'];
    }
}
kirkmadera commented 3 years ago

The confusion for us was that the glew/module/version path was available and then stopped working in version 2.1.6. I found that an isAuthorized check was added to this specific controller in 2.1.6.

It fails with a Magento error because this check is bad and does not also check if $_SERVER['X_GLEW_TOKEN'] is set so it throws a Notice.

Fixing the header check to use the RequestInterface object would avoid this confusion and the notice.