Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add padding to ARQC verification in JCESecurityModule #575

Closed
wants to merge 8 commits into from

Conversation

T-eli
Copy link
Contributor

@T-eli T-eli commented Dec 15, 2023

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
Copy link
Member

ar commented Dec 17, 2023

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

  • IssuerApplicationDataTest. testRandom()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M1()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M1_OA_P19()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M1_OB()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M1_OB_P19()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M2()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P00_M2_OB_P19()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P80_M1()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P80_M1_OB_P19()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P80_M2()
  • JCESecurityModuleTest. testVerifyARQCGenerateARPCImpl_CSKD_P80_M2_OB_P19()
  • JCESecurityModuleTest. testVerifyARQCImpl_CSKD_P00()
  • JCESecurityModuleTest. testVerifyARQCImpl_CSKD_P80()

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
Copy link
Member

ar commented Dec 17, 2023

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
Copy link
Contributor Author

T-eli commented Dec 19, 2023

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
Copy link
Member

ar commented Dec 19, 2023

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
Copy link
Member

ar commented Dec 19, 2023

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
Copy link
Contributor Author

T-eli commented Dec 20, 2023

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
Copy link
Member

ar commented Dec 20, 2023

Excellent.

@T-eli
Copy link
Contributor Author

T-eli commented Dec 21, 2023

please checkout PR #577

@T-eli T-eli closed this Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants