Avoid storing partial credit card payment info

[Bill Erickson]

Evergreen 2.8

When a (non-Stripe) credit card payment is made in Evergreen, some card data is stored locally in the EG database. Retaining even partial credit card payment information considerably raises the bar for PCI compliance. Since most vendors (I think, certainly PayPal) allow you to retrieve payment information directly from them with the approval code / order number, storing the data locally in Evergreen is also redundant.

I'd like to propose that we drop (or anonymize, or leave blank) the following columns in money.credit_card_payment:

cc_type
cc_number (last 4)
expire_month
expire_year
cc_first_name
cc_last_name

Thoughts?

[Rogan Hamby (rogan-hamby)]

I'd be in favor of blocking. Because of PCI compliance we've modified the
staff client so that staff can't attempt to type anything in there and I
routinely wipe anything in the database.

On Mon, Jul 13, 2015 at 12:24 PM, Bill Erickson <email address hidden> wrote:

> *** This bug is a security vulnerability ***
>
> Public security bug reported:
>
> Evergreen 2.8
>
> When a (non-Stripe) credit card payment is made in Evergreen, some card
> data is stored locally in the EG database. Retaining even partial
> credit card payment information considerably raises the bar for PCI
> compliance. Since most vendors (I think, certainly PayPal) allow you to
> retrieve payment information directly from them with the approval code /
> order number, storing the data locally in Evergreen is also redundant.
>
> I'd like to propose that we drop (or anonymize, or leave blank) the
> following columns in money.credit_card_payment:
>
> cc_type
> cc_number (last 4)
> expire_month
> expire_year
> cc_first_name
> cc_last_name
>
> Thoughts?
>
> ** Affects: evergreen
> Importance: Undecided
> Status: New
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: evergreenbugs
> https://bugs.launchpad.net/bugs/1474051
>
> Title:
> Avoid storing partial credit card payment info
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen 2.8
>
> When a (non-Stripe) credit card payment is made in Evergreen, some
> card data is stored locally in the EG database. Retaining even
> partial credit card payment information considerably raises the bar
> for PCI compliance. Since most vendors (I think, certainly PayPal)
> allow you to retrieve payment information directly from them with the
> approval code / order number, storing the data locally in Evergreen is
> also redundant.
>
> I'd like to propose that we drop (or anonymize, or leave blank) the
> following columns in money.credit_card_payment:
>
> cc_type
> cc_number (last 4)
> expire_month
> expire_year
> cc_first_name
> cc_last_name
>
> Thoughts?
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1474051/+subscriptions
>

[Rogan Hamby (rogan-hamby)]

To clarify +1 to dropping the columns

[Jason Boyer (jboyer)]

+1 to dropping them altogether.

[Bill Erickson (berick)]

Here's some code to drop the columns, with pgtap and release notes:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1474051-drop-cc-data

[Mike Rylander (mrylander)]

This will outright break existing reports, including, potentially, local views (real or in-IDL) that mention those columns in any way... should we be concerned about that? If consensus is "those reports are wrong and bad, and you should be ashamed of yourself for creating them" then that's perfectly fine by me, but I felt the point should be raised.

[Rogan Hamby (rogan-hamby)]

I'm on the "you should be ashamed of yourself" side of the opinion fence.
But, I can also be fairly cavalier about it as it doesn't break anything
for my users that I don't want broken. I think most institutions would feel
this way.

If we feel a wider net of feedback is needed I think this might be a good
question to throw out to the general list. I find it hard to imagine that
many would want to keep it but I'm often surprised by such things. I would
be willing to send out an email and report back in the bug feedback.

On Monday, July 13, 2015, Mike Rylander <email address hidden> wrote:

> This will outright break existing reports, including, potentially, local
> views (real or in-IDL) that mention those columns in any way... should
> we be concerned about that? If consensus is "those reports are wrong
> and bad, and you should be ashamed of yourself for creating them" then
> that's perfectly fine by me, but I felt the point should be raised.
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: evergreenbugs
> https://bugs.launchpad.net/bugs/1474051
>
> Title:
> Avoid storing partial credit card payment info
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen 2.8
>
> When a (non-Stripe) credit card payment is made in Evergreen, some
> card data is stored locally in the EG database. Retaining even
> partial credit card payment information considerably raises the bar
> for PCI compliance. Since most vendors (I think, certainly PayPal)
> allow you to retrieve payment information directly from them with the
> approval code / order number, storing the data locally in Evergreen is
> also redundant.
>
> I'd like to propose that we drop (or anonymize, or leave blank) the
> following columns in money.credit_card_payment:
>
> cc_type
> cc_number (last 4)
> expire_month
> expire_year
> cc_first_name
> cc_last_name
>
> Thoughts?
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1474051/+subscriptions
>

