This is a copy of an old issue on ubercart.org: http://www.ubercart.org/issue/5598/special_characters_billing_info_custo...

I'm posting it here so it can be found in a search and so the issue can be tracked. Below is my summary of the issue - for more details read the above thread.

A fundamental flaws of the encryption scheme chosen by Ubercart is the fact that it is limited to only "all 95 printable characters from the ASCII character set EXCEPT single quote, double and backslash". In particular, it can't handle non-ASCII Latin characters, so ironically Ubercart can't even encrypt Übercart. And it certainly can't handle the entire Unicode set, even in UTF-8 form, so Hebrew or Arabic are not possible. The only real solution is to replace the flawed encryption scheme with a more robust and capable one.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave’s picture

Closed #912742: Checkout fails when credit card owner field contains special characters as duplicate.

I just realised it may be possible to use htmlentities() to encode special characters, which is a dirty workaround but might be enough.

longwave’s picture

longwave’s picture

Alternatively perhaps the mcrypt PHP extension should be used and made a dependency for uc_credit?

longwave’s picture

Also, as far as I have seen, no credit card gateways actually validate on this field, even if it is required to be sent for a transaction - presumably partially due to issues like this.

torgosPizza’s picture

mcrypt is somewhat standard in PHP installs these days, isn't it? I could be mistaken. But I wouldn't mind testing this, we could add it into a requirements hook for Ubercart.

longwave’s picture

I guess if you are taking credit card details on a site it is a reasonable expectation that you have a cryptography related module installed, or should be able to install it.

TR’s picture

I actually implemented this a long time ago on a Drupal 5 Ubercart installation. I used the OpenSSL encryption functions, as that is something that is already required for https, and https is already intimately connected with payment modules. So OpenSSL doesn't require any additional dependencies, however small.

I could revive this for D6 and D7 - it's a bit entangled with some other code. And we'd need to be able to deal with legacy data with a hook_update: keys would change, and any in-process transactions would get hosed.

hanoii’s picture

Just found this issue by following this same issue, special characters were also being used on on the cardholder name.

What about using htmlentities() as suggested in #1. The only issue with that is that we might miss some special characters not expected on the crypt function, but I am not really sure about that.

A fail-safe solution might be to base64 encode everything, we might use a bit more memory but that would leave no room for any errors on special characters.

What do you think? I think I will go for base64 for now, but this any solution seems like a very quick solution, and easy to implement.

hanoii’s picture

Priority: Normal » Major

And I rather increase the priority of this bug. I appreciate it has not been that much of an issue as special characters are not exactly common, but this is preventing a customer from making a purchase so it's something to look into rather quickly.

TR’s picture

I like the base64 option a lot better than htmlentities, but I'd really rather do a proper fix by throwing out the crappy encryption that's in there now - lack of support for the full Unicode character set is only a small part of what's wrong with it. I wish I had time to work on this myself ...

hanoii’s picture

And no chance to do the base64 in the meantime? I'd like to help with this but also not having the time to work on that right now. Don't mind rolling a patch for the base64 bit, I just tried it and it worked quite good.

hanoii’s picture

Status: Active » Needs review
FileSize
1.03 KB

For what it's worth, here is the patch.

EDIT: And adding again that although the best solution is not this one, we rather have quick fix and create a new encryption scheme issue for future discussions.

TR’s picture

@hanoii: I'm not disagreeing with you - I'd certainly like to get this fixed even if it's not the best way.

I think you're going to need a hook_update() to alter the cc information that's (temporarily) in the DB when the patch is applied, so that the cc information will remain usable. Otherwise, some orders in progress will get lost.

hanoii’s picture

You mean in the session database? I haven't thought of that, what about clearing all session, this might make orders to have to be replaced, but I am not sure whether that's a good compromise or not.

torgosPizza’s picture

I think destroying any active sessions is probably the way to go. I'd say, an update hook that does this as well as warning administrators that they might want to bring their site into offline mode, and warn their customers about impending cart data loss.

Certainly not a problem, especially if done during a low traffic period, since the trade-off is better customer experience.

longwave’s picture

Couldn't we just detect whether the data is in the old or new format and handle both cases on load, while always saving the new format?

Alternatively, using htmlentities() on save and html_entity_decode() on load for the cardholder name field looks like it would fix this issue while being 100% backward compatible - I think it's highly unlikely anyone has an actual entity string in their name.

hanoii’s picture

I am trying to think of some use case in which html entities won't work but I can't think of any. Are we sure any of the extended ascii set or any unicode characters will be an HTML entity?

Happy to roll back a patch to use that if we all agree to that.

If we were to stick with base64, one dirty way to check this is to check for the return of unserialize(), if it's false in can try to base64 decode it first, this would make it backward compatible, following @longwave suggestion.

TR’s picture

Status: Needs review » Needs work

