8 ms·

Simply Secure PHP Cryptography

147 points by CiPHPerCoder 9y ago

mi100hael 9y ago
I'm frequently amazed by how much Scott has single-handedly accomplished in bringing modern security practices to PHP. Huge props!
- verandaguys_alt 9y ago
  While I don't want to downplay his role in mobilizing the community, it's hardly single-handed. Implementing ext/sodium was a group effort by dozens of developers, reviewers, and testers.
  - mi100hael 9y ago
    Very fair. I don't mean to minimize the efforts of others and the community. I should say as an outsider with a long-held aversion to anything PHP-related, Scott has single-handedly changed my view of the language to a more neutral position.
  - CiPHPerCoder 9y ago
    > it's hardly single-handed I can't emphasize this enough. Libsodium was Frank Denis's project, which was spawned by NaCl by cryptographers Dan Bernstein, Tanja Lange, and Peter Schwabe. The participants who voted on the RFC, for the most part, were involved in the technical discussions over the past two years since I first mentioned the notion of doing so (before the PHP 7.0 release). Similarly, there were 13 people who contributed to the libsodium-php repository (ext/libsodium in PECL). Every single one of them had to consent to relicensing the extension to easily get merged into PHP, and we all did. I often joke that I'm the worst C developer in all of infosec, but there's some truth to that when it comes to modifying the PHP core. My main role in the ext/sodium project was saying, "We should do this," and somehow getting people to listen.
TylerE 9y ago
The problem is all the shitty broken parts of PHP are still there, and (many, many) people will use them. The only secure PHP is no PHP.
- stephenr 9y ago
  Given how much has been deprecated/removed since 5.2, would you care to list some that remain?
  - smt88 9y ago
    Truthiness (e.g. 0 == false, '0' == true, 'false' == true, etc.) makes it easier to introduce bugs. There's a lot of discipline/linting still required in PHP, whereas other languages won't even compile if you do something risky. I wouldn't take that criticism too personally. People also argue that C/C++ can't ever be secure due to the ease of programmer mistakes.
    - CiPHPerCoder 9y ago
      > Truthiness (e.g. 0 == false, '0' == true, 'false' == true, etc.) makes it easier to introduce bugs. Use === instead; except for cryptography, where you want hash_equals(). > There's a lot of discipline/linting still required in PHP, whereas other languages won't even compile if you do something risky. And/or just use scalar type declarations: https://3v4l.org/SJKe1 https://3v4l.org/SJKe1
      - bpicolo 9y ago
        Scalar type declarations don't help detect usage of undefined variables, as an example. Phan is the only tool I've found that does, but it's a very heavyweight thing, given how necessary it is to prevent undefined variable exceptions.
        CiPHPerCoder 9y ago
        That's a separate concern from truthiness. And if you aren't using nullable types (a 7.1+ feature), passing NULL gives you a TypeError. :)