[Dan Scott (denials)]

Crazy hack land thinking here while roasting cover: what if the quick fix
is to add a BEFORE trigger that just inserts random data in place of
whatever might have been captured for those columns, until such time as we
fix the reports etc?

On Mon, Jul 13, 2015, 18:50 Rogan Hamby <email address hidden> wrote:

> I'm on the "you should be ashamed of yourself" side of the opinion fence.
> But, I can also be fairly cavalier about it as it doesn't break anything
> for my users that I don't want broken. I think most institutions would feel
> this way.
>
> If we feel a wider net of feedback is needed I think this might be a good
> question to throw out to the general list. I find it hard to imagine that
> many would want to keep it but I'm often surprised by such things. I would
> be willing to send out an email and report back in the bug feedback.
>
> On Monday, July 13, 2015, Mike Rylander <email address hidden> wrote:
>
> > This will outright break existing reports, including, potentially, local
> > views (real or in-IDL) that mention those columns in any way... should
> > we be concerned about that? If consensus is "those reports are wrong
> > and bad, and you should be ashamed of yourself for creating them" then
> > that's perfectly fine by me, but I felt the point should be raised.
> >
> > --
> > You received this bug notification because you are subscribed to
> > Evergreen.
> > Matching subscriptions: evergreenbugs
> > https://bugs.launchpad.net/bugs/1474051
> >
> > Title:
> > Avoid storing partial credit card payment info
> >
> > Status in Evergreen:
> > New
> >
> > Bug description:
> > Evergreen 2.8
> >
> > When a (non-Stripe) credit card payment is made in Evergreen, some
> > card data is stored locally in the EG database. Retaining even
> > partial credit card payment information considerably raises the bar
> > for PCI compliance. Since most vendors (I think, certainly PayPal)
> > allow you to retrieve payment information directly from them with the
> > approval code / order number, storing the data locally in Evergreen is
> > also redundant.
> >
> > I'd like to propose that we drop (or anonymize, or leave blank) the
> > following columns in money.credit_card_payment:
> >
> > cc_type
> > cc_number (last 4)
> > expire_month
> > expire_year
> > cc_first_name
> > cc_last_name
> >
> > Thoughts?
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/evergreen/+bug/1474051/+subscriptions
> >
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> https://bugs.launchpad.net/bugs/1474051
>
> Title:
> Avoid storing partial credit card payment info
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1474051/+subscriptions
>

[Kathy Lussier (klussier)]

+1 to dropping the columns.

As far as breaking reports, we would need a good release notes entry giving people a heads up about the potential problem before the upgrade to a release with this code. However, I'm hoping there aren't many reports out there that would be impacted by the change. I don't know many libraries that would be likely to report on those fields. If the concern regarding reports is very strong, I would be in favor of a message to the general list as Rogan suggested.

[Bill Erickson (berick)]

Emailed general/dev lists.

[Bill Erickson (berick)]

Current release notes: http://evergreen-ils.org/~erickson/drop-cc-columns.html

[Jason Stephenson (jstephenson)]

I am for getting rid of the information.

My attitude to reports in general is: if they break, they break.

[Bill Erickson (berick)]

Noting for future reference that my "does anyone care?" email has gone unanswered for 1 week.

[Rogan Hamby (rogan-hamby)]

That sounds like consent via disinterest.

On Tue, Jul 21, 2015 at 10:59 AM, Bill Erickson <email address hidden> wrote:

> Noting for future reference that my "does anyone care?" email has gone
> unanswered for 1 week.
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: evergreenbugs
> https://bugs.launchpad.net/bugs/1474051
>
> Title:
> Avoid storing partial credit card payment info
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen 2.8
>
> When a (non-Stripe) credit card payment is made in Evergreen, some
> card data is stored locally in the EG database. Retaining even
> partial credit card payment information considerably raises the bar
> for PCI compliance. Since most vendors (I think, certainly PayPal)
> allow you to retrieve payment information directly from them with the
> approval code / order number, storing the data locally in Evergreen is
> also redundant.
>
> I'd like to propose that we drop (or anonymize, or leave blank) the
> following columns in money.credit_card_payment:
>
> cc_type
> cc_number (last 4)
> expire_month
> expire_year
> cc_first_name
> cc_last_name
>
> Thoughts?
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1474051/+subscriptions
>

[Galen Charlton (gmc)]

Upon reviewing this patch, I have a question: does it result in producing (when not using Square as the processor) receipts that are not acceptable?

