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

confidentialString function uses hard-coded key #108

Closed
asgrim opened this issue Nov 19, 2015 · 108 comments

Comments

Projects
None yet
@asgrim
Copy link

commented Nov 19, 2015

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? 😢

@abhshkdz

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

@rash805115

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

Hi @asgrim . The $key variable holds the key used to encrypt and decrypt, but the key given in the code is just a sample. Each developer while deployment will have their own key which will be secret to them. You might additionally ask why this variable is defined here and not in a config file, its because phpsec was intended to be individual plug and play libraries. Hence that single file should be self contained and ideally should not have any associations with other parts of code.
If you are also wondering why have a key at all, look into other frameworks such as laravel, codeingniter, etc., everyone usually has one config file which contains a single key in plain text to encrypt and decrypt. The idea is that since the servers are not exposed to public, confidential information such as keys can be kept in code.

Also, its been a long time since I contributed to this project, and to the best of my knowledge no one is currently actively participating in it. So I urge you to contribute if you feel you can make improvements.

Please close the issue if you have no more doubts.

@asgrim

This comment has been minimized.

Copy link
Author

commented Nov 20, 2015

@rash805115 the problem is, the "modern" way to use PHP libraries is to pull the library in using Composer etc. and configure things. People don't copy and paste classes from libraries into their code any more, and because that key is private, with no way of overriding the value, it is essentially unmodifiable.

I already noticed it has not been updated for a while after I sent my PR, and therein lies another issue: this repo does not contain best practice code, but still exists, and exists under a namespace (OWASP) that should be a trusted authority on best security practice. However this is not the case.

I strongly believe that this repo should be taken down, so that developers do not explore this repository and start doing things the wrong way.

On the upside: I am happy to start contributing to a new PHP security project, a library that CAN be used securely, and demonstrates (with documentation) best security practices, and recommendations, that can be kept up to date. If you'd like to discuss further to re-start this project, please get in touch with me.

@asgrim asgrim closed this Nov 20, 2015

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I too would offer my services to help on a new security project that did follow best security practices.

@maccath

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi @rash805115 , I have some questions based on what you have just said, then.

How, apart from editing the phpsec Encryption class manually, do you expect users of the class to set their own key? It is a private variable, and there is no method available to set it to anything other than what is defined in the file itself. I don't see why you can't have a configuration mechanism in a plug-and-play library, by the way, especially when you /expect/ people to set their own key.

You use Laravel as an example of having a plaintext key in configuration files; this is true - a plaintext key exists in the environment configuration. This key is stored on the server outside of the public realm, and is not defined by default. It essentially forces you to create this key yourself on a per-environment basis. This is much more secure than hard-coding a private key variable within an encryption class, which people may or may not realise they need to edit!

Although nobody has been contributing to this project, it is nevertheless sad to see that a project with a focus on securty is insecure itself, and with the project still publicly available, no doubt people are still using it. I feel that there is a duty to inform your users of the steps they should take in order to secure this code themselves, even if that is just by editing the class directly, as currently - and correct me if I am wrong - there is no documentation that explains that this must be done.

I also agree that this project should be taken down, if it is not to be updated.

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi Katy, Andre and Rahul

I think is important to mentioned this issue in the github, as you can see
indeed , the project is in incubator stage which means in an experimental
phase in 'owasp' terms

The project has not been updated for more than 10 months which is quite
dormant, if you take a look to owasp inventory of projects in the category
of 'incubators' it happens quite often that projects become dormant and are
not updated in a long time. The idea is to keep the inventory clean, as you
have discovered, this issues should be fixed however these are open source
projects driven by volunteer efforts, so no guarantees what so ever.

It is a rule however that if a project is not been active for more than a
year it will receive a 'low activity' label and after, it will be set as
inactive.

I encourage you to please log this issue in the github and I have copied
owasp project coordinator to set the project under the low activity label

Regards

Johanna

On Fri, Nov 20, 2015 at 5:54 AM, Katy Ereira notifications@github.com
wrote:

Hi @rash805115 https://github.com/rash805115 , I have some questions
based on what you have just said, then.

How, apart from editing the phpsec Encryption class manually, do you
expect users of the class to set their own key? It is a private variable,
and there is no method available to set it to anything other than what is
defined in the file itself. I don't see why you can't have a configuration
mechanism in a plug-and-play library, by the way, especially when you
/expect/ people to set their own key.