pasta 9y ago
1. Clean up old tutorials, forum posts, and blogs. This! I'm still seeing code snippets on StackOverflow that use hashing for password storage and queries without prepared statements. I think at the moment that is still a weak spot of PHP. Edit: my mistake. I was refering to weak hashes like md5 and sha1 or 'home brew' hashing.
- stephenr 9y ago
  > use hashing for password storage As opposed to...?
  - CiPHPerCoder 9y ago
    I assume they meant the use of a cryptographic hash function (which is wrong) instead of a password hash function (which, as the name implies, is suitable for password storage). For example, I see a lot of this construction in proprietary code: $passwordHash = md5(md5($password) . md5($salt)); if ($storedHash == $passwordHash) { // Authenticated successfully } I'm quick to find these offending snippets and migrate our clients to use password_hash() and password_verify().
    - duskwuff 9y ago
      Or, for that matter... SELECT * FROM user WHERE username = '$username' AND password_hash = MD5('$password')
      - chx 9y ago
        Surely not after all these years! Does this easy SQL injection still exist...?
        zaphar 9y ago
        If you only knew the horrors I've seen. This week no less...
        9y ago
petraeus 9y ago
Until then php composer.phar require defuse/php-encryption
theincredulousk 9y ago
If there is one thing I know about PHP, it's that you can count on PHP developers to use the language's capabilities correctly, especially when it comes to security practices.
- tyingq 9y ago
  Meh. Even smart people do dumb things with languages other than PHP. Any language with string interpolation can create sql injection vulnerabilities, for example. PHP's docs encourage the right pattern, as do Ruby's, but... https://github.com/gitlabhq/gitlabhq/issues/2464 https://github.com/gitlabhq/gitlabhq/issues/2464
  - peterwwillis 9y ago
    There are best practices for handling dynamite, too, but I wouldn't build a safe with it. Nobody should be writing code that queries databases using tainted strings. Perl has had a taint mode for almost 30 years. Ruby supposedly has a taint mode, but I'm not a Ruby dev so I don't know how it affects the vuln you mention.
    - dsl 9y ago
      3/4 of the Perl scripts I have audited have had bad input sanitation in front of open() or exec(). I'll take SQLi over RCE any day.
      - peterwwillis 9y ago
        ....so enable Taint mode and fix the errors? And why would you think your code is vulnerable to SQLi and not RCE, or any other user input-related exploit?
- eeZah7Ux 9y ago
  Was that a typo? Serious question.
  - 9y ago
orf 9y ago
I'm not entirely sure I believe that. I saw a Paragonie project that was posted a while back, supposedly a "secure by default" CMS. I looked at the code and saw it was an hard to audit mess that decided to implement everything itself. It's own ORM, own router, own MVC framework. I spent an hour and found numerous issues[1] in the code. I don't believe re-implementing the wheel like that is a 'modern security practice'. I asked why they did that instead of using robust, well tested and well supported libraries and did not get a satisfactory answer. While it's not the author's project (I think?) he is still part of the company, and I'd hope such a security-focused organization wouldn't have done something like that. 1. https://paragonie.com/project/airship https://paragonie.com/project/airship 2. https://news.ycombinator.com/item?id=13905055 https://news.ycombinator.com/item?id=13905055
- CiPHPerCoder 9y ago
  This is off topic and reads a bit like a personal attack, but if you want to have a level-headed discussion about Airship's design and implementation, https://github.com/paragonie/airship/issues https://github.com/paragonie/airship/issues A lot has changed since you last looked at it, and a lot will change before the v2.0.0 rewrite is complete.
  - orf 9y ago
    I'm sorry that you read it as a personal attack, I did not mean for it to read that way. I was merely pointing out something that I consider to be designed contrary to security best practices in response to a comment about security best practices. I'm glad to hear things have changed in the code however, and wish you the best with the rewrite.
    - tptacek 9y ago
      You replied to a comment praising Scott for improving PHP security by saying "I'm not sure I believe that", and then ambiguously attacked his code. Who are you trying to kid? I don't much care one way or the other about whether it's OK to try to take people down a peg, but I do care very much when people do that and then try to get away with pretending that's not what they're doing.
lisper 9y ago
Adding secure crypto to PHP is like putting a Schlage lock on a pup tent.
jedisct1 9y ago
If you can't wait until PHP 7.2 is available, version 2.0 of the standalone extension was released on PECL: http://pecl.php.net/package/libsodium http://pecl.php.net/package/libsodium It's identical to what was merged to PHP 7.2 (actually a bit better since some pull requests haven't been merged to php-src yet).
ivrrimum 9y ago
10/15

147 points by CiPHPerCoder 9y ago

I'm frequently amazed by how much Scott has single-handedly accomplished in bringing modern security practices to PHP. Huge props!

verandaguys_alt 9y ago
While I don't want to downplay his role in mobilizing the community, it's hardly single-handed. Implementing ext/sodium was a group effort by dozens of developers, reviewers, and testers.
- mi100hael 9y ago
  Very fair. I don't mean to minimize the efforts of others and the community. I should say as an outsider with a long-held aversion to anything PHP-related, Scott has single-handedly changed my view of the language to a more neutral position.
- CiPHPerCoder 9y ago
  > it's hardly single-handed I can't emphasize this enough. Libsodium was Frank Denis's project, which was spawned by NaCl by cryptographers Dan Bernstein, Tanja Lange, and Peter Schwabe. The participants who voted on the RFC, for the most part, were involved in the technical discussions over the past two years since I first mentioned the notion of doing so (before the PHP 7.0 release). Similarly, there were 13 people who contributed to the libsodium-php repository (ext/libsodium in PECL). Every single one of them had to consent to relicensing the extension to easily get merged into PHP, and we all did. I often joke that I'm the worst C developer in all of infosec, but there's some truth to that when it comes to modifying the PHP core. My main role in the ext/sodium project was saying, "We should do this," and somehow getting people to listen.

TylerE 9y ago

The problem is all the shitty broken parts of PHP are still there, and (many, many) people will use them. The only secure PHP is no PHP.

stephenr 9y ago
Given how much has been deprecated/removed since 5.2, would you care to list some that remain?
- smt88 9y ago
  Truthiness (e.g. 0 == false, '0' == true, 'false' == true, etc.) makes it easier to introduce bugs. There's a lot of discipline/linting still required in PHP, whereas other languages won't even compile if you do something risky. I wouldn't take that criticism too personally. People also argue that C/C++ can't ever be secure due to the ease of programmer mistakes.
  - CiPHPerCoder 9y ago
    > Truthiness (e.g. 0 == false, '0' == true, 'false' == true, etc.) makes it easier to introduce bugs. Use === instead; except for cryptography, where you want hash_equals(). > There's a lot of discipline/linting still required in PHP, whereas other languages won't even compile if you do something risky. And/or just use scalar type declarations: https://3v4l.org/SJKe1 https://3v4l.org/SJKe1
    - bpicolo 9y ago
      Scalar type declarations don't help detect usage of undefined variables, as an example. Phan is the only tool I've found that does, but it's a very heavyweight thing, given how necessary it is to prevent undefined variable exceptions.
      - CiPHPerCoder 9y ago
        That's a separate concern from truthiness. And if you aren't using nullable types (a 7.1+ feature), passing NULL gives you a TypeError. :)

pasta 9y ago

1. Clean up old tutorials, forum posts, and blogs. This! I'm still seeing code snippets on StackOverflow that use hashing for password storage and queries without prepared statements. I think at the moment that is still a weak spot of PHP. Edit: my mistake. I was refering to weak hashes like md5 and sha1 or 'home brew' hashing.

stephenr 9y ago
> use hashing for password storage As opposed to...?
- CiPHPerCoder 9y ago
  I assume they meant the use of a cryptographic hash function (which is wrong) instead of a password hash function (which, as the name implies, is suitable for password storage). For example, I see a lot of this construction in proprietary code: $passwordHash = md5(md5($password) . md5($salt)); if ($storedHash == $passwordHash) { // Authenticated successfully } I'm quick to find these offending snippets and migrate our clients to use password_hash() and password_verify().
  - duskwuff 9y ago
    Or, for that matter... SELECT * FROM user WHERE username = '$username' AND password_hash = MD5('$password')
    - chx 9y ago
      Surely not after all these years! Does this easy SQL injection still exist...?
      - zaphar 9y ago
        If you only knew the horrors I've seen. This week no less...
      - 9y ago

petraeus 9y ago

Until then php composer.phar require defuse/php-encryption

theincredulousk 9y ago

If there is one thing I know about PHP, it's that you can count on PHP developers to use the language's capabilities correctly, especially when it comes to security practices.

tyingq 9y ago
Meh. Even smart people do dumb things with languages other than PHP. Any language with string interpolation can create sql injection vulnerabilities, for example. PHP's docs encourage the right pattern, as do Ruby's, but... https://github.com/gitlabhq/gitlabhq/issues/2464 https://github.com/gitlabhq/gitlabhq/issues/2464
- peterwwillis 9y ago
  There are best practices for handling dynamite, too, but I wouldn't build a safe with it. Nobody should be writing code that queries databases using tainted strings. Perl has had a taint mode for almost 30 years. Ruby supposedly has a taint mode, but I'm not a Ruby dev so I don't know how it affects the vuln you mention.
  - dsl 9y ago
    3/4 of the Perl scripts I have audited have had bad input sanitation in front of open() or exec(). I'll take SQLi over RCE any day.
    - peterwwillis 9y ago
      ....so enable Taint mode and fix the errors? And why would you think your code is vulnerable to SQLi and not RCE, or any other user input-related exploit?
eeZah7Ux 9y ago
Was that a typo? Serious question.
- 9y ago

orf 9y ago

I'm not entirely sure I believe that. I saw a Paragonie project that was posted a while back, supposedly a "secure by default" CMS. I looked at the code and saw it was an hard to audit mess that decided to implement everything itself. It's own ORM, own router, own MVC framework. I spent an hour and found numerous issues[1] in the code. I don't believe re-implementing the wheel like that is a 'modern security practice'. I asked why they did that instead of using robust, well tested and well supported libraries and did not get a satisfactory answer. While it's not the author's project (I think?) he is still part of the company, and I'd hope such a security-focused organization wouldn't have done something like that. 1. https://paragonie.com/project/airship https://paragonie.com/project/airship 2. https://news.ycombinator.com/item?id=13905055 https://news.ycombinator.com/item?id=13905055

CiPHPerCoder 9y ago
This is off topic and reads a bit like a personal attack, but if you want to have a level-headed discussion about Airship's design and implementation, https://github.com/paragonie/airship/issues https://github.com/paragonie/airship/issues A lot has changed since you last looked at it, and a lot will change before the v2.0.0 rewrite is complete.
- orf 9y ago
  I'm sorry that you read it as a personal attack, I did not mean for it to read that way. I was merely pointing out something that I consider to be designed contrary to security best practices in response to a comment about security best practices. I'm glad to hear things have changed in the code however, and wish you the best with the rewrite.
  - tptacek 9y ago
    You replied to a comment praising Scott for improving PHP security by saying "I'm not sure I believe that", and then ambiguously attacked his code. Who are you trying to kid? I don't much care one way or the other about whether it's OK to try to take people down a peg, but I do care very much when people do that and then try to get away with pretending that's not what they're doing.

lisper 9y ago

Adding secure crypto to PHP is like putting a Schlage lock on a pup tent.

jedisct1 9y ago

If you can't wait until PHP 7.2 is available, version 2.0 of the standalone extension was released on PECL: http://pecl.php.net/package/libsodium http://pecl.php.net/package/libsodium It's identical to what was merged to PHP 7.2 (actually a bit better since some pull requests haven't been merged to php-src yet).

ivrrimum 9y ago

10/15