I'm raising this based on my reading of the MasterCard transaction rules (http://www.mastercard.com/us/merchant/pdf/TPR-Entire_Manual_public.pdf), around page 77, which require that receipts contain (among other things)

- the obscured credit card number (without expiration date)
- the transaction authorization code

I glanced at a few of my personal PayPal and Square receipts, and they do include those data elements.

Tossing this back to Bill to investigate requirements for his library -- and for anybody watching this bug who accepts credit card payments.

Of course, getting this information onto the receipt doesn't require that the obscured CC number be stored in the database in money.credit_card_payment.

[Bill Erickson (berick)]

We're retaining the transaction authorization code, so that should be OK.

"A copy of the Transaction receipt must be provided to the Cardholder, either automatically or upon the Cardholder’s request". If we have to include the last 4 of the CC#, they'd have to be stored (somewhere) indefinitely. *sigh*

I'm requesting feedback locally...

[Galen Charlton (gmc)]

Not necessarily indefinitely, I'd think -- worth also finding out how long CC transaction history has to be retained. I saw a figure of 13 months when I was Googling the other day, but obviously guidance from somebody who actually knows would be good.

[Bill Erickson (berick)]

Got some local feedback and talked to our PCI vendor.

For starters, the PCI vendor was a little confused that we'd want to keep the last-4 at all, but when pressed w/ the posted PDF, said keeping the last-4 as long as "no other cardholder data was stored" was not enough to move you from PCI level C-VT to level D. (Level D is where you're filling out 200 page forms and things get really complicated). The most important things not to store are CCV (we're good there) and expire date (which we propose dropping in this bug). However, the bit about storing "no other cardholder data" concerns me some, since we are potentially storing other cardholder data, because we link the payment to the patron, which in many/most cases is probably the same as the cardholder. That potentially gives us first and last name on the card.

For our purposes locally, we are fine never storing the last-4 or including it on receipts. Given there is still confusion over exactly what it is required, though, I second Galen's suggestion of retaining the last-4 for a period of time.

I propose we drop all of the proposed columns except cc_number, which retains the last-4 as before, and we provide a tool (CRON script) to NULLIfy the values from cc_number once payments reach a configured age.

If this sounds sane, I can work on the changes.

[Bill Erickson (berick)]

Started on the above changes here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1474051-drop-cc-data-take2

TODO: add the cron script to Open-ILS/src/Makefile.am; update release notes to explain cron script.

[Bill Erickson (berick)]

Implemented the remaining TODO's and force-pushed back to user/berick/lp1474051-drop-cc-data-take2

[Martha Driscoll (mjdriscoll)]

I tested this on a test system running evergreen 2.9.1. I found no problems paying credit card payments after the code was installed. The receipt included the last 4 digits of the credit card but did not include the expiration month and year. In the database, there are NULL values in cc_type, expire_month, expire_year, cc_first_name, cc_last_name.

I then dropped the columns in money.credit_card_payment using XXXX.schema.drop_cc_cols.sql and restarted evergreen. I made another credit card payment which went through ok. Patron payment history did not break.

I installed the clear_cc_number.srfsh script in /openils/bin and added the example cron entry. The cron job ran and removed cc numbers for transactions older than 1 month (I changed the script to 1 month from 1 year).

[Kathy Lussier (klussier)]

Hi Bill,

I encountered a problem when logging into the client after loading this patch on a test server. When I launch the client and get to the point of workstation registration, CONS is the only org unit that appears in the organization dropdown menu. I can't register the workstation under any other org unit.

The login then fails with the following message:

Please open a helpdesk ticket and include the following text:

Tue Feb 09 2016 00:59:26 GMT-0500 (EST)

Error during login sequence. The client will logout after this dialog.

{
 "fileName":"oils://remote/xul/2016_02mlnc2_08_235342/server/OpenILS/data.js",
 "lineNumber":591
}

[Bill Erickson (berick)]

Hm, it's probably out of sync with master. I'll rebase and test.

[Bill Erickson (berick)]

Rebased branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1474051-drop-cc-data-rebase

I'm assuming/hoping the previous branch had simply drifted too far from master creating some inconsistency that caused the
errors. I didn't have any problems running the new code after rebase.

[Kathy Lussier (klussier)]

Unfortunately, I'm still having trouble. I loaded the patch on a clean install, and I only see one entry (CONS) in my actor.org_unit table.

[Bill Erickson (berick)]

Base schema was missing a ",". I've force-pushed back to user/berick/lp1474051-drop-cc-data-rebase to fix the problem and confirmed it builds correctly now.

[Kathy Lussier (klussier)]

Thank you Bill and for the great feedback Martha! Merged to master for inclusion in release 2.10.