OWASP / phpsec

OWASP PHP Security Project - THIS PROJECT IS INACTIVE AND MAY CONTAIN SECURITY FLAWS
197 stars 103 forks source link

\phpsec\confidentialString() function is declared twice in the codebase. #33

Closed SvenRtbg closed 11 years ago

SvenRtbg commented 11 years ago

There are two occurrences of this function:

/libs/core/functions.php /libs/crypto/confidentialstring.php

abiusx commented 11 years ago

will be replaced with a proper class. Work is in progress, refactoring is much required.


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Mordad 4, 1392, at 5:21 AM, SvenRtbg notifications@github.com wrote:

There are two occurrences of this function:

/libs/core/functions.php /libs/crypto/confidentialstring.php

— Reply to this email directly or view it on GitHub.

SvenRtbg commented 11 years ago

I have an issue with this function all together. You are expecting my code to be deployed on the webserver writable! Otherwise this function will throw an exception if it cannot rewrite the code file. On the other hand, there is not very much gained from encrypting any strings if the password for decryption is also delivered - and it must be delivered, otherwise it won't work. What's the point of this function then?

There are certain sensitive values that simply have to be stored on the server. I think the best way would be to separate these as good from the code as possible to avoid having them end up in a public (or even private) repository, and allow these configuration values to be added as a local, per-server value that overrides the default application configuration. Encryption might help a bit, but in the end, a compromised machine will provide no protection against accessing the code and all other info that is stored on them, including any passwords for config value encryption.

abiusx commented 11 years ago

You're right about half of it. First, yes it needs to be writable. If its not, it throws an error and asks the developer to write that instead of doing it herself, so no real issue there. Second, it makes for a master key, which is much easier to keep secure than a buch of scattered sensitive information. Third, not all flaws provide full access to the server. Many of them just allow an attacker to access the database, or do some local file inclusions. If its a LFD, the separation of the master key helps greatly. The best idea we've got so far is to split the master key into separate locations, so that only if the attacker has full access they can read the information. If you've got anything better, please share.

Please join the mailing list for discussions. -A


Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Mordad 4, 1392, at 5:58 AM, SvenRtbg notifications@github.com wrote:

I have an issue with this function all together. You are expecting my code to be deployed on the webserver writable! Otherwise this function will throw an exception if it cannot rewrite the code file. On the other hand, there is not very much gained from encrypting any strings if the password for decryption is also delivered - and it must be delivered, otherwise it won't work. What's the point of this function then?

There are certain sensitive values that simply have to be stored on the server. I think the best way would be to separate these as good from the code as possible to avoid having them end up in a public (or even private) repository, and allow these configuration values to be added as a local, per-server value that overrides the default application configuration. Encryption might help a bit, but in the end, a compromised machine will provide no protection against accessing the code and all other info that is stored on them, including any passwords for config value encryption.

— Reply to this email directly or view it on GitHub.

rash805115 commented 11 years ago

I removed the function declared twice. I am not sure how I declared it two times, but I am certain that there are no more duplicate functions like this anymore.

rash805115 commented 11 years ago

Abbas, I cannot close the issue, can you please do this for me ?

SvenRtbg commented 11 years ago

Actually, the class "Encryption" is also declared twice in the same files. And it would be great if you could merge your changes to THIS repo instead of leaving it in your own. Most likely you need to create a pull request, which means you should first create a branch in your repo for the issue you are fixing, then commit the fixes, push the branch to github, then create the pull request.

After the pull request is created, there is the chance of reviewing the changes introduced and either give feedback in the code or pull the change.

rash805115 commented 11 years ago

The class and the function, both are taken care of. Thanks. :)