app/core/Encryption.php

[paragonie-scott]

Hello, we noticed a couple of things in your Encryption class.

encryptId() is a misnomer; you aren't encrypting anything (there is no key involved). You're merely transforming it in a deterministic way.

encrypt() is using the 256-bit version of Rijndael in CBC mode without authenticating the ciphertext. Unauthenticated ciphertexts can be tampered with by an active attacker. (Aside: If you want the standard AES cipher in 256-bit mode, use MCRYPT_RIJNDAEL_128 with a 32-byte key. Mcrypt is pretty terrible, however, so you might want to just switch to OpenSSL.)

Also refer to How to Encrypt Data in PHP by Defuse Security.

[paragonie-scott]

I'd recommend switching to one of the following:

1. PECL libsodium, if you can manage it. Highly recommended.
2. Otherwise, defuse/php-encryption or \Zend\Crypt is the way to go.

If you're dead set on rolling your own implementation, please read the linked material first.

[OmarElgabry]

Yes, encryptId() is not using any key. You can consider it as not actual encryption, but, i wanted to do something quick, simple, just instead of showing the actual id number.

Most importantly is encrypt():

I went through the links you provided so quickly. What you said is definitely deserves digging into the encryption method. I see that you have already contributed to encryption snippets in PHP.

So, either if you fork, and add your modification to encrypt() method, I would be really grateful or I will do it myself in couple of days.

Thank you!

[OmarElgabry]

I saw defuse/php-encryption

The problem is: An exception will be thrown if encrypted text was tampered by attacker. I don't disagree with this, But, What if i need to log this attack, remove/update a row in database, show system error page, or do anything else. Then, I need to wrap the code with try and catch blocks every time i want to encrypt a text.

[joepie91]

Yes, encryptId() is not using any key. You can consider it as not actual encryption, but, i wanted to do something quick, simple, just instead of showing the actual id number.

"Encryption" without a key is not encryption, and is not useful. It can be trivially reversed, and does nothing to protect any data, whatsoever. If you are not using a key, then you might as well just leave it as plaintext - either way, it shouldn't be called 'encrypt' because that's not what you're doing.

Especially as your project bills itself as "surrounded with powerful security layers", this is a serious issue.

The problem is: An exception will be thrown if encrypted text was tampered by attacker. I don't disagree with this, But, What if i need to log this attack, remove/update a row in database, show system error page, or do anything else. Then, I need to wrap the code with try and catch blocks every time i want to encrypt a text.

Yes. This is the correct behaviour. Errors need to be handled - any 'solution' that doesn't involve throwing an error that needs to be handled, is wrong.

try/catch is not something to be avoided; it is simply the part of the language that you use for appropriately handling errors.

[Blackskyliner]

You could catch the exception in your encryptId() function and return false or null if it fails. This way you can check against values instead of exceptions if those are an problem for you.

But if you say "Security is my thing" then a failure while encrypting is an exception which has to be handled properly.

You could also (maybe) wrap you global Application runner (if existent) in a try catch to render or log an apropriate message.

EDIT: Bit too slow with commenting >.>

[joepie91]

You could also (maybe) wrap you global Application runner (if existent) in a try catch to render or log an apropriate message.

That is almost never a good idea, really. The only global error handler you should ever have is one that does every single one of the following three points:

1. Catches every unhandled error, not just specific types
2. Sends a HTTP 500 with an error message, and
3. Logs the error for review by site operators.

Failures like these should always be handled on a case-by-case basis - otherwise, it becomes too hard to predict what the behaviour of your application is going to be upon encountering an error.

If you are handling an error in the same way in many places, that simply means you need to abstract your code more, ie. better code reuse.

[OmarElgabry]

@joepie91 I am not willing to avoid throwing exception, I am willing to avoid writing the same code over and over again,

What @Blackskyliner says about catching exception inside encrypt() method and return null/false could be more appropriate, So, try and catch blocks will be written only once.

@joepie91 Yes, I agree, I see there are some failures that need to be handled explicitly by wrapping them with try and catch block instead of using the global error handler.

[joepie91]

What @Blackskyliner says about catching exception inside encrypt() method and return null/false could be more appropriate, So, try and catch blocks will be written only once.

No, it isn't. Sentinel values are extremely dangerous, especially in cryptography-related code, as they will fail silently and unpredictably when you forget to handle them. try/catch will fail closed; ie. it will make a lot of noise and stop execution if you forget to handle them, which is the correct thing to do.

