you could make the diff shorter by simply switching > to < in the existing condition as it will be strictly equivalent to your new code

Yes I know, but following Robert C. Martin (Uncle Bob) principles in "Clean Code" I have chosen the way clearest and intuitive ;-)

I think this is wrong, but one way or another, there's clearly been some confusion on this little line. See #1602 for why it looks how it looks now.

I've never used WSSE, but the purpose of the nonce (as I understand it) is to prevent replay attacks. See http://publib.boulder.ibm.com/infocenter/wsdoc400/v6r0/index.jsp?topic=/com.ibm.websphere.iseries.doc/info/ae/ae/cwbs_noncev6.html.

1. Client send a Nonce and a Created (the date the Nonce was created)
2. We check to make sure that the Nonce is no more than 5 minutes old. Basically, it's looking to make sure the request is "fresh"
3. Since the Nonce will only be valid for 5 minutes (due to the freshness check), we check to make sure that it has not been used during that 5 minute period. If it has, this could be someone making a replay attack (i.e. sniffing my first request, then sending a new one right after with the same nonce).

If I understand this all correctly, I believe the new code is wrong, which is basically:

// Expire timestamp after 5 minutes
if (time() - strtotime($created) > 300) {
    return false;
}

if (file_exists($this->cacheDir.'/'.$nonce) && (time() - file_get_contents($this->cacheDir . '/' . $nonce) ) > 300) {
     throw new NonceExpiredException('Previously used nonce detected');
}

A few cases, using a simple UNIX timestamp of around 1000 for simplicity.

So as I see it, the purpose of this line is to make sure that the nonce can only be used once... until it is regenerated again later. Honestly, it's still a bit fuzzy for me, and checking the times as we are now seems very obtuse (and hence why there's confusion).

What do you guys think? Much better than discussing this (because it's really not a Symfony-topic, and trying to logic out security is never a good idea) would be to find a solid implementation and copy it. Does anyone know of one... in any language?

Thanks!

Ryan, you've summed it up nicely and are dead on.

The whole purpose of the Nonce and Created headers is to prevent replay attacks. If the application decides the Created header is valid within five minutes of the current time, then the nonces are only needed until those five minutes are up. After five minutes, the request will be rejected because the Created timestamp is no longer within the allowed margin of time.

So the existing code says "IF Nonce-X exists, AND this request is within five minutes of when Nonce-X was used, throw an exception. Otherwise, let it through and set Nonce-X to the current timestamp."

This PR does the opposite. It says "If Nonce-X exists, AND this request is AFTER five minutes of when Nonce-X was used, throw an exception". So it's wrong.

If you're using something like memcache, this would look a lot simpler, as you'd just be setting Nonce-X to expire after 5 minutes, and there'd be no need for the logic above. But we're not that smart in this example :P

... Maybe we'd be better off changing to OAuth? :)

Hi guys!

I've added some small comments at sha: 6833c30. I want this part to be understandable, however it's also something specific to WSSE, and this post is meant to teach people about Symfony's custom authentication. If you'd like to make some improvements on mine change, please do!

Thanks!

I like it

—
Brent Shaffer

On Sat, Nov 16, 2013 at 3:52 PM, Ryan Weaver notifications@github.com
wrote:

Closed #3134.

Reply to this email directly or view it on GitHub:
#3134

    public function authenticate(TokenInterface $token) {
        $user = $this->userProvider->loadUserByUsername($token->getUsername());

        if ($user && $this->validateDigest($token->digest, $token->nonce, $token->created, $user->getPassword())) {
            $authenticatedToken = new WsseUserToken($user->getRoles());
            $authenticatedToken->setUser($user);

            return $authenticatedToken;
        }

        throw new AuthenticationException('The WSSE authentication failed.');
    }

how to prevent attack while we load user from database first?

@weburnit what do you mean ?

whatever the validateDigest is, we always load $user from database which costs amount of environment!

well, you cannot validate the digest without loading the hashed password from the DB to compare it. Look at the arguments of validateDigest

Hi guys!

@weaverryan My example
See: https://docs.wso2.com/display/IS500/Timestamp+in+WS-Security+to+Mitigate+Replay+Attacks
Bundle with implementation: https://github.com/CedrickOka/oka-api

$currentTime = time();
$lifetime = 300; // Seconds
$cacheDir = '/tmp';

// Check that the timestamp has not expired
if (($currentTime < strtotime($created) - $lifetime) || ($currentTime > strtotime($created) + $lifetime)) {
     throw new AuthenticationException('Created timestamp is not valid.');
}

$nonceDecoded = base64_decode($nonce);
$nonceFilePath = $cacheDir.'/'.$nonceDecoded;

// Validate that the nonce is *not* used in the last 5 minutes
// if it has, this could be a replay attack
if (file_exists($nonceFilePath) && (((int) file_get_contents($nonceFilePath)) + $lifetime) > $currentTime) {
    throw new NonceExpiredException('Digest nonce has expired.');
}

if (!is_dir($this->cacheDir)) {
    mkdir($this->cacheDir, 0777, true);
}

file_put_contents($nonceFilePath, $currentTime, LOCK_EX);

// Valid the secret
$expected = base64_encode(sha1($nonceDecoded.$created.$secret, true));

return hash_equals($expected, $digest);