You use Laravel as an example of having a plaintext key in configuration
files; this is true - a plaintext key exists in the environment
configuration. This key is stored on the server outside of the public
realm, and is not defined by default. It essentially forces you to create
this key yourself on a per-environment basis. This is much more secure than
hard-coding a private key variable within an encryption class, which people
may or may not realise they need to edit!

Although nobody has been contributing to this project, it is nevertheless
sad to see that a project with a focus on securty is insecure itself, and
with the project still publicly available, no doubt people are still using
it. I feel that there is a duty to inform your users of the steps they
should take in order to secure this code themselves, even if that is just
by editing the class directly, as currently - and correct me if I am wrong

  • there is no documentation that explains that this must be done.

I also agree that this project should be taken down, if it is not to be
updated.


Reply to this email directly or view it on GitHub
#108 (comment).

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

Where is the insecurity in that?
I just explained that the purpose of that encryption class is to hide sensitive information, not encrypt critical data.

On Nov 20, 2015, at 4:54 AM, Katy Ereira notifications@github.com wrote:

Hi @rash805115 https://github.com/rash805115 , I have some questions based on what you have just said, then.

How, apart from editing the phpsec Encryption class manually, do you expect users of the class to set their own key? It is a private variable, and there is no method available to set it to anything other than what is defined in the file itself. I don't see why you can't have a configuration mechanism in a plug-and-play library, by the way, especially when you /expect/ people to set their own key.

You use Laravel as an example of having a plaintext key in configuration files; this is true - a plaintext key exists in the environment configuration. This key is stored on the server outside of the public realm, and is not defined by default. It essentially forces you to create this key yourself on a per-environment basis. This is much more secure than hard-coding a private key variable within an encryption class, which people may or may not realise they need to edit!

Although nobody has been contributing to this project, it is nevertheless sad to see that a project with a focus on securty is insecure itself, and with the project still publicly available, no doubt people are still using it. I feel that there is a duty to inform your users of the steps they should take in order to secure this code themselves, even if that is just by editing the class directly, as currently - and correct me if I am wrong - there is no documentation that explains that this must be done.

I also agree that this project should be taken down, if it is not to be updated.


Reply to this email directly or view it on GitHub #108 (comment).

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

We’d love to welcome you aboard.
The current PHP security project is outdated, by its main goals are to avoid bloat, and provide decoupled libraries, so that we don’t force developers to either integrate our entire platform or lose it all.

On Nov 20, 2015, at 4:09 AM, Andrew Carter notifications@github.com wrote:

I too would offer my services to help on a new security project that did follow best security practices.


Reply to this email directly or view it on GitHub #108 (comment).

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

James,
One point you are missing is that the intent of this function is to hide, not encrypt, sensitive information, so that when someone passes by your screen, or leaks one file of your code, they do not immediately recognize your password.
Passwords still need to be part of the code, but hiding them is slighly better than having them in plain sight. It just involves a programmatic step of dehiding them, which is easy for the system administrator but hard for the bare eye.

Do you still think that this does not server that purpose, even with a fixed, static key?

On Nov 20, 2015, at 3:50 AM, James Titcumb notifications@github.com wrote:

Closed #108 #108.


Reply to this email directly or view it on GitHub #108 (comment).

@asgrim

This comment has been minimized.

Copy link
Author

commented Nov 20, 2015

That intent is not obvious, nor clearly documented. What looks like, to an untrained eye, an encryption technique (with classes called Encryption for example), is actually nothing more a vague obfuscation, nothing more secure than base64 encoding in reality.

The issue here is that this library is publicly accessible, it is under the trusted OWASP namespace on GitHub, and thus is likely considered an authoritative source on how to do encryption properly. Someone may use that code directly in their application, and be unaware of the fact that the encryption key must be changed. The public API for that class is such that it cannot even be changed in a clean way, so already presents a burden for implementation.

The fact that the fixed, static key exists and is also publicly accessible means that someone could be using this code to actually encrypt strings, but is actually using a very overpowered obfuscation technique. I hope you see my point :)

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I see the point, but please read your comment again.
If we held everything to such standards, there would be no problems anywhere anytime.

On Nov 20, 2015, at 9:19 AM, James Titcumb notifications@github.com wrote:

That intent is not obvious, nor clearly documented. What looks like, to an untrained eye, an encryption technique (with classes called Encryption for example), is actually nothing more a vague obfuscation, nothing more secure than base64 encoding in reality.

