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

Test failure in CryptoTest line 53 #22

Open
rpavlik opened this issue Dec 11, 2023 · 4 comments
Open

Test failure in CryptoTest line 53 #22

rpavlik opened this issue Dec 11, 2023 · 4 comments

Comments

@rpavlik
Copy link
Contributor

rpavlik commented Dec 11, 2023

I poked at Ant some more to get the tests running using the jcardsim library: see https://github.com/rpavlik/GidsApplet/tree/junit . All the tests pass (on both variants), except for one: CryptoTest.

It fails on line 53, with the error message: expected: 9000 but was: 618c. Line 53 has the commennt "generate asymmetric key" and is using instruction 0x47.

I can't find where it's actually returning 618c in the first place, so I'm not sure if this is an issue with the jcardsim or something else.

Here's the log file:
TEST-com.mysmartlogon.gidsAppletTests.CryptoTest.txt

I checked out the 1.3 source without touching the build system, and it looks like it too has the same issue, though 1.2 does not.

Not sure what version of jcardsim you used originally, maybe I can revert to that version?

I'm running the tests with jcardsim 3.0.5 under jdk17, though I build the applet with jdk8.

@rpavlik
Copy link
Contributor Author

rpavlik commented Dec 11, 2023

now that's interesting...

This is the commit that broke the test: 9477d5c "remove feitian workaround"

I don't fully grasp what it was doing, but the fact it breaks j3h145 (jcop3) is interesting. Absence of it apparently breaks jcardsim. I haven't tried enabling that "workaround" on my j3r180 or my ancient credentsys lite (jc2.2.1) cards but I can if it's useful.

In a pinch, we could do the same thing as with the buffer size, build multiple variants with/without it, however it seems very strange that the self-tests fail without that workaround.

To try different versions, all I did was:

git checkout 1.2 -- src/com/

replacing 1.2 with a git hash if looking at a specific commit.

@vletoux
Copy link
Owner

vletoux commented Dec 12, 2023

you're getting at the root of supporting multiple cards ...

@rpavlik
Copy link
Contributor Author

rpavlik commented Dec 12, 2023

Ah ha ha, I have fallen too far down the rabbit hole 😅

Ok, no problem. Honestly I feel better seeing that it works with the workaround, mentally I can categorize it as "the simulator acts like a Feitian card".

I promised myself I wouldn't touch Java cards today, but eventually I'll probably do something about this 😅

@rpavlik
Copy link
Contributor Author

rpavlik commented Dec 15, 2023

Notes to self:

618c means that we needed an extended APDU when returning the public key, apparently: it's literally saying that the remaining length to send is 0x8c. We get there because le is 0, because we are generating an asymmetric key pair so ins is equal to INS_GENERATE_ASYMMETRIC_KEYPAIR. The minimum of 0 and 0x8c is 0, so that's how much we send back in our first response apdu.

On the other hand, when the "workaround" is uncommented, we get to this line, letting us send 256 bytes back in our first response apdu! https://github.com/vletoux/GidsApplet/blob/master/src/com/mysmartlogon/gidsApplet/TransmitManager.java#L304

because in this case, ins is somehow 0xbb in my case - because we overwrite buf in the workaround! (we extract the parts of the private key to buf temporarily)

So, we just need to find out why we are setting le = 0 when ins == GidsApplet.INS_GENERATE_ASYMMETRIC_KEYPAIR, because at least on the jcardsim, that appears to be the reason why the workaround changes anything. I don't see anything in the GIDS spec saying that you can't send any data back in the first response APDU.

Looks like it was added in this commit: 4e24e39

OK, so why is Le 0? Does it have to do with being in T=0 for the simulator instead of T=1 for real card?

Le gets populated in the sim in APDUProxy.internalReset.

Goes through the "Case 3" case, because that's the APDU case that got passed in from SimulatorRuntime, because that's what gets detected from the command APDU with ApduCase in the sim. That looks right: longer than 7, the Lc byte is nonzero (it's 8), and the APDU parses to not have an Le field.

(Coincidentally, CommandAPDU from the java runtime I guess? also decides it's Case 3s, which has no Le field)

OK, so that's why Le = 0. The command APDU is structured to not have an Le field which implies it's 0. However, we have some non-zero thing to return - we have a public key! which is why we get the error (effectively E_AGAIN) telling us there's more data. (I'm not sure how legal a command that is, how legal it is to send that generate command that way and try to reply with an extended APDU, nor how legal it is to pretend Le was 256 in some cases when it's zero. Presumably these are issues you hit when the code encounters the real world.) The GIDS spec says that Le should be specified for that command APDU as the "length of public key of DO template", which I'll take a wild guess that it's 0x83.

So actually maybe the 618c is a correct response for that line? it did the generation of the key pair and stored it, it's just trying and failing to return it. I imagine that test exists because some existing device sent that APDU, though, so the solution is probably not to just add 8c to the end of the command APDU, I fear. But it looks like a test that invokes undefined behavior, or at least unspecified behavior, as far as the GIDS spec goes.

But, the test passes without the "workaround" (which at least as far as this failure goes, looks like a bug) if I add 8c to the end of the test command APDU.

This was referenced Dec 19, 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

No branches or pull requests

2 participants