Ruining the Magic of Magento's Encryption Library | Hacker News Reader

Ruining the Magic of Magento's Encryption Library

openwall.com

132 pointsbased250 comments

Look, I don't care if you use PHP. But if you aren't an expert, and your code hasn't been audited by experts, don't write your own crypto. Use libsodium, or libressl's libcrypto (if you think openssl is a good bet, you really need to see the talk that discusses the reasons for that fork), or libgcrypt. But for crying out loud, use a library that is actively used and vetted by people who KNOW WHAT THEY'RE TALKING ABOUT. And do your best to know what you're talking about, too.

No! Libre/Open/BoringSSL's libcrypto exposes the same low-level primitives as PHP's terrible crypto bindings do. Please, please don't build things directly with libcrypto.

Ah. Ouch. Sorry.

And that - in an ironic nutshell - is the problem with crypto: had you been tasked with implementing the crypto, you would have (unknowingly) done it wrong and called it a day. Someone else would be berating you to "leave it to the experts"

I was going to write a comment pointing that out. But I couldn't figure out how to write it.

Go ahead, laugh. It means you'll be hesitant to make that kind of mistake, which is the main thing.

I did laugh - but not at you. As I said - the problem is with crypto (not you). Crypto gives the appearance of being simple, straightforward and immutable - but its not. A lot of things can go wrong with your PRNG, or the key exchange, or the cipher, or a combination of any/all of the above. Oh, by the way a novel attack was discovered last week; it's the full package, complete with a mildly ominous ALLCAPS name, a fancy logo and a bloody domain - the state-of-the-art has changed. Again.

I don't know if the 'crypto problem' can be solved for all F/OSS projects that need it - there might not be enough experts to go around.

Almost the entire Magento code base qualifies for an example of code horror.

I'm not going to say don't write crypto. Instead I will give more useful advice: if you do write crypto, make it boring. Really boring. Do not invent anything. Do not be creative. Use an established modern construction with modern ciphers and use them in only one way. That one way should be the way cryptographers recommend with no deviations.

In a previous discussion: https://news.ycombinator.com/item?id=10345965

The crypto in Magento is extremely boring. In fact: it's too boring. It predates misuse-resistant interfaces like Nacl. It's not enough to be boring: you have to be the right amount of boring.

I was joking 10 years ago when I wrote "if you're typing A-E-S into your code you're doing it wrong", but more and more I feel like I was on to something.

What would be a good, boring, way to do encryption with PHP in 2016?

https://github.com/jedisct1/libsodium-php

https://paragonie.com/book/pecl-libsodium

Docs happen to be written by the OP too...

FWIW if I read correctly our own CiPHPerCoder in this thread is Scott Arciszewski from paragonie.

I often find his posts interesting and sometimes a welcome adjustment to some long-running counterproductive memes.

Yes, I am Scott. And thank you. :)

Use an AEAD. Learn that PHP doesn't support an AEAD. Wait for PHP 7.1... I recently covered this a bit when I looked at owncloud's crypto: https://blog.hboeck.de/archives/880-Pwncloud-bad-crypto-in-t...

The situation is far from ideal. Recommending libsodium-php is also not as simple, as this requires an additional thing to be installed on a server, which probably isn't the case for most shared hosting environments.

What does the comment about not using authentication mean? What would be authenticated in this case?

I think I'm in a bit over my head looking at the code, but does the author mean that somehow the data should be verified to come from a trusted source first? But doesn't the attacker have to have some of the encrypted data first for that to be meaningful?

Authentication in this context means baking in a way to verify that what you receive is something you encypted and has not been altered. Or, in other words, it lets you know that the data hasn't been tampered with.

As a practical example, imagine using encrypted cookies for a session store (like Rails can do), but imagine that they're not authenticated. It's possible that with clever manipulation of the encrypted cookie, I can alter the cookie such that it still decrypts into the structure ecorcyed, but with different data. For example, maybe I change the initialization vector a bit in a clever way and that makes the userId stored in the cookie change.

For a more detailed explanation and hypotherical, I thought [1] was a great article. It uses PHP for the examples, but it's mostly the theory that's of interest.

Of note, things are actually worse in Magento's case than this hypotherical since you can, as an attacker, get Magneto to use some pretty bad crypto when decrypting your message even if they used something decent when encrypting their payload (remember: since it's unauthenticated, what they give you and what you give back are not necessarily the same).

[1] https://paragonie.com/blog/2015/05/using-encryption-and-auth...

thanks for this. If I may sum up and you could verify this:

When I encrypt something and give it to you, I should take a signature (hash of the encrypted msg) and append it to the encrypted message. Then if you twiddle bits before asking me to decrypt it, I'll see that your provided signature is incorrect.

