
Ruining the Magic of Magento's Encryption Library - based2
http://www.openwall.com/lists/oss-security/2016/07/19/3
======
qwertyuiop924
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.

~~~
tptacek
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.

~~~
qwertyuiop924
Ah. Ouch. Sorry.

~~~
sangnoir
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"

~~~
qwertyuiop924
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.

~~~
sangnoir
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.

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

------
api
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.

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

~~~
dguido
[https://github.com/jedisct1/libsodium-
php](https://github.com/jedisct1/libsodium-php)

[https://paragonie.com/book/pecl-libsodium](https://paragonie.com/book/pecl-
libsodium)

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

~~~
reitanqild
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.

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

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

~~~
chug
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...](https://paragonie.com/blog/2015/05/using-encryption-and-
authentication-correctly)

~~~
nchelluri
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.

~~~
chug
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](https://en.wikipedia.org/wiki/Message_authentication_code)
[2] [https://en.wikipedia.org/wiki/Hash-
based_message_authenticat...](https://en.wikipedia.org/wiki/Hash-
based_message_authentication_code)

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

------
spilk
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.

~~~
taspeotis
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.

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

------
davidgerard
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](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...](http://web.archive.org/web/20120215011525/http://www.magentocommerce.com/product/enterprise-
pricing-contact)

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.

~~~
DCoder
> _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/...](https://github.com/OpenMage/magento-
mirror/blob/magento-1.9/lib/Varien/Db/Adapter/Pdo/Mysql.php#L2739)

[1]: [https://github.com/OpenMage/magento-
mirror/blob/magento-1.9/...](https://github.com/OpenMage/magento-
mirror/blob/magento-1.9/lib/Varien/Object.php#L616)

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

[3]: [https://github.com/OpenMage/magento-
mirror/blob/magento-1.9/...](https://github.com/OpenMage/magento-
mirror/blob/magento-1.9/app/code/core/Mage/Catalog/Model/Product.php#L684)

> _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_.

~~~
meepmorp
> 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.

------
Theodores
> 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.

------
kdbuck
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.

~~~
toast42
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.

~~~
maaarghk
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 :/

~~~
njloof
This is why we have forks.

~~~
georgestephanis
What fork of Magento actually gets usage?

~~~
davidgerard
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.