The issue here is that this library is publicly accessible, it is under the trusted OWASP namespace on GitHub, and thus is likely considered an authoritative source on how to do encryption properly. Someone may use that code directly in their application, and be unaware of the fact that the encryption key must be changed. The public API for that class is such that it cannot even be changed in a clean way, so already presents a burden for implementation.

The fact that the fixed, static key exists and is also publicly accessible means that someone could be using this code to actually encrypt strings, but is actually using a very overpowered obfuscation technique. I hope you see my point :)


Reply to this email directly or view it on GitHub #108 (comment).

@asgrim

This comment has been minimized.

Copy link
Author

commented Nov 20, 2015

Actually, I hold the opinion that if anyone should have the highest of
standards, it should be OWASP, as they are making recommendations on
security. If this is the perspective of the whole OWASP organisation, then
I'm afraid I'm going to have to stop recommending people look up OWASP.

I may have missed the point (please, someone point out something I
missed?), but leaving a private encryption key publicly accessible on the
web, no matter what the intended use case, is not advocating secure coding.

This sort of attitude "if we used the highest standards, we'd have no
problems" is surely what OWASP should be advocating (i.e. be as secure as
possible), but the responses on here have led me to believe that is not
what OWASP wants...

As I said, maybe I've got the wrong end of the stick, but to me, this looks
really bad.

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I personally am in more favor of solutiosn that work, instead of solutions that require you to do 1000 things so that a rarely possible problem is avoided.

We’ve had many of those projects at OWASP, and they have all failed because nobody uses them.

I’m not in favor of 100% security at any price.

On Nov 20, 2015, at 9:39 AM, James Titcumb notifications@github.com wrote:

Actually, I hold the opinion that if anyone should have the highest of
standards, it should be OWASP, as they are making recommendations on
security. If this is the perspective of the whole OWASP organisation, then
I'm afraid I'm going to have to stop recommending people look up OWASP.

I may have missed the point (please, someone point out something I
missed?), but leaving a private encryption key publicly accessible on the
web, no matter what the intended use case, is not advocating secure coding.

This sort of attitude "if we used the highest standards, we'd have no
problems" is surely what OWASP should be advocating (i.e. be as secure as
possible), but the responses on here have led me to believe that is not
what OWASP wants...

As I said, maybe I've got the wrong end of the stick, but to me, this looks
really bad.

Reply to this email directly or view it on GitHub #108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

@abiusx

The class is called Encryption inside the crypto directory using AES and with variables named $encryptedString.

Please do not say that the point of this class is to "hide, not encrypt, sensitive information" - that is false. This class is an attempt at encryption and it is clearly advertised as that. Besides, if the purpose is hiding sensitive encryption then encryption is what it should be doing.

The encryption key and the IV should be passed as method parameters rather than hard coded into class properties - then the user can decide if they're setting them at run time or hard coding them. More importantly, the user knows that they need them.

All of this is actually distracting from the much larger issue - this whole repository is plagued with serious security flaws that have the potential to cause significant damage to anyone who trusts the OWASP name and uses the code. In my opinion the only option is to delete the whole thing as soon as possible and replace it with a warning to anyone who has ever used the code asking them to immediately review their applications security and replace any code that depended upon this library.

I honestly don't believe that this repository is fixable, but the exposure to risk of people trusting the OWASP namespace can be reduced if this is done.

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi James and Abbas

If this is the perspective of the whole OWASP organisation, then I'm
afraid I'm going to have to stop recommending people look up OWASP.

Please consider again that projects that are in incubator phase, are in an
experimental process. At this phase , the project is still in development
and is not ready for major stream use (please read the description what
means the incubator label)

https://www.owasp.org/index.php/OWASP_Project_Stages#tab=Incubator_Projects

As mentioned, owasp tries todo its best to promote security but users and
consumers of projects must also realise the stage of a project. Like
mentioned before , the project is an incubator and at this stage is still
in experimentation phase. Getting a Flasghip status is not simple at OWASP
and it is given to projects that have shown a track record.

if you can help provide the project a practical solution and contribute and
want to help us make improvements please, let us know

@Abbas, if you don't mind, I can update the wiki page and the Github with
the observation James mentioned
Again . to me you both show different points of views but I think is
important to mentioned how this key is implemented in the library

regards

Johanna

On Fri, Nov 20, 2015 at 10:41 AM, AbiusX notifications@github.com wrote:

I personally am in more favor of solutiosn that work, instead of solutions
that require you to do 1000 things so that a rarely possible problem is
avoided.

