jpos / jPOS

jPOS Project
http://jpos.org
GNU Affero General Public License v3.0
599 stars 458 forks source link

add padding to ARQC verification in JCESecurityModule #575

Closed T-eli closed 6 months ago

T-eli commented 6 months ago

this is related to issue #573.

according to EMV book 2 Application Cryptogram Generation, the data should be padded according to ISO9797 method 2 .

I see that JCESecurityModule.java has a private implementation for the padding method but it is not used in ARQC verification , while it is being used in ARPC calculation and generateSM_MACImpl.

i am not sure if Visa and and MasterCard requires the same or different padding, but at least it is part of common core definitions.

i have added a Test unit for ARQC generation using data based on EMV®Issuer and Application Security Guidelines v3.0 A.3.3 - ARQC Generation.

this pull request also include some text corrections and a fix for Issuer application data not matching if the hex string is lowercase.

ar commented 6 months ago

Thank you for the PR. Unfortunately, it breaks a few tests, namely:

Some of the failures are caused by the toUpperCase in ApplicationData IAD and we can correct them, but some others need more research.

Please try gradle test --tests=IssuerApplicationDataTest --tests=JCESecurityModuleTest --info

ar commented 6 months ago

FYI, just one example, in testVerifyARQCGenerateARPCImpl_CSKD_P00_M1 the calculated ARQC is the expected one, 36DBEE15F36B1E7F. With your padding change circa line 1047, we get 55BE45DD2C9E0CBF instead.

T-eli commented 6 months ago

hello @ar

Thank you for the PR. Unfortunately, it breaks a few tests, namely ...

the tests fail because the methods add the padding to data before passing it to calculateARQC or verifyARQC methods.

what i had in mind is that all common core specification implementations for cryptogram generation follow the same ISO9797 method 2 padding rule as per EMV book 2 A1.2.1. for example the Common Payment Application (CPA) specs follow the same padding method. so i thought it should be implemented directly in the cryptogram generation function instead of letting the developer manually add the padding. another reason is also because the CryptogramDataBuilder class which is used to build the transaction data does not add the required padding.

Some of the failures are caused by the toUpperCase in ApplicationData IAD and we can correct them, but some others need more research.

my bad, the ISOUtil.bytetohex() method return lowercase characters only. i suggest the solution in 9cf4c70 else if (len == 32 && (iad.startsWith("0F") || iad.startsWith("0f")) && (iad.startsWith("0F", 32) || iad.startsWith("0f", 32)))

ar commented 6 months ago

The tests fail because the methods add the padding to data before passing it to calculateARQC or verifyARQC methods.

@demsey You OK with the suggested changes?

ar commented 6 months ago

I see you've removed the test you've added in the initial PR:

   /**
     *  Test Data is based of EMV Issuer and Application Security Guidelines v3.0  Annex A.3.3 - Example of ARQC Generation
     */
    @Test
    public void TestARQCGeneration() throws Throwable {

        String PAN = "5413339000006165";
        String PANSeqNo = "00";
        String amount = "000000010000";
        String amount_other = "000000001000";
        String terminalCountryCode = "0840";
        String terminalVerificationResult = "0000001080";
        String transactionCurrencyCode = "0840";
        String transactionDate = "980704";
        String transactionType = "00";
        String unpredictableNumber = "11111111";
        String applicationInterchangeProfile = "5800";
        String applicationTransactionCounter = "3456";
        String IssuerApplicationData = "0FA500A03800000000000000000000000F010000000000000000000000000000";

        byte[] k = ISOUtil.hex2byte("9E15204313F7318ACB79B90BD986AD29");

        SecureDESKey kek =  jcesecmod.generateKey(SMAdapter.LENGTH_DES, SMAdapter.TYPE_RSA_PK);
        byte[] encKey = jcesecmod.encryptData(CipherMode.ECB, kek, k , new byte[8]);
        SecureDESKey mkac =  jcesecmod.importKey((short) 128, SMAdapter.TYPE_MK_AC, encKey, kek, false);

        byte[] d = ISOUtil.hex2byte(
                amount
                + amount_other
                + terminalCountryCode
                + terminalVerificationResult
                + transactionCurrencyCode
                + transactionDate
                + transactionType
                + unpredictableNumber
                + applicationInterchangeProfile
                + applicationTransactionCounter
                + IssuerApplicationData
        );

        boolean result = jcesecmod.verifyARQC(MKDMethod.OPTION_A, SKDMethod.EMV_CSKD, mkac, PAN, PANSeqNo,ISOUtil.hex2byte("C20039270FE384D5") ,ISOUtil.hex2byte(applicationTransactionCounter), ISOUtil.hex2byte(unpredictableNumber), d);

        assertTrue( result);

    }

If I add this test, it fails, so I'm not sure if this PR, in its current state, is missing something.

T-eli commented 6 months ago

If I add this test, it fails, so I'm not sure if this PR, in its current state, is missing something.

the test fails because the data is not correctly padded.

if it ok i will close this PR and create another one with a suitable solution. i think the cryptogram Data builder interface should have the padding method option to apply to the data

ar commented 6 months ago

Excellent.

T-eli commented 6 months ago

please checkout PR #577