confidentialString function uses hard-coded key

[asgrim]

https://github.com/OWASP/phpsec/blob/master/libs/crypto/confidentialstring.php#L37

This looks like a hard coded key that is publicly available on the internet, plain for anyone to see and easily decrypt anything encrypted by this function.

Please tell me it's not the case? :cry:

[paragonie-scott]

@abiusx Let me see if I can elaborate:

• Threat modelling is important and relevant: The browser ****** is a mitigation against shoulder-surfing. The closest parallel to source code isn't encrypting the field, it's never using hard-coded credentials, and definitely never storing them in version control. (.gitignore is wonderful.)
• That this library is in a directory named /lib/crypto/ and is called Encryption means it is highly likely that developers would expect it to serve as a general purpose cryptography library.

Says who?

Just about every cryptographer on the Earth? There are valid use cases for encrypting rather than hashing, but you really ought to know what you're doing. Unless you're building a password manager or equivalent functionality, it's probably a mistake.

[SvenRtbg]

What do you do with the password for your database? Hashing is not an option.

This function addresses this usecase, but it feels weird. And it does patching of the original PHP source, so it requires the password to be hardcoded.

Am 25. November 2015 18:19:46 MEZ, schrieb AbiusX notifications@github.com:

"The fact is, this is still an issue. This "confidentialString" function looks, and acts like encryption. If the intention is to "mask passwords" I'm not sure what possible use case this would have in PHP. The only thing you should ever do in a PHP application with a password is hash it, end of."

I agree with the first part of your sentence. The use-case is not communicated properly.

As to what use it might have, the exact same use as the browsers replacing passwords with *, and is part of the security standard. If someone looks at them, they can't know what they are.

" The only thing you should ever do in a PHP application with a password is hash it, end of."

Says who?

---

Reply to this email directly or view it on GitHub: https://github.com/OWASP/phpsec/issues/108#issuecomment-159678789

Mit freundlichen Grüßen

Sven Rautenberg

[asgrim]

@abiusx as @paragonie-scott pointed out, there's only rare cases you need to decrypt a password, and when you do, you MUST do it properly, and THIS is not properly. When dealing with 99% of cases (i.e. normal user authentication), a password should simply be hashed and discarded.

[kelunik]

You will always need access to the cleartext password when connecting to something like MySQL. Doesn't matter which insecure obscurity method you use. It's still readable when not properly encrypted. You can only prevent that with a unknown key.

[paragonie-scott]

Search github for uses of encryption in PHP projects and you'd be amazed.

Okay, either you know who I am and are being condescending, or you don't and are bluffing. Either way, I'm offended.

[kelunik]

Which is not useful in any way in a PHP application.

[ircmaxell]

Question for @abiusx what is the specific threat and/or vulnerability you are attempting to protect against with this library?

[paragonie-scott]

@paragonie-scott I don't know who you are but don't understand what it has to do with bluffing.

You said:

Search github for uses of encryption in PHP projects and you'd be amazed.

See: https://paragonie.com/security <- I'm not exactly amazed here.

[kelunik]

You type passwords in browsers and terminals everywhere, in your office but maybe also in presentations or in public transport. You usually don't do that with server credentials. You either put them into an access restricted file or one that might even be encrypted. But for that encryption you'd need a secret key as well that has to be stored somewhere. The thing you do here not encryption done right.

[paragonie-scott]

@paragonie-scott so you found 7 vulnerabilities. Is that basis for bringing down all PHP projects?

When it comes to searching for encryption in PHP projects: Been there, done that, reported so many redundant vulnerabilities most of which I don't even bother writing up on Full Disclosure or my company's website.

I highly recommend (for any PHP developer) reading up on basic cryptography concepts.

I'm not bashing PHP, I'm just saying that your remark "search this and you'd be amazed" can be interpreted as an insult given my background, but I'm going to ignore that since you confessed you were not aware of it.

Do not store credentials in source code files.

https://github.com/vlucas/phpdotenv + .gitignore is a better solution. Keep your ENV files outside of your web root and you've mitigated all the threats you can reasonably mitigate.

[paragonie-scott]

Also, please answer @ircmaxell's question, @abiusx.

[Manouchehri]

@abiusx Since it's not encryption, what's the point of masking again? It seems just as safe to store things in plaintext, no?