We’ve had many of those projects at OWASP, and they have all failed
because nobody uses them.

I’m not in favor of 100% security at any price.

On Nov 20, 2015, at 9:39 AM, James Titcumb notifications@github.com
wrote:

Actually, I hold the opinion that if anyone should have the highest of
standards, it should be OWASP, as they are making recommendations on
security. If this is the perspective of the whole OWASP organisation,
then
I'm afraid I'm going to have to stop recommending people look up OWASP.

I may have missed the point (please, someone point out something I
missed?), but leaving a private encryption key publicly accessible on the
web, no matter what the intended use case, is not advocating secure
coding.

This sort of attitude "if we used the highest standards, we'd have no
problems" is surely what OWASP should be advocating (i.e. be as secure
as
possible), but the responses on here have led me to believe that is not
what OWASP wants...

As I said, maybe I've got the wrong end of the stick, but to me, this
looks
really bad.

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


Reply to this email directly or view it on GitHub
#108 (comment).

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I agree. The class should be renamed. Its initial purpose was not to do encryption. It was to hide sensitive information.
Also it shouldn’t have hardcoded keys, but a default key would do no harm.

As for the points mentioned by James and Andrew, they are already discussed in this issue.

If they want to help, I’d really appreciate it.
I also asked him specifically to provide me with a few sentences to add to the related wiki pages.

On Nov 20, 2015, at 9:51 AM, owaspjocur notifications@github.com wrote:

Hi James and Abbas

If this is the perspective of the whole OWASP organisation, then I'm
afraid I'm going to have to stop recommending people look up OWASP.

Please consider again that projects that are in incubator phase, are in an
experimental process. At this phase , the project is still in development
and is not ready for major stream use (please read the description what
means the incubator label)

https://www.owasp.org/index.php/OWASP_Project_Stages#tab=Incubator_Projects

As mentioned, owasp tries todo its best to promote security but users and
consumers of projects must also realise the stage of a project. Like
mentioned before , the project is an incubator and at this stage is still
in experimentation phase. Getting a Flasghip status is not simple at OWASP
and it is given to projects that have shown a track record.

if you can help provide the project a practical solution and contribute and
want to help us make improvements please, let us know

@Abbas, if you don't mind, I can update the wiki page and the Github with
the observation James mentioned
Again . to me you both show different points of views but I think is
important to mentioned how this key is implemented in the library

regards

Johanna

On Fri, Nov 20, 2015 at 10:41 AM, AbiusX notifications@github.com wrote:

I personally am in more favor of solutiosn that work, instead of solutions
that require you to do 1000 things so that a rarely possible problem is
avoided.

We’ve had many of those projects at OWASP, and they have all failed
because nobody uses them.

I’m not in favor of 100% security at any price.

On Nov 20, 2015, at 9:39 AM, James Titcumb notifications@github.com
wrote:

Actually, I hold the opinion that if anyone should have the highest of
standards, it should be OWASP, as they are making recommendations on
security. If this is the perspective of the whole OWASP organisation,
then
I'm afraid I'm going to have to stop recommending people look up OWASP.

I may have missed the point (please, someone point out something I
missed?), but leaving a private encryption key publicly accessible on the
web, no matter what the intended use case, is not advocating secure
coding.

This sort of attitude "if we used the highest standards, we'd have no
problems" is surely what OWASP should be advocating (i.e. be as secure
as
possible), but the responses on here have led me to believe that is not
what OWASP wants...

As I said, maybe I've got the wrong end of the stick, but to me, this
looks
really bad.

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


Reply to this email directly or view it on GitHub
#108 (comment).


Reply to this email directly or view it on GitHub #108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

@owaspjocur

"Please consider again that projects that are in incubator phase, are in an
experimental process. At this phase , the project is still in development
and is not ready for major stream use (please read the description what
means the incubator label)"

This project hasn't had any significant development for months. The README doesn't mention how insecure this whole repository is and I discovered it because @abiusx recommended that the insecure CSPRNG was used in the PHP CSRF Guard on the OWASP website (also insecure).

"if you can help provide the project a practical solution and contribute and
want to help us make improvements please, let us know"

This repository needs to be emptied and replaced with a warning to anyone who ever used the code it previously contained. I'm happy to work on a replacement guide to using secure PHP packages that already exist in the wild, there are many well tested and used ones that do everything this tries to.

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I don’t see the point in emptying the repository.
A simple readme file will do the trick. Plus prior users can investigate the unsafeties, and new users can realize them.

