thephpleague / glide

Wonderfully easy on-demand image manipulation library with an HTTP based API.
http://glide.thephpleague.com
MIT License
2.55k stars 200 forks source link

Security issue with md5 #153

Open Hacker12345577342 opened 8 years ago

Hacker12345577342 commented 8 years ago

Nowaday there are a lot of tools to decrypte md5 which Glide use for sign on images.when people decrypte one of them they will see our secret key because Glide generate sign like this return md5($this->signKey.':250182131287'.ltrim($path, '/').'?'.http_build_query($params));(I added before some numbers but it doesnt matter there).If someone decrypt key they will my secret key,then they will try change sizes (maybe with some script to automatic generated sizes on the while loop) and then they will md5 this then they will send this sign to Glide server,then Glide server will try to handle a lot of cache and will crash

Alex12341232 commented 8 years ago

Wow it is true ,I can also do this with online decryptes.I didnt think that Glide security use md5,Do you know other libs to do this?because I used Glide a lot,but I need something like this now.

francescolaffi commented 8 years ago

this security concern is probably far fetched in 99.99% of use cases, however it looks like is trivial to override the generateSignature method in https://github.com/thephpleague/glide/blob/master/src/Signatures/Signature.php to use a more secure hashing algo if you deem it necessary

just imho, im not a maintainer of this lib

reinink commented 8 years ago

First of, yes, this can be an issue, which is why we highly recommend using a large signing key.

image

The reason we use md5 is because it's fast. These request happen in realtime, so have a 1+ second encryption delay isn't very practical. That said, there isn't any reason why you couldn't extend the signature class as @francescolaffi is suggesting and add in your preferred signature generation technique.

At this point I'd probably like to get some input from some security experts who know more about this stuff than I do. I'm going to reach out to a couple folks.

I'd like to also see what other libraries that offer similar functionality are doing. Here are a couple examples to start:

paragonie-scott commented 8 years ago

https://github.com/thephpleague/glide/blob/5d71f2fb030907fdd9552b4952d1ecdc1804702e/src/Signatures/Signature.php#L61

You want hash_hmac(). The current construction is vulnerable to length-extension attacks.

Try replacing that line with:

return hash_hmac(
    'sha256',
    ltrim($path, '/') . '?' . http_build_query($params),
    $this->signKey
);

And this line with:

if (!hash_equals($params['s'], $this->generateSignature($path, $params))) {

Also, make sure your key generator uses a CSPRNG. For PHP 5.6 support, see https://github.com/paragonie/random_compat

reinink commented 8 years ago

Thanks @paragonie-scott, I really appreciate your input on this!

kpcyrd commented 7 years ago

The reason we use md5 is because it's fast. These request happen in realtime, so have a 1+ second encryption delay isn't very practical.

@reinink Even then md5 is a poor choice, given that md5 is slow compared to more recent hashing algorithms that aren't broken yet.

blake2 benchmark

baileylo commented 6 years ago

Apologies for digging up and old issue here. Having skimmed the comments in the #170, specifically regarding CDN caching there are a couple issues with the existing signature mechanism I'd like to bring up:

https://github.com/thephpleague/glide/blob/5e4e4fe1bbe13d0cfc3d823bd60231deddf9cbf1/src/Signatures/Signature.php#L33-L62

Operation order is ignored

Sorting the keys in the query string increases the number of valid URLs for a given signature

$sig = new \League\Glide\Signatures\Signature('my-key');
echo $sig->generateSignature('/path', ['w' => 300, 'h' => 200]); // e3c187f739828cdd9ff303d4ea429398
echo $sig->generateSignature('/path', ['h' => 200, 'w' => 300]); // e3c187f739828cdd9ff303d4ea429398

Example Code Shows Decoded URLs

The example code uses $_GET to retrieve the query string parameters. The values in $_GET and $_REQUEST are both URL decoded as noted in the docs. This means that there are even more valid inputs for a given signature. Every character in the value has two valid values, the plain text, , and the url encoded %2C.

None of these work to expose your signature or grant the attacker access to use your service free of charge. They seem better used as DDOS attacks. Find a series of large images on the site and then use the multiple of valid inputs to attack the origin server.

This only effects CDNs with cache read through, not the current implementations where the images are served from an "on disk" cache.

An Inelegant Solution

/**
 * Validate a request signature.
 * @param  string             $path   The resource path.
 * @param  string             $params The raw query string not url encoded.
 * @throws SignatureException
 */
public function validateRequest($path, $query_string)
{
    // Extract Signature From Query String
    preg_match('/&s=([a-zA-Z0-9]+)$/', $query_string, $matches);
    if (!$matches) {
        throw new SignatureException('Signature is missing.');
    }

    // Remove the signature from the end of the query string and sign the data
    $our_signature = hash_hmac(
        'sha256',
        ltrim($path, '/') . '?' . substr($query_string, 0, -1 * (strlen($matches[1]) + 3)),
        $this->signKey
    );

    if (!hash_equals($our_signature, $matches[1])) {
        throw new SignatureException('Signature is not valid.');
    }
}

Your glide controller code would look something like this. This a BC change as it requires $query_string to be a string.

try {
    $this->signature_verifier->validateRequest($request->path(), $_SERVER['QUERY_STRING']);
} catch (SignatureException $e) {
    return new Response('', Response::HTTP_FORBIDDEN, [
        'x-reason' => $e->getMessage()
    ]);
}

return $this->glide_server->getImageResponse($path, $request->all());