BryanJacobs / FIDO2Applet

FIDO2 Javacard Applet
MIT License
63 stars 12 forks source link

Don't chain response when makeCredential() is called using long APDU #4

Closed icedevml closed 10 months ago

icedevml commented 10 months ago

makeCredential() would always return a chained response, even if extended APDU is used to invoke the command. This is not compliant with the specification and would fail with iOS 17 Beta (and with earlier versions as well, I suppose).

This PR would simply force the response to be sent in one shot if makeCredential() is called using extended APDU, which would fix that command for iOS 17 Beta.

Merging this PR together with #3 would cause FIDO2Applet to start working correctly with iOS 17 Beta (tested with J3H145).

BryanJacobs commented 10 months ago

It looks to me like doSendResponse already has code for checking for E-APDU requests.

I'll write additional testing to figure out why it's not working, rather than specially handling makeCredential differently from the other commands.

icedevml commented 10 months ago

I think that's the problem:

        if (isExtendedAPDU) {
            // long APDUs are wonderful things, aren't they?
            apduBlockSize = (short) apdu.getBuffer().length;

so in my case (J3H145) the APDU buffer is smaller than the total response length, the function would decide to always use chaining, even with extended APDUs. And chaining is unfortunately not allowed by the spec in combo with E-APDUs and would not work with iOS unfortunately.

By doing:

            apdu.setOutgoing();
            apdu.setOutgoingLength(outputLen);
            apdu.sendBytesLong(bufferMem, (short) 0, outputLen);

we are able to dodge the problem of the small APDU buffer size. It is possible to actually return a response that is bigger than the APDU buffer itself. The sendBytesLong could also be called multiple times, and some of the content may be outputted from the APDU buffer itself, if that's necessary.

BryanJacobs commented 10 months ago

That makes sense to me. I don't have a physical reader that chooses E-APDUs, so I hadn't tested this case. The implementation in there rn Works In Theory but apparently not in reality.

I'll change the doSendResponse to use sendBytesLong as you suggest; that way, all commands will be fixed instead of just makeCredential.

Give me a moment, I'll push up a change and you can test.

icedevml commented 10 months ago

This is an example of what should happen:

=> 80 10 00 00 0000d9 01a401582057d51a2f704f91ec01f5e7fd1a514dd4171ddf52aef3d38f44799ebae41e905c02a26269646f64656d6f2e79756269636f2e636f6d646e616d656f64656d6f2e79756269636f2e636f6d03a36269645820ae08d45890b123d3e667a5cb2a8ba89ffd877616700a364a9dcb080aa5bbbd1b646e616d657059756269636f2064656d6f20757365726b646973706c61794e616d657059756269636f2064656d6f20757365720482a263616c672664747970656a7075626c69632d6b6579a263616c6739010064747970656a7075626c69632d6b6579 0000

<= 00a301667061636b65640258c4c46cef82ad1b546477591d008b08759ec3e6d2ecb4f39474bfea6969925d03b74100000000000000000000000000000000000000000040d3694a3155fd118d5b07363eff686580c7960ad55e0727f23c02960a53dfb0d1dfa7bfc1b99cdf053a19dde9c535ed5122e4839cb7cb9521ac8e2051eddf2f81a50102032620012158208c855c076ae65fbb670ffa8c2692a7dcc2b7bf6e115cf781013bbba566d0ed2a22582096141454cb83b8a01d3eb86645e055798478cf6d485aa22b1382020270c4a3ae03a263616c672663736967584630440220482ada75a4ae6582f492cb900cccbff21610788fa1ea3566070e76d007eab63d02207730b9d32f3761760221f121e571847b3dda84fdc53b1b06ca8036b87b550040 9000

The response may be chained with ISO 14443-4 chaining (that is handled automatically on both card OS level and the reader), although it's not allowed to use ISO 7816-4 chaining and shouldn't throw 0x61XX status code, because this wouldn't be understood.

BryanJacobs commented 10 months ago

I've pushed some code to the branch eapdu_testing. Could you possibly give it a try?

I'll set up to test E-APDUs better (patching jcardsim?) but that will take me a bit more time.

What I've done in the branch there is to repeatedly call sendBytes with an APDU-full at a time. I think that should work?

icedevml commented 10 months ago

This is what happens now with J3H145 and E-APDU.

The authenticatorGetInfo goes through succesfully, then on makeCredential we have:

   23652708 |   23652708 | Tag |13  00  a3  01  66  70  61  63  6b  65  64  02  58  c4  c4  6c  ef  82   |     |
            |            |     |ad  1b  54  64  77  59  1d  00  8b  08  75  9e  c3  e6  d2  ec  b4  f3   |     |
            |            |     |94  74  bf  ea  69  69  92  5d  03  b7  41  00  00  00  02  00  00  00   |     |
            |            |     |00  00  00  00  00  00  00  00  00  00  00  00  00  00  40  02  f0  c6   |     |
            |            |     |cd  dd  0f  a7  ab  08  cd  99  73  37  f5  cf  ca  9e  67  d6  b4  cc   |     |
            |            |     |53  e3  5b  80  40  a0  8e  ba  c2  c6  9a  d9  97  7d  b8  30  6b  00   |     |
            |            |     |a8  0b  02  12  f7  6f  6a  2e  5e  b3  a8  0f  ef  0f  db  1a  f8  f7   |     |
            |            |     |6b  02  40  64  5e  ac  e3  a5  01  02  03  26  20  01  21  58  20  34   |     |
            |            |     |f9  f9  49  7a  be  42  99  9b  7b  59  e8  29  ba  de  1b  a1  c3  98   |     |
            |            |     |62  72  51  bc  1f  e1  c5  c4  ff  b7  dc  31  3d  22  58  20  aa  dc   |     |
            |            |     |29  6e  23  42  3c  02  9f  49  62  9e  b8  ea  61  3b  d9  9c  4a  c6   |     |
            |            |     |e4  05  7b  97  93  f7  3d  80  1c  66  91  4e  03  a2  63  61  6c  67   |     |
            |            |     |26  63  73  69  67  58  46  30  44  02  20  66  8d  0c  fb  66  08  c0   |     |
            |            |     |5c  2d  f1  d8  8e  ef  1c  e5  29  4b  99  0b  95  46  ae  65  fd  ee   |     |
            |            |     |1d  3d  4f                                                               |  ok |
   23953840 |   23957392 | Rdr |a2  e6  d7                                                               |  ok |
   23967348 |   23981236 | Tag |12  c4  06  c0  b2  17  b9  02  20  7f  a1  4e                           |  ok |
   23988528 |   23992080 | Rdr |a3  6f  c6                                                               |  ok |
   24084340 |   24090164 | Tag |03  6f  00  ed  ac                                                       |     |

So it sent two chunks correctly and then the third chunk threw 0x6F00.

BryanJacobs commented 10 months ago

I've tried manually adjusting the APDU buffer length the code sees to 251 bytes.

I was, in that case, calling setOutgoing twice (against the rules). Try with the new commit on the branch?

By the way, thanks tons for debugging this. I really appreciate it.

icedevml commented 10 months ago

Uhm... My existing J3H145 card has just died during these tests. I had to switch to A40CR. It seems to work on my end with both iOS and Windows PC/SC.

BryanJacobs commented 10 months ago

Hopefully this applet didn't do it - I've installed hundreds of times onto my J3H145 :-).

I'll merge the branch, close the PRs, and mark iOS tested in a bit. Thanks again for the help!

icedevml commented 10 months ago

Awesome, thanks for your prompt replies and fixes. At the first glance, it looks good and I think this branch can be merged.

I will get back to you if I spot anything.

Some of the cards are really stressed here on daily basis, so I really doubt it's the applet, although that was a very surprising application of murphy's law 😅

BryanJacobs commented 10 months ago

No problem. Feel free to open another PR/issue if something else is wrong.

I'll work out an off-card testing strategy for these cases when I get a spare moment.