The correct solution is to let the error propagate. You should not ever return sentinel values from a function - the only reason there are methods that currently do this, is because try/catch has not always been available as a feature.

[OmarElgabry]

Ok, consider the following situation:

1. A user logged in with his account,
2. the application added an encrypted cookie into user's browser(remember me token)
3. Then, user logged in again after a while,
4. the application gets the remember me token cookie,
5. It detected the token has been modified

So, according to your suggestion(as i understood so far), Inside decrypt() method, an exception will be thrown, and caught.

Now, Regardless of the implementation, I would like to:

1. Log this error; modified encrypted token
2. remove/clear token from database and browser
3. Logout user

How can i achieve this in case i won't return a value null/false value indicating the token is invalid. I understand your point of view, but, i am trying to get into intermediate point between your suggestions and the logic of the code.

[joepie91]

The responsibility of the decrypt() method ends at "either decrypt the cookie, or throw an error if invalid". It should not have any knowledge of what the data is for, or what to do if it's invalid - it simply throws an error.

The point where you should handle this error is most likely the point where decrypt() is called. At this point, you can catch the decryption error if it occurs, and take further application-specific steps (eg. clear cookie, log out user).

The key points, to reiterate; a function:

1. ... only has one, single responsibility, and
2. ... is not aware of the rest of your application, nor does it modify any state anywhere - it simply accepts arguments, and provides a return value, independent of what the rest of the application is doing.

This results in loosely coupled code, which is far less likely to contain bugs, and far easier to audit. If a function only has one purpose and has no side-effects, then you can safely reuse and change it as needed, as long as the arguments and return values / errors remain compatible.

[OmarElgabry]

I am aware of these principles, as well as DRY. I am not going to re-write try and catch block where the decrypt() and encrypt() methods are called.

In my application, I would call decrypt() and encrypt() methods about 10 times, should I re-write try catch block every time?

I didn't say decrypt() will do anything else other than decrypt, if failed, then return null, false, or as you are suggesting throws an exception.

Again, I am not trying to arguing, we are trying to get to a suitable solution.

[joepie91]

I am aware of these principles, as well as DRY. I am not going to re-write try and catch block where the decrypt() and encrypt() methods are called.

You should.

In my application, I would call decrypt() and encrypt() methods about 10 times, should I re-write try catch block every time?

Yes. They are used in 10 different situations, and could need 10 different kinds of error handling. You're going to have to handle the errors in all those 10 places anyway - throwing an error simply ensures that you're not missing a spot.

Again, try/catch isn't something to avoid or reduce. It's a language construct that ensures you are handling errors correctly.

I didn't say decrypt() will do anything else other than decrypt, if failed, then return null, false, or as you are suggesting throws an exception.

Right. Then throwing an error should not be an issue, as the handling of decryption errors is context-dependent and needs to happen outside of the decrypt() function anyway.

[OmarElgabry]

Ok, Now, everyone has a point of view.

You: Suggest to wrap encrypt() and decrypt() methods with try & catch blocks whenever they are used. So, if they are used 10 times, then each one should be wrapped by try & catch blocks.

Me: I don't see re-writing the same code for error handling for the methods encrypt() and decrypt() again and again is something will keep your code nice and clean. Duplicates are always not a good practice. Yes, they will be used in different situations, but the same approach for error handling(i.e. redirect to not found error page)

You will ending up having different places in your code, each does the same thing. So, If there is any bug, or modification, you will have to fix/modify the same piece of code independently.

And What I am saying does NOT discouraging from using try & catch blocks.

Finally: I totally understand your point, I don't have an ideal solution in my mind instead. The most suitable solution that I see till now, is to return false (as long as It will be handled correctly) If encrypted string was modified(salt/key is wrong). Otherwise, throws an exception inside encrypt() and decrypt() that will be caught either by Exception Handler, or try & catchblock inside encrypt() and decrypt().

What I am saying is not something I've invented, nor fabricating. It's already implemented in PHP Frameworks(as I understood):

• CakePHP
  • CakePHP Book
  • CakePHP Security Class
  • CakePHP Utility Crypto
• Laravel
  • Laravel Encrypter

These frameworks are used by millions of developers and have been battled and tested by many developers on different circumstances.

[joepie91]

The problem is that you are confusing two different aspects - the error throwing and the error handling.

As I said before, the responsibility of the decrypt() method ends at attempting to decrypt or throwing an error if it can't. It doesn't know about sessions or logging or page redirects - it just knows about decryption, and it should remain that way.