Unfortunately I don’t have the time to make that change at the moment, and would appreciate your help if you’re willing.

On Nov 20, 2015, at 9:57 AM, Andrew Carter notifications@github.com wrote:

@owaspjocur https://github.com/owaspjocur
"Please consider again that projects that are in incubator phase, are in an
experimental process. At this phase , the project is still in development
and is not ready for major stream use (please read the description what
means the incubator label)"

This project hasn't had any significant development for months. The README doesn't mention how insecure this whole repository is and I discovered it because @abiusx https://github.com/abiusx recommended that the insecure CSPRNG was used in the PHP CSRF Guard on the OWASP website (also insecure).

"if you can help provide the project a practical solution and contribute and
want to help us make improvements please, let us know"

This repository needs to be emptied and replaced with a warning to anyone who ever used the code it previously contained. I'm happy to work on a replacement guide to using secure PHP packages that already exist in the wild, there are many well tested and used ones that do everything this tries to.


Reply to this email directly or view it on GitHub #108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

@abiusx - The point in emptying the repository is that it contains a plethora of insecure the code that could cause significant damage (financial, personal, etc...) to anyone mislead into trusting the OWASP namespace.

If you are seriously suggesting that we leave this up so that people can play roulette with the insecurities in the code then I withdraw my offer of working with an organisation that has this approach to security.

This repository isn't advertised as a learning tool plagued with the vulnerabilities that it is. It is advertised as a framework for creating secure web applications (which it isn't). It should be emptied.

@abiusx

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

I don’t think we’re having a constructive argument here.
Have a good day.

On Nov 20, 2015, at 10:02 AM, Andrew Carter notifications@github.com wrote:

@abiusx https://github.com/abiusx - The point in emptying the repository is that it contains a plethora of insecure the code that could cause significant damage (financial, personal, etc...) to anyone mislead into trusting the OWASP namespace.

If you are seriously suggesting that we leave this up so that people can play roulette with the insecurities in the code then I withdraw my offer of working with an organisation that has this approach to security.

This repository isn't advertised as a learning tool plagued with the vulnerabilities that it is. It is advertised as a framework for creating secure web applications (which it isn't). It should be emptied.


Reply to this email directly or view it on GitHub #108 (comment).

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi Andrew

Even though I understand your point of view , I can only say that many
people worked hard in this project. Although the results are not up to
expectations right now, does not mean that there were not significant
efforts to make the library a security library. I t is valid to consider a
README warning, but emptying the project is another story. I don't think
this is fair to say to the volunteers, especially after all the efforts
were put to create and build this project.
Like I mentioned, the project is at incubator stage and I understand that
this can be misleading or confusing for people not familiar with the OWASP
labels. I think maybe OWASP should considering making this labelling system
more clear to users so they know what they get at this stage. This project
maybe a security framework, but basically incubator means , it has no
guarantees at all

regards

Johanna

On Fri, Nov 20, 2015 at 11:02 AM, Andrew Carter notifications@github.com
wrote:

@abiusx https://github.com/abiusx - The point in emptying the
repository is that it contains a plethora of insecure the code that could
cause significant damage (financial, personal, etc...) to anyone mislead
into trusting the OWASP namespace.

If you are seriously suggesting that we leave this up so that people can
play roulette with the insecurities in the code then I withdraw my offer of
working with an organisation that has this approach to security.

