Friends don’t let friends reinvent System.Security.Cryptography
A fellow coworker is upgrading one of our long time customer’s ASP.NET web site that was originally created in 2003, and all is going swimmingly. Alas, this project could not go gently into that good night.
He makes the web page and code changes, modifies the database, pushes all the changes up to the web site, and the site is working well. Customers are placing orders on the web site, order e-mails are being sent to the site owners with encrypted credit card information, and all are happy.
But there is always a fly in the ointment. All of a sudden, an order is placed on the web site, but when the site owner tries to run a decrypt on the credit card information in the e-mail, the decryption routine is reporting a failure. So naturally, we assume that we broke the code somehow, so we started to dive into the project to see what we did to destabilize it.
He asked for my opinions on this problem, so I came over and we started to go through the VB.NET Framework v2.0 project. While investigating the area of the code containing the exact message that the customer was getting, my blood pressure started to go up right away as we started to see code that looked like this:
If intNumberLength = 16 Then 'go through each of the 16 numbers and 'add the code for them into the return string intNumber(0) = CType(Mid(strNumber, 1, 1), Integer) intNumber(1) = CType(Mid(strNumber, 2, 1), Integer) intNumber(2) = CType(Mid(strNumber, 3, 1), Integer) intNumber(3) = CType(Mid(strNumber, 4, 1), Integer) intNumber(4) = CType(Mid(strNumber, 5, 1), Integer) intNumber(5) = CType(Mid(strNumber, 6, 1), Integer) intNumber(6) = CType(Mid(strNumber, 7, 1), Integer) intNumber(7) = CType(Mid(strNumber, 8, 1), Integer) intNumber(8) = CType(Mid(strNumber, 9, 1), Integer) intNumber(9) = CType(Mid(strNumber, 10, 1), Integer) intNumber(10) = CType(Mid(strNumber, 11, 1), Integer) intNumber(11) = CType(Mid(strNumber, 12, 1), Integer) intNumber(12) = CType(Mid(strNumber, 13, 1), Integer) intNumber(13) = CType(Mid(strNumber, 14, 1), Integer) intNumber(14) = CType(Mid(strNumber, 15, 1), Integer) intNumber(15) = CType(Mid(strNumber, 16, 1), Integer) strCharCodes(0) = encrypt_Char1(intNumber(0), intIndexCode) strCharCodes(1) = encrypt_Char2(intNumber(1), intIndexCode) strCharCodes(2) = encrypt_Char3(intNumber(2), intIndexCode) strCharCodes(3) = encrypt_Char4(intNumber(3), intIndexCode) strCharCodes(4) = encrypt_Char5(intNumber(4), intIndexCode) strCharCodes(5) = encrypt_Char6(intNumber(5), intIndexCode) strCharCodes(6) = encrypt_Char7(intNumber(6), intIndexCode) strCharCodes(7) = encrypt_Char8(intNumber(7), intIndexCode) strCharCodes(8) = encrypt_Char9(intNumber(8), intIndexCode) strCharCodes(9) = encrypt_Char10(intNumber(9), intIndexCode) strCharCodes(10) = encrypt_Char11(intNumber(10), intIndexCode) strCharCodes(11) = encrypt_Char12(intNumber(11), intIndexCode) strCharCodes(12) = encrypt_Char13(intNumber(12), intIndexCode) strCharCodes(13) = encrypt_Char14(intNumber(13), intIndexCode) strCharCodes(14) = encrypt_Char15(intNumber(14), intIndexCode) strCharCodes(15) = encrypt_Char16(intNumber(15), intIndexCode) 'strReturnValue = strCharCodes.ToString ElseIf intNumberLength = 15 Then ' you get the idea |
And this…
'first char Select Case Left(strEncodeCard, 1) Case "X" ' these character constants were originally all different strCardNumber(0) = "0" Case "X" ' I have masked them out for this blog posting strCardNumber(0) = "1" Case "X" strCardNumber(0) = "2" Case "X" strCardNumber(0) = "3" Case "X" strCardNumber(0) = "4" Case "X" strCardNumber(0) = "5" Case "X" strCardNumber(0) = "6" Case "X" strCardNumber(0) = "7" Case "X" strCardNumber(0) = "8" Case "X" strCardNumber(0) = "9" Case Else blnSuccess = False End Select |
(The above code was repeated 32 times, once for each digit of a potential 16 digit credit card number, and there were two sets of these that were switched if the digit position was odd or even. There was definitely some thought put into this. I’m not saying it was good thought, just thought. Perhaps they were paid by the lines of code written?)
OK, I have done things like this from time to time, usually not to this extent, but the original designers and coders of this project at least had the good sense to encrypt the credit card information that was being e-mailed, so maybe it will get better.
Yeah, I was wrong about that, I should remember a tenet of code maintenance that I learned a long time ago: TWGW. (This stands for Things Will Get Worse.) As we stepped through the order that was confusing the system, we came upon this nugget.
'second, select the expiration year code Select Case intExpYear Case 2003 cryptoExp_Year = "X" ' these character constants were originally all different Case 2004 cryptoExp_Year = "X" ' I have masked them out for this blog posting Case 2005 cryptoExp_Year = "X" Case 2006 cryptoExp_Year = "X" Case 2007 cryptoExp_Year = "X" Case 2008 cryptoExp_Year = "X" Case 2009 cryptoExp_Year = "X" Case 2010 cryptoExp_Year = "X" Case 2011 cryptoExp_Year = "X" Case 2012 cryptoExp_Year = "X" End Select |
Of course, the customer had a credit card that expired in 2013, and as such, no character was begin inserted into the encrypted string for the expiration year, and that was causing the problem.
I don’t even know where to begin, except to say that eventually this code was going to break, and that was the day. (System.Security.Cryptography was in .NET Framework v1.1, so they can’t use that as an excuse.) So the moral of the story is, if you are going to reinvent a Microsoft namespace, at least make it work more than six years.
But the code above is “more secure than the military requires”!
That is what I have heard. However I don’t believe it.