If you need the same "remove cookie, redirect to other page" logic in more than one place, then it's perfectly fine to abstract that, but not within the decrypt() method - it's out of scope. If you genuinely need the same error handling in all places, then you simple write another function - say, decryptOrLogout() - that calls decrypt() and logs the user out and aborts execution if the decryption fails.

The most suitable solution that I see till now, is to return false (as long as It will be handled correctly) If encrypted string was modified.

No. "Returning false" is wrong, no matter how you modularize your code or what the error handling is like. A return value is never, ever, ever an error. That is what error throwing is for.

Otherwise, throws an exception inside encrypt() and decrypt() that will be caught either by Exception Handler, or try & catchblock inside encrypt() and decrypt().

See above suggestion. And when correctely modularized, nothing needs to be returned anyway, as the error condition aborts any further page execution. There is no rule that says you can only have a single function for the entirety of this functionality.

What I am saying is not something I've invented, nor fabricating. It's already implemented in PHP Frameworks(as I understood):

[snip]

These frameworks are used by millions of developers and have been battled and tested by many developers on different circumstances.

This is an appeal to authority (or appeal to popularity, depending on how you look at it) and completely irrelevant here.

A large framework using a particular approach is in no way a reason to use the same approach. Not only do many widely used frameworks and applications have security issues, you also do not know the context in which certain decisions are made, nor whether your situation might be different from theirs.

The only correct way to make security-related decisions is to have a solid understanding of what is going on, and why certain approaches are preferable over others. This is a purely objective process, there is no room for personal opinion. Something is either optimally secure or it is not.

What anybody else does is irrelevant, if you don't have the objective reasoning to back it up.

[OmarElgabry]

If you need the same "remove cookie, redirect to other page" logic in more than one place, then it's perfectly fine to abstract that, but not within the decrypt() method

Did you hear me saying decrypt() will take care of anything else other than decryption?, If failed then as you are suggesting throws an exception

The moral of the long argument is, you say decrypt() should throw an exception if the encrypted string was modified(this exception should be caught every time the method is called), I say in this particular situation, returning false is more suitable, and throw an exception otherwise(i.e. if internal failure occurred).

This is an appeal to authority (or appeal to popularity, depending on how you look at it) and completely irrelevant here.

Yes, I agree. Not because a framework uses a particular approach, then this would be the most optimal one. But, We still can't discard the claim of those developers in charge of building these frameworks, and their situation is nearly the same here.

I am not an expert, that's why i jumped over their implementation to see how they take care of these kind of issues, why a framework would prefer to use certain implementation over the other. Your knowledge and experience is an accumulation of some books, talks by others, code written by good developers, and so forth, and your decision will be based on it.

See, I am not a big fan of writing long comments, So, we need to put an end to this. I am really thankful for your suggestions, and I will take it forward, except throwing an exception if encrypted string was tampered. You thankfully suggested something, someone else could have another point of view.

[joepie91]

Did you hear me saying decrypt() will take care of anything else other than decryption?

That is what you seem to be suggesting, yes. The "redirect to a not found page", specifically.

The moral of the long argument is, you say decrypt() should throw an exception if the encrypted string was modified(this exception should be caught every time the method is called), I say in this particular situation, returning false is more suitable, and throw an exception otherwise(i.e. if internal failure occurred).

Yet you've completely failed to provide any valid arguments for that whatsoever. You are going to have to handle the error anyway, regardless of whether it's a thrown exception or a return value of false. However, when you use a return value of false, as I have explained, you will have silent failure if you forget to handle it.

No, there are no cases where returning false on an error is a valid thing to do. None. Nada. Zero. Squat.

But, We still can't discard the claim of those developers in charge of building these frameworks, and their situation is nearly the same here.

They can be discarded just fine if they fail to address the issues I (or anybody else) am raising. They do not become any less wrong just because they were made in the context of a large framework. The frameworks are completely, 100% irrelevant here.

So, we need to put an end to this. I am really thankful for your suggestions, and I will take it forward, except throwing an exception if encrypted string was tampered. You thankfully suggested something, someone else could have another point of view.

This isn't about "another point of view". Returning false is objectively the wrong thing to do, and is dangerous. I've tried to explain the reason for this (silent failure) several times now, and you've not responded to that issue even a single time - not to contradict it, not to ask for elaboration.

And frankly, I feel like you don't even really understand what I'm trying to explain to you, because there are no arguments in favor of returning false here. It literally doesn't matter in any way, other than allowing for silent failure, which should never be possible.

[OmarElgabry]

If I replied, this conversation won't end. I am thankful for your time, I am sure I understand what you are trying to address here.