[jonathan-daniel]

@abiusx You keep talking about the "intended use-case", but what is this use-case? And don't say "hiding sensitive data" because that's not what this library is doing, its not hiding anything, only writing it in another form.

[abiusx]

"Also, please answer @ircmaxell's question, @abiusx."

Where is it?

[paragonie-scott]

https://github.com/OWASP/phpsec/issues/108#issuecomment-159681374

Question for @abiusx what is the specific threat and/or vulnerability you are attempting to protect against with this library?

[paragonie-scott]

Disclosure of sensitive information by quick looking through the files.

Easy solution:

1. Make the sensitive information not part of the source code, make it part of an external configuration file that is never checked into version control and only stored in production outside of a publicly accessible directory (i.e. the document root).
2. There is nothing else you need to do. Anything further is security-through-obscurity.

[AndrewCarterUK]

@abiusx PHP already provides an API for what you're suggesting.

http://php.net/manual/en/function.str-rot13.php

[Manouchehri]

@abiusx

The same point browsers take when masking passwords. Prevent disclosure by quick looks.

Sorry, I'm not sure if I follow. Quick looks by who?

[kelunik]

Yes, this library does exactly security through obscurity. It's not always bad.

Relying on it is always bad.

[paragonie-scott]

Plus, putting it in "non-source code files" increases the risk of LFD.

How does this code stop the LFD? An attacker who can look at your source file can also presumably look at the code that obfuscates it.

Steps to pwn:

1. Find LFD vulnerability.
2. Look at obfuscated string.
3. Look at code that obfuscated the string, copy code, deobfuscate it.

At its core, this library is almost like a really weak attempt at DRM.

[paragonie-scott]

I'd appreciate arguments or citations instead of personal opinions.

https://en.wikipedia.org/wiki/Kerckhoffs's_principle

GG

[paragonie-scott]

Your proposed solution increases the chances of LFD

No it doesn't. If there's a vulnerability that can cause LFD, that vulnerability exists regardless.

[paragonie-scott]

https://github.com/OWASP/phpsec/issues/108#issuecomment-159689730

@paragonie-scott Kerckhoff's principle applies to cryptography, not application security.

I'm going to print this quote out and frame it.

I'm done. This is too stupid to not be a troll.

[kelunik]

What's LFD / LFI?

[paragonie-scott]

Local File Disclosure / Local File Inclusion

[Manouchehri]

@abiusx

If I have access to your computer for two minutes, it's going to get a remote shell (which will work indefinitely). Nobody is going to spend all their time going through PHP files if they have a limited attack window.

If you present a slideshow with a password "masked" with the same key as everyone else, somebody in the crowd is going to pick it up. (Assuming it's an experienced security audience.) Given that this thread is getting somewhat popular, it'll likely come up on the first page of Google sooner or later for "OWASP phpsec password hash".

[joepie91]

[Preventing] disclosure of sensitive information by quick looking through the files.

Not possible. End of story. If you don't want it disclosed, don't put it in there.

[jonathan-daniel]

@abiusx

I can easy reverse Rot13 in my brain. I can also remember those characters and do it later using a computer. I can't do either with a proper encryption function.

Well with this library you don't have to remember anything because you can look up the key in this repo. The only currently secure method is either hashing (perfect for passwords) or proper encryption with a true random key.

[jonathan-daniel]

@abiusx We can't provide better ideas if you don't explain your use-case.

[joepie91]

@joepie91 Please let me know if you have any better ideas.

Not shipping the information to begin with.

[paragonie-scott]

I don't think it is easy to remember a maximum entropy string.

Oh good, I was worried someone might think to take a photo or screenshot, but I guess that's impossible.

[Manouchehri]

@abiusx My suggest is to store the password in plaintext to avoid misleading users.

This reminds me of Snapchat hardcoding their encryption key into the app. You spot it instantly as it's the only weird looking string in a class related to encryption. https://gibsonsec.org/snapchat/snapchat_gibsonsec.txt

[Manouchehri]

@abiusx It provides transparency. (It obviously does not increase security at all.)

[joepie91]

but it's better than plaintext

It's not better than plaintext if you're shipping the key along. It's worse - it provides a false sense of security, which is likely to make people act more recklessly.

[joepie91]

@joepie91 I don't agree with your "false sense of security" argument. Please provide use-cases where it actually worsens things.

There's nothing to "agree" with. This is how it works. I've already given you a usecase.

[Manouchehri]

@abiusx Right now users will likely falsely believe their credentials are properly encrypted if they do not look at the source code or this thread.

[joepie91]

@joepie91 I don't respond to force. Either provide an argument or withdraw your case.

This isn't "force". This is attempting to explain something to you that has been well-established in the area of cryptography for the last few decades. You can either agree or be wrong. There's no arguments left to be made here, that haven't already been extensively made and documented.

EDIT: Also outside of the area of cryptography, by the way. This is generally how people respond to their environment, from a psychological point of view.

[Manouchehri]

@abiusx You're hiding credentials from users, not an attacker. Why is hiding credentials from the user a good idea?

[joepie91]

@joepie91 "you can either agree with what I say, or be wrong." I call that force.

I think I have enough crypto background to realize this is not the case.

I don't really care about how much "crypto background" you have. Designing secure systems is half technology and half psychology. You have failed to make a compelling argument on the former, and evidently aren't sufficiently familiar with the latter.

There is no reason for this to be used in this way - it doesn't provide any technical security, and it creates a false sense of security, which objectively leads to people making more dangerous decisions. That is how people work.

[kelunik]

1) It doesn't add real security 2) It ships with a default key 3) The key is not easily customizable