htmlentities($arg1->payment_details, ENT_QUOTES, 'UTF-8') will work, but the output is horribly verbose compared to base64_encode(). So I still prefer base64_encode().

The "dirty" way of checking is OK with me. BUT. Please add code comments explaining why base64_encode()/base64_decode is being called, and add lots of comments on your unserialize() fail check, so that someone who's reading the code for the first time will understand what is being done and why.

hanoii’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Hope this is verbose enough.

hanoii’s picture

Sorry, just changed some small English corrections.

TR’s picture

Status: Needs review » Fixed

Committed #20 to 6.x-2.x and ported it and committed it to 7.x-3.x. I changed the comments a bit.

I would still like to throw out the old encryption algorithm and replace it, but this patch solves the immediate problem.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

longwave’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Closed (fixed) » Active

This is causing the following error in 7.x:
Notice: unserialize(): Error at offset 0 of 308 bytes in uc_credit_cache() (line 946 of uc_credit.module).
We could use @unserialize(), or perhaps we can use a better test: if the string contains a colon, it must be a plain unserialized string, otherwise it's a base64 encoded.

longwave’s picture

Status: Active » Needs review
FileSize
2.68 KB

This patch implements the improved base64 detection described above, and also adds three missing base64_encode() calls.

Haven't tested this in 6.x yet; an easy way to test is enable the card owner field and paste the Euro € symbol into it.

longwave’s picture

Status: Needs review » Fixed

Committed #24 to 7.x. Perhaps needs backporting, but no time to test right now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

hanoii’s picture

Version: 7.x-3.x-dev » 6.x-2.x-dev
Status: Closed (fixed) » Needs review
FileSize
2.67 KB

Well, I went over the patch and it does make sense to backport it.

Attached is the patch for 6.x dev

Just as a reference for me and for anything who needs it, because I haven't updated to 2.7 yet.. if you want to apply this fix to 2.6 you can apply this patch first: http://drupalcode.org/project/ubercart.git/patch/de6cd27bfb1caa925d84ce8...

and then the one attached here.

longwave’s picture

Status: Needs review » Fixed

Committed, thanks.

rickmanelius’s picture

FYI I'm having a similar issue appear

Notice: unserialize() [function.unserialize]: Error at offset 0 of 156 bytes in uc_credit_cache() (line 756 of /public_html/sites/all/modules/ubercart/payment/uc_credit/uc_credit.module).

Is this the same or a different issue?

rickmanelius’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Fixed » Needs work

PS. The error in #29 in ubercart 7.x-3.x-dev (Nov 8th). I wasn't getting the error last week before some module upgrades, so I feel the issue was recently introduced.

rickmanelius’s picture

I might want to also add that I had a major issue on the update.php process because the update was timing out during the update to remove certain debugging features from the credit card arrays. I'm don't know if it's related, but I just want to put that out there as a possibility as I have a test server running this version of ubercart with no problem.

longwave’s picture

What exactly happened during update.php? How many orders do you have in your database, and were all (or most) of them paid by credit card?

rickmanelius’s picture

4000 orders in the db, almost all of them by credit card.

I was simply getting timeout errors during the update because it was taking forever for the process. I then updated my php max time and it went through.

I'm looking at the data from $data = $crypt->decrypt($key, $data); right now to see if I can see the issue.

The update hook was uc_credit_update_7000. The while loop seemed to take forever and looking at it, I'm worried that it ran on the data multiple times.

rickmanelius’s picture

Status: Needs work » Closed (fixed)

Nevermind re:#29. I was using a different encryption key on my local copy and pulled down my production key and now that error has went away. So I apologize for opening this issue back up before a thorough analysis of all these possibilities.

For anyone else following this issue: you may have been using separate encryption keys in the past and now you must have it synced at all levels.

longwave’s picture

Priority: Major » Normal
Status: Closed (fixed) » Needs work

Thanks for posting your updates on this.

Reopening anyway, as we should convert update 7000 to a batch process so it never times out no matter how many orders.

rickmanelius’s picture

No problem. What triggered me onto this is my printable invoices are breaking (i.e. they are no longer using the templates anymore and look crappy). I thought it might be connected but no dice.

Thanks for your swift responses. And yeah, I did go through a mild bit of panic when this time out was occurring, so batch processing would be super nice!

Again, thanks for all your help/replies.

longwave’s picture

Status: Needs work » Closed (fixed)

Oops, actually I forgot uc_credit_update_7000() was introduced in #499782: Credit card debug mode should be removed so this really can be closed :)

wuh’s picture

I realise this issue has been fixed and closed, however, I just wanted to add a note. If anyone's running 6.x-2.x-dev and still has this issue - read this comment on ubercart.org: http://www.ubercart.org/forum/support/10895/protect_our_customers_identi...

I had the very same problem and removing an empty line in uc_credit.key resolved the problem. Hope this helps someone.