Proxmark / proxmark3

Proxmark 3
http://www.proxmark.org/
GNU General Public License v2.0
3.2k stars 911 forks source link

Issues with the parity calculations on ISO 14443 tags #25

Closed peterfillmore closed 9 years ago

peterfillmore commented 10 years ago

Currently the proxmark firmware is limited to sending a max of 32 byte ISO 14443 tags due to issues with the parity generation. Naturally this limit is not readily apparent as no indication of this limit is provided. Possibly we should put in a warning when attempting to read/emulate large tags. This is due to the parity calculations for packets being performed using a uint32_t variable as a shift register. This affects a large portion of the code base for the ISO14443 support.

I've implemented fixes in an old version of the proxmark3 code - but will need to port it to the latest release for it to work.

Additionally we should also consider moving the modulation/demodulation code into the FPGA if possible. The current limits with RAM in the hardware makes emulating tags of >64 bytes impossible - as tag emulation requires taking the raw bit stream and expanding it to the required modulation in RAM. If the modulation/demodulation code is moved to the FPGA then RAM would be conserved allowing bigger tags to be modulated.

iceman1001 commented 10 years ago

Sorry for not understanding the code properly, but when you say "max of 32byte tag" what does that mean? What is the max dataframe size for a iso14443 enabled tag?

I suppose it is this function, and usages that you changed to uint64_t? iso14443a.c --- uint32_t GetParity(const uint8_t * pbtCmd, int iLen)

iceman1001 commented 10 years ago

I've been reading the desfire imp, in which the max frame size seems be to 64bytes. So, that would explain why I have been having trouble with implementing it if the parity only works up to 32bytes.

peterfillmore commented 10 years ago

@iceman1001 there is no max dataframe size in the iso14443 standard, however its generally 256 bytes in practice (set in the RATS command, FSDI). I initially tried changing parity to a uint64_t - but that just caused a lot more issues, and then just sets a limit of 64 byte frames anyway. In the end i ripped out the parity generation code and implemented as a structure of parity bytes which gets bigger as the frame increases. This requires changing the modulation code, the tracing code and functions using parity - hence why i've not commited the changes to date, but will start merge the solution over time.
Just though it would be good to point out one of the more frustrating bugs in the code.

iceman1001 commented 10 years ago

At these times, I wish I knew more of the whole codebase. Since I don't understand why we sent the parity seperately, I can't even think of a better solution. But when you mention it, why should it be sent with a uint32_t to start with?

Lets talk about the solution instead, why are we sending parity bytes at all? Why can't it be calculated at hand?

peterfillmore commented 10 years ago

The parity isn't sent separately, its calculated using the "GetParity" function which takes the frame bytes and length of frame and returns the calculated parity stream (as a uint32_t). I fixed it in my version by calculating parity on the fly in the "CodeIso14443aAsTagPar" function (instead of using the supplied parity) - this generates the modulated bit stream which is then sent to the FPGA. For the demodulation code (MillerDecoding,ManchesterDecoding), it may be easier to just ignore parity anyway (we don't check it to begin with). The tracing code will also need to be altered to ignore parity in this case.

iceman1001 commented 10 years ago

I agree, it should be easy to call "GetParity()" from inside "CodeIso14443aAsTagPar, in the mainloop.

pwpiwi commented 10 years ago

