Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #87: security helper converted into component and improved #4089

Merged
merged 19 commits into from Jun 28, 2014

Conversation

klimov-paul
Copy link
Member

Fix #87: security helper improvement

Migrated from #4052

@@ -17,7 +17,7 @@ When a user provides a password for the first time (e.g., upon registration), th


```php
$hash = \yii\helpers\Security::generatePasswordHash($password);
$hash = \yii\helpers\Yii::$app->getSecurity()->generatePasswordHash($password);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\yii\helpers has to be removed in this document.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@cebe cebe modified the milestones: 2.0 GA, 2.0 RC Jun 27, 2014
@Ragazzo
Copy link
Contributor

Ragazzo commented Jun 27, 2014

Should we then change User entities ? What the approach is better:

  • security service injection with __construct ;
  • security service as a parameter to generatePassword($generator, $password).

Which one to choose ? Maybe last one is better IMO

@klimov-paul
Copy link
Member Author

Should we then change User entities ?

What do you mean. User model and related fixtures from advanced application are fixed with this PR.

@Ragazzo
Copy link
Contributor

Ragazzo commented Jun 27, 2014

ah, i see so you make it with simple replace , ok

$chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-.';

return substr(str_shuffle(str_repeat($chars, 5)), 0, $length);
return mcrypt_create_iv($length, MCRYPT_DEV_URANDOM);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mcrypt should be put in the minimum requirement of Yii then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can keep orignal fallback.

@qiangxue
Copy link
Member

Looks good to me overall. Good job!

@klimov-paul
Copy link
Member Author

TODO: advance hashPassword().

@SDKiller
Copy link
Contributor

Btw, what if password and encryption functions would be split into separate classes, i.e. passwordManager and encryptionManager (or whatever you like)?

Then calling for example Yii::$app->getSecurity()->encrypt(); will instantiate corresponding class if it has not been already instantiated and call corresponding method.

Then they could be configured separately or overriden, for example:

  return [
      'components' => [
          'security' => [
             'encryptionManager' => [
                 //override default config of default encryptionManager
                 'cryptBlockSize' => 16,
                 'cryptKeySize' => 24,
                 'derivationIterations' => 1000,
              ],
             'passwordManager' => [
                 //set custom password manager and add some more params if nesessary
                 'class'            => 'my\custom\passwordManager',
                 ...
              ],
         ],
          // ...
];

@klimov-paul
Copy link
Member Author

Such separation has been already rejected.

* Static helper `yii\helpers\Security` has been converted into an application component. You should change all usage of
its methods to a new syntax, for example: instead of `yii\helpers\Security::hashData()` use `Yii::$app->getSecurity()->hashData()`.
If you have used `yii\helpers\Security` for encryption or hash generating, you need to explicitly configure 'security'
component for the legacy code support in following way:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make more clear that the following is not recommended but only for apps that must reuse cyphertexts generated with the previous version:

Yii has upgraded its default encryption and hash parameters since Yii 2.0 Beta. If you need to decrypt/validate data that was encrypted/hashed before this upgrade, use the following configuration of the 'security' component:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

],
// ...
];
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add another note:

You may configure two components 'legacySecurity' and 'security' with different configurations to use the legacy component to deal with values encrypted with the old class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but insert "and re-encrypt/re-hash using the 'security' component" at end.

@tom--
Copy link
Contributor

tom-- commented Jun 27, 2014

I'm uncomfortable with the fallback to PHP iteration of hash_hmac() in deriveKey(). I understand that hash_pbkdf2() is only in PHP 5.5.0+ but I think we shouls be consistent in taking a more strict approach to security.

We sacrificed portability with the original password helper because crypt() with blowfish option was the only good implementation. People complained and we said no, if you want to be insecure, do it without our help.

With this revision from @klimov-paul we are requiring mcrypt. That's a good decision consistent with the idea that if you want security you need to meet certain platform requirements.

But fallback to PHP iteration in deriveKey() in absence of hash_pbkdf2() is obviously inconsistent. This revision upgrades $derivationIterations from 1000 to 1,000,000 because cracking speed is now very high, which proves that nobody should be using PHP iteration. In order to make that work in acceptable time, users will have to reduce the number of iterations.

@ekerazha
Copy link
Contributor

You are talking about special "non practical" case, when AES-192 proof itself better then AES-256. But for general attacks this are opposite and AES-256 is better then AES-192.

Define "general attacks". Exhaustive search? Nope.

@ekerazha
Copy link
Contributor

This is a choice of the security expert we have contact with. I have quoted this opinion above.

Based on the papers that I posted, it is a bad choice.

@klimov-paul
Copy link
Member Author

Based on the papers that I posted, it is a bad choice.

At papes, which you have posted says it is no reason to worry about AES-256. It said there rather clearly.

@ekerazha
Copy link
Contributor

Repetita iuvant:

is there any reason why you should deliberately choose the algorithm with the poorest security margin?

@cebe
Copy link
Member

cebe commented Jun 29, 2014

@ekerazha are you only refering to the abstract of the paper or have you read it in full? As far as I understand from the abstract the decision to make here needs complete information about the whole details of the paper content.

@ekerazha
Copy link
Contributor

@cebe I read it, and although I'm not a cryptography expert, I'm a graduated computer engineer and I perfectly know what "computational complexity" is. This is not philosophy and these are numbers:

AES-128: 2^126.1 (biclique attack)
AES-192: 2^176 (related-key attacks)
AES-256: 2^99.5 (related-key attacks)

@cebe
Copy link
Member

cebe commented Jun 29, 2014

I see your point with comparing complexity but that is not all that has to be considered. There may be other attacks that have different complexity and may be more practical than the ones described in the paper.

There also seem to be a bunch of papers with more up to date information. Yours is from 2009. To decide on this topic we should also check out information in more up to date papers describing differnt attacs.

@klimov-paul
Copy link
Member Author

Here are the quotations from the links you provided:

http://eprint.iacr.org/2009/374 says:

While these complexities are much faster than exhaustive search, they are completely non-practical, and do not seem to pose any real threat to the security of AES-based systems.
...
neither AES-128 nor AES-256 can be directly broken by these attacks

Conclusion at http://eprint.iacr.org/2009/317 says:

However, both our attacks are still mainly of theoretical interest and do not present a threat to practical applications using AES

Conclusion at https://www.schneier.com/blog/archives/2009/07/another_new_aes.html says:

And for new applications I suggest that people don't use AES-256. AES-128 provides more than enough security margin for the forseeable future. But if you're already using AES-256, there's no reason to change.

Can you point a sentence, which explicitly says AES-192 should be preferred to AES-256?

@ekerazha
Copy link
Contributor

There may be other attacks that have different complexity and may be more practical than the ones described in the paper.

I took what currently is the best attack for every algorithm: biclique attack for AES-128 and related-key attacks for AES-192 and AES-256.

@ekerazha
Copy link
Contributor

Can you point a sentence, which explicitly says AES-192 should be preferred to AES-256?

Repetita iuvant:

AES-128: 2^126.1 (biclique attack)
AES-192: 2^176 (related-key attacks)
AES-256: 2^99.5 (related-key attacks)

@cebe
Copy link
Member

cebe commented Jun 29, 2014

I took what currently is the best attack for every algorithm: biclique attack for AES-128 and related-key attacks for AES-192 and AES-256.

Can you give a reference that states that these are the "best attacks"?

@ekerazha as said before, just numbers are not the only thing to decide on this topic.

@ekerazha
Copy link
Contributor

Can you give a reference that states that these are the "best attacks"?

If you want a "summary": http://en.wikipedia.org/wiki/Advanced_Encryption_Standard

Best public cryptanalysis
Attacks have been published that are computationally faster than a full brute force attack, though none as of 2013 are computationally feasible:[3]
For AES-128, the key can be recovered with a computational complexity of 2126.1 using the biclique attack. For biclique attacks on AES-192 and AES-256, the computational complexities of 2189.7 and 2254.4 respectively apply. Related-key attacks can break AES-192 and AES-256 with complexities 2176 and 299.5, respectively.

@ekerazha
Copy link
Contributor

as said before, just numbers are not the only thing to decide on this topic.

It is. Cryptography is math.

@klimov-paul
Copy link
Member Author

Once again your own links prove your wrong:

http://en.wikipedia.org/wiki/Advanced_Encryption_Standard says:

Attacks have been published that are computationally faster than a full brute force attack, though none as of 2013 are computationally feasible

@klimov-paul
Copy link
Member Author

You are pointing to the abstract mathematical experements and algorithm, wich in some abstract case can break AES algorithm, while NONE of them is feasible!

@klimov-paul
Copy link
Member Author

While completely ignoring the common threats, which higher key size opposites better.

@dilip-vishwa
Copy link
Contributor

77a186b9e9f8c21e26d3f64dc9e8009ffee137de81ba7a942cd5af4fcbad9532s:32:"�Ï��:�)Û²È1Wa)��©e�>��H7JÑ'd¾×À�";

as the cookie of csrf token.
This seems to be case after updating Yii now.

@ekerazha
Copy link
Contributor

While completely ignoring the common threats, which higher key size opposites better.

You still have to define "common threats" or "general attacks". Please stop spreading urban legends and bring scientifically valid arguments like I did.

@ekerazha
Copy link
Contributor

You are pointing to the abstract mathematical experements and algorithm, wich in some abstract case can break AES algorithm, while NONE of them is feasible!

That's true, but you still have to explain why you should deliberately choose an algorithm with a poorer security margin (AES-256) when you can choose an overall better algorithm (AES-192).

@ekerazha
Copy link
Contributor

In a nutshell... computational complexity for key recovery against AES:

Exhaustive search

  • AES-128 -> 2^128
  • AES-192 -> 2^192
  • AES-256 -> 2^256

Biclique attack

  • AES-128 -> 2^126.1 [best attack against AES-128]
  • AES-192 -> 2^189.7
  • AES-256 -> 2^254.4

Related-key attack

  • AES-128 -> N/A
  • AES-192 -> 2^176 [best attack against AES-192]
  • AES-256 -> 2^99.5 [best attack against AES-256]

At the moment every attack is not computationally feasible (AES is not broken).

The point is that the best thing is to choose the algorithm with the best security margin. Actually, it is AES-192 because the best attack against AES-192 has a 2^176 complexity, while the best attack against AES-256 has a 2^99.5 complexity (2^99.5 < 2^176) and the best attack against AES-128 has a 2^126.1 complexity (2^126.1 < 2^176). So, while these attacks are all infeasible, AES-192 has the best security margin (at the moment). And while it's true that related-key attacks are more difficult to be performed (because you need relations between different keys), they can be performed (still, at the moment a key recovery is not computationally feasible, as I've already said). So, while AES-128, AES-192 and AES-256 are all safe to use (at the moment), AES-192 has the overall larger security margin and it is the most balanced algorithm (at the moment).

Moreover, as I've already said, AES-128, AES-192 and AES-256 use a 128 bit block size, so that's another thing to fix (in the merged code it says that AES-256 uses a 256 bit block size).

/**
* @var string strategy, which should be used to generate password hash.
* Available strategies:
* - 'password_hash' - use of PHP `password_hash()` function with PASSWORD_DEFAULT algorithm. This option is recommended,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't entirely understand PHP's documentation for PASSWORD_DEFAULT.

I think it says that PASSWORD_DEFAULT is – at present – the same as PASSWORD_BCRYPT in all versions of PHP >= 5.5.0 but that it may change in the future if PHP decides to adopt a stronger algorithm.

Can someone verify this for sure?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PASSWORD_DEFAULT will change over time. Check out the RFC Documentation for more details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ircmaxell thanks. Can you confirm that PASSWORD_DEFAULT at present amounts to the same thing as PASSWORD_BCRYPT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@ircmaxell
Copy link

Ok, here's my $0.02:

AES-256 is weaker than AES-192

In specific cases, yes it is. In the general case, no it is not. In the specific case used here, it's not.

Related key attacks are extremely useful when you're deriving keys. If you're using a single key, this is not a problem. If you're deriving keys similar to how WAP did, you're in big trouble.

In the case used here where it's used as a simple block cipher with random keys (or strongly derived keys using PBKDF2 or HKDF), AES-256 is still significantly stronger.

So yes, the best possible attack against AES-256 is faster than the best possible attack against AES-192, the usage here isn't likely to trigger that style of attack. Therefore, the best case attack is still 2^254.4

The Bottom Line

I think it's worthless to discuss. PHP isn't a strong enough language to provide robust crypto in the first place. The presence of side-channel attacks and the chances of an implementation error are going to be the security threats.

If you're storing data where the attacks against AES are important, then they are going to be far more effective by attacking PHP than the AES encrypted data. Let's be real with the expectations of the secured data...

* - 'pbkdf2' - PBKDF2 key derivation. This option is recommended, but it requires PHP version >= 5.5.0
* - 'hmac' - HMAC hash key derivation.
*/
public $deriveKeyStrategy = 'hmac';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the default to "pbkdf2"? I know it won't work in PHP 5.4 but the exception thrown alerts the developer to the security question and explains how to fix it. I think this is a good way to nudge people to 5.5 if they want to use key stretching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's a good idea. 5.5 isn't that wide adopted yet and it's very likely that people will use 5.4 sice framework itself runs well on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I withdraw my request. I ran some tests and found that the PHP loop implementation is much faster than I imagined. It's something like 30% slower than hash_pbkdf2(). Test code here

There are discrepancies with the IETF test vectors that I will report in an issue.

@ekerazha
Copy link
Contributor

ekerazha commented Jul 1, 2014

In the case used here where it's used as a simple block cipher with random keys (or strongly derived keys using PBKDF2 or HKDF), AES-256 is still significantly stronger.

It's a curious statement when you consider the current implementation (PHP fallbacks etc.).

the usage here isn't likely to trigger that style of attack.

It could be possible, therefore AES-192 is better than AES-256 because it is the algorithm with the more balanced security margin.

@tom-- tom-- mentioned this pull request Jul 1, 2014
tvdavid pushed a commit to tvdavid/yii2 that referenced this pull request Jul 24, 2014
Fix yiisoft#87: security helper converted into component and improved
@klimov-paul klimov-paul deleted the 87-security-component-3 branch June 6, 2015 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve SecurityHelper