Changes coming to improve CC security

Posts: 6997
Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Alrighty... we're in the release candidate phase and really eager to get out, but I was encouraged by Barry Jaspan of Acquia to really bone up our default credit card security settings. My action also follows on the heels of TR's (I believe it was him) concerns about the strength of the built-in encryption routine and several other conversations about PCI compliance that never went anywhere.

While I have introduced new and potentially disruptive features, I feel it was a necessary "bug fix" to bring in prior to the RC 5. I have no further plans to modify code after this work gets committed, but I wanted to put the ideas out in the open and call for reviewers once it gets committed to Bazaar.

So, the basic problem was it took some complicated setup for stores to be PCI compliant before. This was due to the nature of Drupal's Forms API... particularly in that it was working against the ability to pass CC details between the checkout form and the review page without storing the information somehow. Further, the default settings were weak in terms of security... encryption wasn't required, for example. But that doesn't matter, because in order to be PCI compliant, you're not supposed to sensitive authentication data after CC authorization anyways. Folks not using the CVV field before obviously wouldn't have any trouble... but there's no way to keep someone from doing something inappropriate.

Basically, when the code jester of Acquia takes the time to let you know you have a problem, it's worth digging a little deeper to find a solution.

And a solution I found. It's as close as I can get in Drupal:

  1. On the checkout page, a customer enters their CC details.
  2. When the checkout form gets processed, this data will be encrypted and held in the session while we redirect to the review page.
  3. The review page will then immediately destroy that temporary data and put the encrypted info in a hidden field, thus bypassing the database. Furthermore, that data can only be decrypted server side with a private decryption key, so it's pretty darn safe. Combine that with SSL and maybe an armed guard posted to your web server and you're about as secure as you can possibly get.
  4. Post checkout, we'll store the last 4 digits of the CC number to the order's data array.

What does this mean for the future?

  • Well, there is still a debug mode that you can use for testing that operates pretty similar to the previous version. I'm keeping the uc_payment_credit table in for now so we don't hose folks who have stored the last 4 digits of past CC numbers in their credit tables, but I'll remove this table as soon as we port to D6.
  • Encryption will be required before you can use the credit module, even in testing. Server side encryption is necessary so we can confidently place the CC details in the form as a hidden variable.
  • We can leave the encryption routine as is, because the encrypted data isn't being stored in the database as it was before. It's instead kept in the checkout form.
  • No more need for a cron run to wipe CC details for abandoned orders.
  • I'll be writing a tutorial on making sure you're handling CC data securely.
  • Full PCI compliance by default. Sleep soundly.

Please let me know if you have any questions or concerns about the new method itself (though not about the timing of the update). I think it will work out great, and it's in testing right now on the Livetest.

This took quite a bit of tweaking in hook_form_alter() to come up with, but I'm happy with where it's going. It led to a couple code improvements in the cart and payment modules as well.

Posts: 1730
Joined: 08/14/2007
Bug FinderEarly adopter... addicted to alphas.Getting busy with the Ubercode.

Awesome news! I'm a little taken aback by requiring encryption, but at the same time it makes sense. Our last shop we used (not an Open Source one) didn't even give the option; things were just encrypted by default.

Looking forward to having a go with it, even if it means taking the site down for a few moments while I enable and configure the encryption scheme.

Nice work, Ryan - as always.

--

Wanna help send me to DrupalCon Paris? Or do you just like my work? Donate via PayPal!

Posts: 6997
Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Thanks, tP. I'm almost there... and it's working out better than expected! I just reviewed the PCI security standards and found out that it's only against their rules to not store sensitive authentication data subsequent to authentication. In all honesty, this means that it's possible to be totally PCI compliant right now... and of course we'll be stellar once the changes come into core!

Posts: 102
Joined: 08/08/2007
Bug FinderGetting busy with the Ubercode.Not Kulvik

*sigh*

while I agree that this is a great feature, this is something that should be done in 2.0 ubercart. This is getting very frustrating for those wanting an official stable release of ubercart, and with new code, this should be considered beta not RC software.

but as tP said, good feature to have, being PCI compliant is definitely a good thing to have!

Posts: 6997
Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

While understanding your sighs regarding more new code in an RC, I have to say that protecting CC data isn't something that should wait for 2.0 which may be 6 months away. If the issue had been pointed out to me after the 1.0 and I came up with this solution, I would've implemented it as a 1.1. While it was not a technical bug (code not working) it was a serious functional bug (difficult to the point that most sites probably weren't PCI compliant). I just can't let that slide for months... wish I'd come up with the solution sooner. Smiling

Also, in all honesty, this change involves taking more code out than it does putting code in, and it will make CC administration easier which has been a pain in our backside for some time now.

Posts: 6997
Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Alrighty, folks... I've committed the changes, and Lyle and I have performed various local tests. Please check out this thread to learn about helping test the new features. Now is the time to weed out bugs so RC 5 can be the last release candidate.

Thanks!

Posts: 95
Joined: 08/09/2007
Uber DonorBug Finder

Why must the debug information be stored in the uc_order table instead of the uc_payment_credit table?

--

Biodiesel * (ubercart + drupal) = Sundays Energy

Posts: 6997
Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Well, the uc_payment_credit table is deprecated. I'll be removing it altogether when we move on to 2.0. The main reason is that there's no good reason to ever query against a CC table except to load data up with an order. Furthermore, I was having to encrypt, store, and decrypt the various fields separately. Since the order object has a data array that automatically gets serialized/saved and loaded/unserialized with the order, it just made sense to move the data. It wouldn't be feasible to try and handle saving/loading differently for normal and debug modes.

Posts: 19
Joined: 10/24/2007
Ryan wrote:
  • I'll be writing a tutorial on making sure you're handling CC data securely.
  • I'd be really interested in reading that. Is it available?

    Posts: 6997
    Joined: 08/07/2007
    AdministratorHead Code Monkey - I eat bugs.

    Start here and be sure to read the linked page that talks about setting up your credit card settings:

    http://www.ubercart.org/docs/user/7104/accepting_credit_card_payments