I know there's more in it, and I've briefly read through the article you provided, which I may look at it in more detail later, but I think that's the gist of it.

Exactly! In fact, that's embarrassingly concise given my rambling. The only major thing missed is that the hash input has to contain something that the attack doesn't know in addition to the message. Or in other words, the private key is usually also involved in the hashing. If you ever want to get into details, the MAC [1] and HMAC [2] wiki pages go into depth.

[1] https://en.wikipedia.org/wiki/Message_authentication_code [2] https://en.wikipedia.org/wiki/Hash-based_message_authenticat...

thanks for the followup :) and especially for the clarification about private keys (and MAC, HMAC) for the signature.

Aside from the obvious deficiencies, who the heck writes code like "if ($false === initVector)"? Is this a side effect of people who speak other languages with different grammatical structures? Very odd to read through code written "backwards" like that.

It's a code pattern from C. It guards against accidentally using = instead of == or ===.

I believe it's a once-popular way to avoid this mistake:

    if (variable = false) // == intended

Which is valid in C and some other languages. But

    if (false = variable)

Is not valid.

That makes way more sense than I would have thought. Thanks. Guess I've been away from C for a while.

https://en.wikipedia.org/wiki/Yoda_conditions

Yoda code, Wordpress introduced me to the phrase.

Magento is incompetent on much simpler levels than this.