[Manouchehri]

@abiusx

1. What do you define as "real security?" If unreal security adds benefits, I'm happy with that.
2. We can fix that.
3. We can fix that too.

Great! Until 2 & 3 are done, this shouldn't be shipped as is. Hence my suggestion to revert to plaintext by default unless the default key is changed.

[joepie91]

@joepie91 If you want to provide criticism, either establish your point, or provide a better alternative. Just because you say "this is not good", does not make it so.

I've already provided the better alternative, as have others, several times by now. The better alternative is to not encrypt things in the first place, as there is no point in doing so. "Do nothing" is a valid solution, and the correct one in this case, as far as I can tell.

I have already established my point. That you are unwilling to accept it, does not change that.

There is ample use-case scenarios in this thread for benefits of this.

I have not seen any. Can you reference them?

[kelunik]

If you fix 2 and 3, it's already a lot better.

[paragonie-scott]

This is my recommendation, as someone with a lot of experience breaking cryptography implementations in PHP land:

Remove the Encryption library, re-think your threat model.

Any further bickering is just going to make this thread even more embarrassing for OWASP, which is a shame because it's an organization that I like.

If anyone needs me, contact security@paragonie.com as I'm unsubscribing from this thread.

Good day.

[Manouchehri]

@abiusx We gain several CPU cycles by not encrypting strings. Encrypting with a default key only hurts performance and doesn't gain any security.

[kelunik]

You're an owner here. I don't have time for more open source than I already do currently.

[Manouchehri]

@abiusx Which is why we're suggesting to remove the "encryption" library. We understand it's an open source library and the issue will not be resolved instantly.

Even if one of us removes the offending functions, would you accept a PR? Based on this discussion, it seems like it wouldn't be accepted.

[paragonie-scott]

If you have arguments that do not rely on "your position and background", feel free to provide them.

I already have, you seem to have ignored them. Re-read the thread from the beginning.

1. The current best practice is to NOT store credentials in your source files (or leave them in version control in the first place!)
2. Blinding passing data to PHP's unserialize() function is a huge security risk.
3. Unauthenticated encryption is not secure

If you absolutely must obfuscate, use defuse/php-encryption, which has been well studied by PHP security experts, on the untracked configuration file with the sensitive information and _stop rolling your own crypto_.

[AndrewCarterUK]

@abiusx I do not intend to patch this repository or try and lead anyone into thinking that it is being actively maintained or undergoing security review.

I've posted those as evidence to support the argument that this repository needs to be deleted (which is what the primary focus of this thread now is).

If you'd like me to start a "We should delete this repository" thread I will happily do so.

[paragonie-scott]

Not storing credentials in source files? Then where? The previous solution you provided adds security risks, as I've explained before.

How does it add a security risk above what the current code does? You haven't demonstrated that at all.

• If a project has a LFD/LFI vulnerability, it's already swiss cheese.
• If it doesn't, storing a file in a non-public location doesn't increase your risk at all.