(we don't check it to begin with)

but we do - on the client side. Parity errors are indicated with an exclamation mark in the output of hf 14a list.

peterfillmore commented 10 years ago

Do we need to check the parity in the logger? It can at least be optional.

pwpiwi commented 10 years ago

Is there any reason NOT to check the parity?

peterfillmore commented 10 years ago

Because the parity (in the case of iso14443a tags > 32 bytes) is wrong (see above). This then affects the trace printing code (at least in iso14443a.c): 110 if (isResponse && (oddparity != ((parityBits >> (len - j - 1)) & 0x01))) { since parity is stored in a uint32_t, a 33 byte packet means - parityBits >> (33-0-1) = parityBits >> 32 = 0; always. so lots of parity errors. Either we fix the parity scheme to support >32byte packets, or at least warn the user about this.

merlokk commented 10 years ago

The parity of mifare crypto1 different. It not as standard iso14443A parity. Because of that it was coded by this strange way. You cant calculate parity in mifare operations withouth decrypting crypto1. But... The 32-bit parity was implemented because it was exist at codebase. I tried to refactor code, but it was too much work there.

pwpiwi commented 10 years ago

Calculating parity on the fly works only on unencrypted transfers as merlokk pointed out. The parity bits need to be encrypted / decrypted together with the data bits. There is therefore no way to get rid of it. Best solution would be to fix it to support more than 32 bits. Any volunteers?

pwpiwi commented 10 years ago

Having read the iso14443 once again I am not convinced any more that we have an issue at all. Don't card and reader agree on the maximum supported frame size during the card selection process? And don't they use chaining then to transfer bigger PDUs? So why is there a need for the PM3 to support frame sizes bigger than 32 Bytes?

peterfillmore commented 10 years ago

It is an issue as its not compliant with the ISO14443 docs, frame sizes defined up to 256 bytes, and custom apps can go bigger if wanted. Many applications hard code a frame size of 256 bytes and fail if not detected. Other devices may not support chaining (i.e the ACR-122U readers for example). Having a small frame size also makes it painful to send big packets, as chaining is not yet added to the iso14443 code (although i have one coded up). Not having the ability to generate long tags means that we're not able to fuzz lots of applications.

pwpiwi commented 10 years ago

OK - agreed that the PM3 is a tool which should be able to communicate even with non-ISO-compliant devices.

Because nobody else volunteered, I will fix the code to support 256 byte frames (including parity of course). However I need someone to test - I have no tags supporting big frames available.

peterfillmore commented 10 years ago

Cheers, thanks for that. I can test it, i have a range of ISO14443 devices/terminals and cards to test against. i'll also update my github to have my experimental EMV version up - which has a hacked together parity fix. Once the parity stuff is fixed i'll put together code to read and emulate NFC payments so people can start playing with Apple pay etc.

iceman1001 commented 10 years ago

Cheers PwPiwi! Since I don't understand those parts of the code, I'm not able to take on the challenge. However, didn't Peter say he had an implementation for tagsizes up to 256? Or was that 64? I'm confused.

martoni5218 commented 9 years ago

Hi, I am just working on a electronic passport simulator using SimulateIso14443aTag function as my bachelor thesis and I encountered with this parity problem. As a passport reader I am using Golder reader and it does not allow to read less than 32bytes. The problem is, that I need to send at least 37 bytes in total. I tried to chain, but I am using acs acr122U reader, so it failed. So I am glad to see that someone is up to solve this issue. I would be much help :)

pwpiwi commented 9 years ago

pushed changes to my repository (https://github.com/pwpiwi/proxmark3.git). Parity is now a uint8_t[] which required quite some changes. I could only verify that the hf 14a and hf mf functions still work with Mifare cards (max 16 Byte frames). Can please someone check the iclass functions and of course cards with >32 byte frames.

pwpiwi commented 9 years ago

Finally found a tag with which I could test bigger frames (my own passport :grinning:) - and it works.

@holiman: Instead of pushing directly I will make a pull request. Can you please check the iclass functions before merging?

holiman commented 9 years ago

Great work, piwi!

I haven't had access to iclass tags/readers since june, but as it happens, a fellow forum member has promised to ship me a reader and a couple of tags, so hopfully I can verify the functionality in January.

iceman1001 commented 9 years ago

Indeed great work pwpiwi! I and the rest of us are very grateful of your hard och dedicated work to the proxmark community.

Sadly my passport is still not read. I think I may need a stronger HF antenna to power the chip.

pwpiwi commented 9 years ago

I finally found an iclass reader and tag to test the extended parity with the hf iclass commands as well. No issues. Merged.