1. Someone thought chmod 777 was a good idea, ever, under any circumstances. Not only is this standard practice in Magento installs (it's a how-to step in many books on Magento), it's all through the actual codebase.

The below is from the Magento Enterprise 1.14.0.1 tarball, downloaded from the company (and I double-checked this after someone questioned this last time I brought this up):

  $ grep -r chmod .|grep 777
  ./downloader/lib/Mage/Backup/Filesystem.php: chmod($backupsDir, 0777);
  ./app/code/core/Mage/Compiler/Model/Process.php: @chmod($dir, 0777);
  ./app/code/core/Mage/Install/Model/Installer/Console.php: @chmod('var/cache', 0777);
  ./app/code/core/Mage/Install/Model/Installer/Console.php: @chmod('var/session', 0777);
  ./app/code/core/Mage/Install/Model/Installer/Config.php: chmod($this->_localConfigFile, 0777);
  ./app/code/core/Mage/Catalog/Model/Product/Attribute/Backend/Media.php: $ioAdapter->chmod($this->_getConfig()->getTmpMediaPath($fileName), 0777);
  ./app/Mage.php: chmod($logDir, 0777);
  ./app/Mage.php: chmod($logFile, 0777);
  ./lib/Zend/Service/WindowsAzure/CommandLine/PackageScaffolder/PackageScaffolderAbstract.php: @chmod($path, '0777');
  ./lib/Zend/Service/WindowsAzure/CommandLine/PackageScaffolder/PackageScaffolderAbstract.php: @chmod($path, 0777);
  ./lib/Zend/Service/WindowsAzure/CommandLine/PackageScaffolder/PackageScaffolderAbstract.php: @chmod($path, 0777);
  ./lib/Zend/Cloud/StorageService/Adapter/FileSystem.php: chmod($path, 0777);
  ./lib/Varien/Autoload.php: @chmod($this->_collectPath, 0777);
  ./lib/Varien/File/Uploader.php: chmod($destinationFile, 0777);
  ./lib/Mage/Backup/Filesystem.php: chmod($backupsDir, 0777);
  ./errors/processor.php: @chmod($this->_reportFile, 0777);

2. The company thinks there's nothing wrong with storing money as floats: https://github.com/magento/magento2/issues/555

The way we eventually dealt with hosting Magento (which we had strongly advised against) was a concrete sarcophagus and a thirty-kilometre exclusion zone:

* a cron line specifically to remove o-w permissions from all files in the webroot every minute (which is very inelegant, but the alternative is maintaining our own patches to core).

* Files not owned www-data, except where Magento must be able to write to them.

* deploy all webroot files as a user the webserver can't write.

* cron.sh (Magento's internal cron) runs as root out of the box. We ran it as www-data.

* AppArmor to keep Magento from ever, ever being able to pull shit. This caught Magento's more antisocial tendencies on more than one occasion.

* Admin login: use a path other than "/admin" to foil quite a lot of attack bots at the very simplest level.

We have outsourced our remaining Magento, thankfully, and I don't personally have to maintain the above any more. (You know you've been administering Magento a bit long when you can hum along to bits of "Metal Machine Music" accurately.)

The use case for Magento is (apparently deliberately) confused. It's an unholy melange of a CMS and a shopping basket. There is no good out-of-the-box experience; in practice it's a job creation scheme for consultants.

Even crap-tier "well technically I can tell my boss's boss we have paid support" support, with a four-day response time for them to ask you a simple question you already put the answer to in the original ticket, is swingeingly expensive. I can't say what we're paying for this standard of quality, but I can say that it's public knowledge that Magento is at least $13k/yr: http://web.archive.org/web/20120215011525/http://www.magento...

The problem Magento seems to solve is when the business wants a quick site without developer involvement. After a few other abortive platforms (Plone, Drupal - which are both fine for what they are, in ways Magento just isn't, but didn't end up matching our needs), our eventual solution to this was Wordpress, which we have outsourced so I don't have to think about that either. Outsourced Wordpress with securing it being the host's problem is totally the right answer.

I don't have a good answer on the shopping basket, but Magento was bad enough at that too that we went back to our in-house homerolled system.

I understand some work has gone into Magento 2.0 to make it less mind-bogglingly horrible.

> The use case for Magento is (apparently deliberately) confused. It's an unholy melange of a CMS and a shopping basket. There is no good out-of-the-box experience; in practice it's a job creation scheme for consultants.

The CMS capabilities of Magento are quite limited, as the Community Edition does not even include a "menu" module or a hierarchy for your static pages. My opinion is similar to yours, it is a powerful shopping basket that will require lots of custom development or third-party module juggling to get a decent result.

I have been working with Magento 1.x CE for a few years now, and I wanted to add a few more factoids from a developer perspective:

* The database API has an "you want it, you got it!" mentality - if you tell it to add a unique index and MySQL refuses to because existing data is not unique, the API will parse the MySQL error message to identify the duplicated fields/values and delete them silently. You will get your index, but you might no longer have all your data. [0]

* The model classes store nearly all the in-memory data in an untyped dictionary. You can call $model->setFoobar('12345') and later $model->getFoobar() to get it back - all unknown method calls get forwarded to a "catch-all" method which treats the method name as a key into this dictionary, and gets/sets a value of that key [1]. Most of the time, these calls are unannotated, so IDEs can't make any sense of them (see the "Undefined method" part in [2]). This unknown method forwarding is also a performance issue, so they make optimizations like [3] by inlining pieces of the catch-all handler code for commonly-used data.

* The JavaScript is based on Prototype.js, not jQuery, so half the modules and themes bundle their own copies and drop them in different locations. Some of them provide a configuration setting so you can disable their jQuery and use your own, some don't.

* It includes a "Developer Mode" where php notices and warnings are turned into exceptions like in sane languages. But plenty of modules and themes have never been tested with this setting on and they cause faults on every page load - either you have to disable the Developer Mode, or fix this third-party code.

[0]: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/...

[1]: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/...

[2]: https://imgur.com/RMxWEgR

[3]: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/...

> I don't have a good answer on the shopping basket, but Magento was bad enough at that too that we went back to our in-house homerolled system.

Our clients were originally in love with Magento because "we can just add all these existing modules and get all sorts of awesome functionality without having to pay you for it!" We warned them that that was a pipe dream, but they didn't listen. Now they have several thousands of hours worth of custom development on top of Magento 1.x (95% of it written solely by me, no bus factor problems with that) and it's not even finished yet. Last I checked, the end-of-life for M1.x is planned in December 2018, and the client has no plan for what to do then.

> I understand some work has gone into Magento 2.0 to make it less mind-bogglingly horrible.

When I looked at Magento2, they had an amazing solution to PHP's lack of generics: they bundle multiple "template" classes and during system installation, you're supposed to run a CLI script that goes through those classes with a list of wanted concrete classes and generates specialized class files for all of them. If you fail to run this script, don't worry - their custom autoloader will detect this and generate those classes at runtime, when they are needed.

> The database API has an "you want it, you got it!" mentality - if you tell it to add a unique index and MySQL refuses to because existing data is not unique, the API will parse the MySQL error message to identify the duplicated fields/values and delete them silently. You will get your index, but you might no longer have all your data. [0]

Holy shit. Just, holy shit.

> Magento, one of the largest open source e-commerce platforms, ships a broken cryptography library that clueless developers are probably using to encrypt your credit card information for their client's customers.

Nope. There is a built in credit card method that would utilise the Magento crypt() but you cannot actually hook this up to a bank and get any actual money. The lamest of lamest Magento developers will use an off the shelf payment gateway that will use things like iframes so that even a non https Magento site will not have any credit card information pass through Magento, instead you get payment references.

In theory you could break the crypto and get into Magento admin, to then export out customer email and address details. You could probably refund all customers orders but not get fresh money out of them.

I appreciate the code may be 2008 vintage but there is no new vulnerability here that gives any means to access any Magento credit card data in a meaningful way, e.g. a lucrative way.

I think it's great that stuff like this is brought to the surface, but it also troubles me that the author makes no mention of submitting a PR to improve the _open source_ code base... They seem more content to discuss boycotting and abandonment instead.

Is the author obligated to fix the broken code? If they aren't willing to fix it, would you prefer they said nothing and left everyone using insecure crypto?

Definitely not. That's why I said it I think it's great that this stuff is brought to the surface. I can see how what I wrote isn't entirely clear though. What I am trying to say is that it would be nice if the message also aimed at encouraging contribution to open source software as well. It seems healthier to OSS as a whole to encourage improvement rather than abandonment.

Oh, yes, I agree with you. My sibling comment seems to suggest that Magento isn't actually OSS, so maybe that's the reason?

It definitely is open source insofar as it being a public repository that accepts contributions. However, the disclaimer at the top of the repo does seem to suggest that free offerings might be scooped up and put to other uses... I can see where the degree of "open sourceness" might be questioned in a case like this. I will re-iterate that I am willing to accept that there might be other politics at play here.

Open source has a specific definition, though. I think what Magento does is called "source available" or similar.

There is simply no way that realistically a third party contributor will rewrite a core part of a platform product for a company that generally does not accept such work, and has an enterprise version of the software that really gets all the goodies instead, which is where their focus is.

If it were true open source I think the project would get a lot more contributions. The enterprise edition, with the "best" features, is very much not free.

I am willing to accept that other variables might be involved here. I still think the general message of improving OSS is an important one.

This is absolutely true. They actually have a hostile attitude towards contributions. I used to work at a Magento shop (bad times all round) and my (extremely talented) colleague would often submit patches for long-standing bugs through the ticket tracker which would never appear in general release. Tickets would go for months without responses. It's commercial software at the end of the day. With an obscene price tag on it to scam cash out of non-technical business founders.

I didn't like my magento job much :/

This is why we have forks.

What fork of Magento actually gets usage?

None as far as I can tell. There's OpenMage, which accepts contributions. But the problem is that Magento Enterprise is shit, and the Community Edition is shit with the functional bits removed.

Magento is very much open-core in all practical terms.

It sure would be nice if the author did that, but crypto is a non-trivial amount of work to get right.

It's easy to look at one small part of a codebase and say "this is wrong", but to get it right you have to learn large chunks of the code. Be aware what level of work you are shoving onto the author in your comment.

I did aim to clarify things in one of my responses below. It wasn't my intent to suggest that the author should necessarily be the one making a PR. My main point is that contribution and improvement should be encouraged where this post leans more towards staying away from a piece of open source code altogether.

But keep in mind that people have plenty of time to have a look at a codebase, but often do not in any way shape or form have anywhere near enough time to actually work on that code, even to file what you might call "a simple PR". Spotting errors is a very quick job. Writing even a small PR on a codebase you do not work on, that properly passes tests (in this case crucial) as well as fuzzing (again, it's crypto, it's crucial) takes hours if not days. Sometimes, and this is one of those times, the only thing you're going to realistically get is feedback that you should act on ASAP, not code that helps you get started. And that's a critical contribution, too.

Like showing up one day with a PR saying "Hey guys, I rewrote some of your core functionality. Tests pass!" ...I'm not sure it would be that simple.

I'm not certain that one would necessarily need/want to take a another stab a "rewriting" core code here. If they are leaning on an interface, which it appears as such (EncryptorInterface), and have managed to avoid implicit coupling etc, it might be possible to swap out the implementation with something else. Maybe one of the other libs that have been recommended in this thread.

I'm the person who published this advisory to oss-sec and a co-author of https://github.com/defuse/php-encryption

I also wrote https://github.com/paragonie/halite which is a PHP library aims to make libsodium incredibly easy for PHP developers to get right.

For systems that need asymmetric crypto and can't install libsodium, I also wrote https://github.com/paragonie/EasyRSA which side-steps a lot of the mistakes developers make. EasyRSA uses defuse for symmetric-key encryption, then encrypts the AES key with RSA.

Writing secure cryptography isn't trivial. Assuming the three projects I just linked to are secure enough to use (all indications point to: they are), there's literally no reason to reinvent the wheel solely for Magento.

Magento already uses Composer; they could just add them to their composer.json file and rewrite their routines to use defuse + maybe EasyRSA if they have a use case for it.

Zend\Crypt is also an acceptable choice, as long as you don't install the version with an RSA implementation vulnerable to padding oracle attacks. https://framework.zend.com/security/advisory/ZF2015-10

Magento is a for profit platform owned by eBay. He's already donated them enough of his time. It is open sores, not open source, although it's easy to confuse the two!

Magento was sold from eBay to Permira late last year.

https://magento.com/company/press-room/press-releases/magent...

Reply on news.ycombinator.com