This repository isn't advertised as a learning tool plagued with the
vulnerabilities that it is. It is advertised as a framework for creating
secure web applications (which it isn't). It should be emptied.


Reply to this email directly or view it on GitHub
#108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

@owaspjocur

None of the code in this repository is useful for anything other than an example of how not to do things. Saying that it is "in development" or "in incubator phase" is hiding the fact that the whole thing needs to be completely rewritten.

Could you confirm to me that you consider the feelings of your volunteers and contributors more important than the security of the applications developed by people trusting the OWASP namespace?

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Could you confirm to me that you consider the feelings of your volunteers
and contributors more important than the security of the applications
developed by people trusting the OWASP namespace?

Andrew, you are right to think so, I wanted to point out that the
intentions and efforts to make this a security library were there, but if
you consider that this has quite many security issues, could you please let
us know, apart from the Key encryption issue, what other issues did you
find that this library should be taken down?

On Fri, Nov 20, 2015 at 11:17 AM, Andrew Carter notifications@github.com
wrote:

@owaspjocur https://github.com/owaspjocur

None of the code in this repository is useful for anything other than an
example of how not to do things. Saying that it is "in development" or
"in incubator phase" is hiding the fact that the whole thing needs to be
completely rewritten
.

Could you confirm to me that you consider the feelings of your volunteers
and contributors more important than the security of the applications
developed by people trusting the OWASP namespace?


Reply to this email directly or view it on GitHub
#108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

https://github.com/OWASP/phpsec/blob/master/libs/core/random.php

        //If openssl is present, use that to generate random.
        if (function_exists("openssl_random_pseudo_bytes") && FALSE)
        {
            $random32bit=(int)(hexdec(bin2hex(openssl_random_pseudo_bytes(64))));
        }

The '&& FALSE' means that the CSPRNG openssl API is never used. Even if it was to be used the return value isn't checked (it can be false) and neither is the optional '$secure' output variable which flags whether the CSPRNG managed to securely generate a random value.

The intended fallback uses 'mt_rand' which should never be used for the purposes of cryptography. It also merges the seed values in a way that gives a ridiculously high number of collisions. Similar mistakes are made throughout the file.

https://github.com/OWASP/phpsec/blob/master/libs/auth/user.php#L71

This method converts the password to lower case before hashing it. This massively increases the opportunity for a brute force attack because 'PASSWORD' is the same as 'password'.

https://github.com/OWASP/phpsec/blob/master/libs/auth/user.php#L88

This method is vulnerable to timing attacks (should use hash_equals).

And all that is on top of the "encryption" library that this issue was originally about. I've literally discovered the last two of those in the 15 minutes since you posted your comment by looking in a single file.

On top of this, the whole library does not follow object oriented design principles that make it more maintainable (no interfaces for example) or modern coding standards that help it get deployed to projects (thank god).

I'm not exaggerating when I say this, I can genuinely see no other option than emptying the whole repository and replacing it with a warning to anyone who ever used this code.

@SvenRtbg

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

As one of the contributors to this repository, it's current state makes me sad.

You all are correct asserting that this particular library, in its current state, is a failure. Its goals, set before a Google "summer of code" event long ago, were to write for once a secure library that everyone should use. In the effort that followed, @rash805115 wrote code for basically everything that you'd need in a decent web framework: Form validation, controllers, database, logging, security stuff, session handling.

This probably has been a very educational effort, however I always found that this approach is flawed: There are already plenty of libraries around that do database abstraction or logging. IF they have security problems, the solution should be to fix them, not write another library that cannot compete with the other's features (no matter if it is more secure or not).

I have tried to find the one unique feature that no other library has, and there probably is one: Asserting secure passwords. There are several interesting checks I haven't seen anywhere else. This should be separated into one package, made to live up to expectations, and be released.

However, with this library being started at a time where Composer was in it's infancy, some things would have to change: I do find it annoying that there are so many violations of coding standards just because "Well yeah this is better for entry level developers I think".

Let's put an end to this project as it was intended at the start. Nobody did work on it since Aug 18, 2014. My merge from @philsturgeon 's pull request only added fixes to the TravisCI build tool.

So effectively this project is on hiatus for more than a year!

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi Sven, Andrew

Thank you for your feedback and I think there are enough valid points to
consider that the project is inactive right now and it should be set as an
inactive project.

As a defender security library project , there are definitely high risks
using it at this stage and that can be misinterpreted by potential users.
Considering owasp guidelines, the project in indeed inactive since it has
not been updated for that long.

I have asked Claudia, our project coordinator to set the project as an
inactive project and also set a label on the wiki similar to this
https://www.owasp.org/index.php/Category:OWASP_WebScarab_Project

With regards on the github content, since it falls under OWASP github,
there should be also a clear label that the project is inactive and
contains many security issues that makes the project not usable as a
security library

I will also contact the wiki editors group to make sure we have a label for
this kind of situation as the project wiki page must also be archived.

Best regards

Johanna
OWASP Volunteer

On Fri, Nov 20, 2015 at 12:17 PM, SvenRtbg notifications@github.com wrote:

As one of the contributors to this repository, it's current state makes me
sad.

You all are correct asserting that this particular library, in its current
state, is a failure. Its goals, set before a Google "summer of code" event
long ago, were to write for once a secure library that everyone should use.
In the effort that followed, @rash805115 https://github.com/rash805115
wrote code for basically everything that you'd need in a decent web
framework: Form validation, controllers, database, logging, security stuff,
session handling.

This probably has been a very educational effort, however I always found
that this approach is flawed: There are already plenty of libraries around
that do database abstraction or logging. IF they have security problems,
the solution should be to fix them, not write another library that cannot
compete with the other's features (no matter if it is more secure or not).

I have tried to find the one unique feature that no other library has, and
there probably is one: Asserting secure passwords. There are several
interesting checks I haven't seen anywhere else. This should be separated
into one package, made to live up to expectations, and be released.

However, with this library being started at a time where Composer was in
it's infancy, some things would have to change: I do find it annoying that
there are so many violations of coding standards just because "Well yeah
this is better for entry level developers I think".

Let's put an end to this project as it was intended at the start. Nobody
did work on it since Aug 18, 2014. My merge from @philsturgeon
https://github.com/philsturgeon 's pull request only added fixes to the
TravisCI build tool.

So effectively this project is on hiatus for more than a year!


Reply to this email directly or view it on GitHub
#108 (comment).

@AndrewCarterUK

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

Hi @owaspjocur

I think they are all wise moves. It does sound, however, like this code will remain public in the OWASP namespace (just with labels of being inactive). The project you linked has no label of inactivity on its GitHub page (https://github.com/OWASP/OWASP-WebScarab).

Unfortunately, not everybody reads README files and not everybody checks your wiki pages. Also, every code file in this repository has been indexed by Google.

How will you guarantee that nobody ever stumbles across this code and uses it in their project?

Regards,

Andrew

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Hi @owaspjocur https://github.com/owaspjocur

I think they are all wise moves. It does sound, however, like this code
will remain public in the OWASP namespace (just with labels of being
inactive). The project you linked has no label of inactivity on its GitHub
page (https://github.com/OWASP/OWASP-WebScarab).

Unfortunately, not everybody reads TODO files and not everybody checks your
wiki pages. Also, every code file in this repository has been indexed by
Google.

How will you guarantee that nobody ever stumbles across this code and uses
it in their project?

Regards,

Andrew

Andrew:

I have cc Claudia who can bring this discussion at higher level

If you have a former request to take down the library from OWASP Github
repository, please, make a former request to Claudia who can further take
this issue with the staff as they control the Github account

We have also recently discussed setting a higher level for accepting
defenders libraries as this has many risks associated of using insecure
libraries

Regards

Johanna

On Fri, Nov 20, 2015 at 12:51 PM, Andrew Carter notifications@github.com
wrote:

Hi @owaspjocur https://github.com/owaspjocur

I think they are all wise moves. It does sound, however, like this code
will remain public in the OWASP namespace (just with labels of being
inactive). The project you linked has no label of inactivity on its GitHub
page (https://github.com/OWASP/OWASP-WebScarab).

Unfortunately, not everybody reads TODO files and not everybody checks
your wiki pages. Also, every code file in this repository has been indexed
by Google.

How will you guarantee that nobody ever stumbles across this code and uses
it in their project?

Regards,

Andrew


Reply to this email directly or view it on GitHub
#108 (comment).

@owaspjocur

This comment has been minimized.

Copy link

commented Nov 20, 2015

Regarding Inactive projects in OWASP github, is wise to setup an inactive
label too I grew
https://github.com/OWASP/OWASP-WebScarab/blob/master/README

On Fri, Nov 20, 2015 at 12:56 PM, johanna curiel curiel <
johanna.curiel@owasp.org> wrote:

Hi @owaspjocur https://github.com/owaspjocur

I think they are all wise moves. It does sound, however, like this code
will remain public in the OWASP namespace (just with labels of being
inactive). The project you linked has no label of inactivity on its GitHub
page (https://github.com/OWASP/OWASP-WebScarab).

Unfortunately, not everybody reads TODO files and not everybody checks
your wiki pages. Also, every code file in this repository has been indexed
by Google.

How will you guarantee that nobody ever stumbles across this code and uses
it in their project?

Regards,

Andrew

Andrew:

I have cc Claudia who can bring this discussion at higher level

If you have a former request to take down the library from OWASP Github
repository, please, make a former request to Claudia who can further take
this issue with the staff as they control the Github account

We have also recently discussed setting a higher level for accepting
defenders libraries as this has many risks associated of using insecure
libraries

Regards

Johanna

On Fri, Nov 20, 2015 at 12:51 PM, Andrew Carter notifications@github.com
wrote:

Hi @owaspjocur https://github.com/owaspjocur

I think they are all wise moves. It does sound, however, like this code
will remain public in the OWASP namespace (just with labels of being
inactive). The project you linked has no label of inactivity on its GitHub
page (https://github.com/OWASP/OWASP-WebScarab).

Unfortunately, not everybody reads TODO files and not everybody checks
your wiki pages. Also, every code file in this repository has been indexed
by Google.

How will you guarantee that nobody ever stumbles across this code and
uses it in their project?

Regards,

Andrew


Reply to this email directly or view it on GitHub
#108 (comment).

@Manouchehri

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

@joepie91

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

@joepie91

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

@paragonie-scott

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

@Manouchehri

This comment has been minimized.

Copy link

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link

commented Nov 25, 2015

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.
@paragonie-scott

This comment has been minimized.

Copy link

commented Nov 25, 2015

I don't agree that all vulnerabilities are equal.

Says the person defending a library that, if used to encrypt/decrypt data supplied by a user, is vulnerable to PHP object injection and possible memory corruption on older versions of PHP.

Turning an LFI into and LFD is not a security improvement.

Having a LFI in the first place is the problem. Worried about a +0.001% increase to an attack's impact, but greatly increasing the odds of the attack succeeding in the first place, is not a laudable goal to me.

Please explain to me how it could matter at all where you store the file.

https://paragonie.com/blog/2015/10/how-securely-allow-users-upload-files

@jonathan-daniel

This comment has been minimized.

Copy link

commented Nov 25, 2015

I don't think this discussion has any benefit.
The owners expect random people to save their horrible code and re-invent the wheel at the same time.
The security vulnerabilities in this library have been cracked before computers even existed.

@paragonie-scott

This comment has been minimized.

Copy link

commented Nov 25, 2015

@abiusx Explain to me, in detail, how moving your passwords &c outside of your PHP script into a configuration file that is stored outside the web root (so you can't just guess the URL) increases your risk in the absence of an existing vulnerability that already destroyed the integrity of your application.

@paragonie-scott

This comment has been minimized.

Copy link

commented Nov 25, 2015

Easy:

  1. If it's checked into version control (say: a private Github repository) and Github gets hacked, your password is compromised.
  2. If you have a large team of developers with access to the source code and not the production server, and one of them gets malware on their laptop, the attacker gets the password to your database.

If it's only stored in the production environment, neither of these attacks will succeed.

@jonathan-daniel

This comment has been minimized.

Copy link

commented Nov 25, 2015

@abiusx I'm not blaming you personally. And I am random in the sense that I don't have any connection to this repo.
Let me just put the function comment here so we have the original use-case clear.

Function to encrypt the sensitive data on its first run. For rest of the run, this function decrypts the encrypted data for use.

@abiusx abiusx closed this Nov 25, 2015

@jonathan-daniel

This comment has been minimized.

Copy link

commented Nov 25, 2015

  1. Security flaws need to be fixed. While they are not fixed the code needs to be removed or at least suggested not to use it for the advertised purpose (encryption).
@kelunik

This comment has been minimized.

Copy link

commented Nov 25, 2015

+1 for class removal, not sure about whole repository, didn't look closer at the rest.

@mk-pmb

This comment has been minimized.

Copy link

commented Nov 3, 2016

Please excuse me for commenting late, I found this thread just today. I'll put in front those points that I think might still be worth a thought:

the problem is, the "modern" way to use PHP libraries is to pull the library in using Composer etc. and configure things.

Should be a non-problem because composer should be able to run a post-install script that replaces the key, isn't it?

In some projects I did, where low security keys were needed, I took some randomness from modification dates of files, and where possible, also auto-generated a secret keyfile next to my library.

Whatever method is used to configure keys: If users are required to configure a key, or have it be configured by the installer, the package should fail to work until they configured the key.

I think maybe OWASP should considering making this labelling system more clear to users

Yes. I'd even recommend a separate github namespace, like OWASP-incubator/phpsec.


… and then below here are some thoughts that aren't news but I still feel an urge to endorse:

The attempts to explain the security problems away, and to play ignorant of advice already given at that time, are just awful.

That intent is not obvious, […] with classes called Encryption

👍

If some day I woke up and discovered that for strange reasons one of my own projects was at such a bad state, I'd at least immediately rename the misleading stuff asap, and take a few minutes to clarify documentation, including source comments near the key.

I personally am in more favor of solutiosn that work,

I can't see how you'd have to choose exclusively. From this thread it looks like the code could work just as well with less confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.