Closed (fixed)
Project:
Commerce Authorize.Net
Version:
7.x-1.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Reporter:
Created:
2 Mar 2013 at 08:04 UTC
Updated:
31 Jan 2018 at 17:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
dwkitchen commentedI think the best option is to add a option in the gateway settings, default to TRUE.
If the setting is not set then it will fallback to FALSE for upgrading user, maybe should consider a message to tell the site admin to set it.
Comment #2
rszrama commentedThought about this most of the morning, and while I like the compromise and think the chances of a MITM attack of this nature are low for any given site, I still don't like providing a UI to "opt" for insecurity. We did that with the example recurring payment method in UC Recurring and basically ended up with a bunch of live sites storing CC numbers in the database, some of whom even made the news for getting hacked and losing that CC data. : P
Soooo... here's what I'm thinking. Let's verifyhost / peer by default for all live transactions - it makes good sense, and the fact that we haven't been stretches back to my original development of the Ubercart payment method modules in a time when all the API docs were written without that verification (apparently to accommodate poor cURL maintenance). For test / sandbox transactions I don't see any real importance in verification, but until we start getting issues of people saying they aren't able to verify SSL certs on test servers, let's just go ahead and verify them, too. (The only reason I mention this is a test in the related Ubercart thread that failed for the PayPal sandbox; shouldn't affect this module regardless.)
For sites that get error messages pertaining to certificate verification, let's have the alternate option be to put the cert file into the module folder or the sites/all/libraries folder and reference that using the CURLOPT_CAINFO parameter. The question becomes how to label the option for that in the UI... perhaps with a checkbox right above the logging settings we just say "Use a local certificate bundle file." with a link to a documentation page explaining how to get the file (http://curl.haxx.se/docs/caextract.html) and where to put it on the server.
Also, for reference's sake, here's the source white paper: http://www.cs.utexas.edu/~shmat/shmat_ccs12.pdf
Comment #3
rszrama commentedSee my commit in #1931758-2: CURLOPT_SSL_VERIFYPEER should not be disabled for my eventual solution for PayPal. Basically, I opted not to clutter the user interface at all with options to disable peer verification. The vast majority of users will be fine, and what I do is make a variable available for those minority users who need to specify an alternate CA certificate bundle file. If the variable is set, I populate the CURLOPT_CAINFO setting with its value. Otherwise we trust the system to have up to date information. I'd recommend the same for Authorize.Net and any other Commerce module, obviously using alternate variable names (e.g. commerce_authnet_cacert).
Comment #4
travis-bradbury commentedComment #5
travis-bradbury commentedI followed the commit to the paypal module, but I changed the comment a bit so there's other suggestions before downloading today's CA bundle and never updating it.
Comment #6
travis-bradbury commentedA note about compatibility:
Most sites should not notice anything.
Sites which are the victims of a man-in-the-middle attack may suddenly be able to connect, protecting credit card details from being sent to attackers.
Sites which are running in an environment that is unable to verify authorize.net's certificate, possibly from using a very out of date openssl or system-level CA cert bundle, may be unable to process payments. This is the correct behaviour. You don't want to send payment details until you trust it's really Authorize.net you're communicating with.
Comment #7
heddnThanks for that update Travis. I was curious what this meant for backwards compatibility. It makes sense to do this. However, we should make sure this gets mentioned in release notes. But otherwise, this looks good to go. A straight forward solution to a long standing potential security vulnerability.
One might even argue this should use the security finding process, instead of the open forum.
Comment #9
mglamanThanks. Committed. Publishing the change record.