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
multiple fixes for 2K and 4K cards #2028
Conversation
The last 8 sectors of a 4K card have 16 blocks per sector. There are still only 4 sets of access control bits, so they apply to groups of blocks: (0-4, 5-9, 10-14, and 15). Added some helper functions to map block numbers of sector and group, and use these in the sector trailer encode/decode functions instead of blocknumber%4. Fixed the spelling error of EncodeSectorAndClockTailer() by renaming to EncodeSectorAndBlockTailer() and adding a synonym for the old name. Fixed an error in FormatNdef for 2K and 4K cards - it was writing block 3 a second time rather than writing block 67. Renamed the first parameter of SetCapacity from ATAQ to ATQA. fixed CRC byte for MAD directory in sector 16 write all four blocks in sector 16 re-factor writing of all NFC sector trailers Rename blockNumber to blockGroup in DecodeSectorTailer Treat KeyA in AuthenticateBlockKeyA the same as KeyB in AuthenticateBlockKeyB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements and fixes. Just few comments and then we'll be good to go.
@@ -888,6 +913,8 @@ public bool FormatNdef(ReadOnlySpan<byte> keyB = default) | |||
/// <param name="message">The NDEF Message to write</param> | |||
/// <param name="writeKeyA">True to write with Key A</param> | |||
/// <returns>True if success</returns> | |||
/// TODO: check the MAD to determine which sectors can be used, instead of assuming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the comment for the intellisense comment, meaning the developer needs to check it or it for the code and that needs to be done in a later fix/improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a note to the developer. I've removed it and will open a bug for the additional change.
@@ -487,7 +523,17 @@ public AccessType BlockAccess(byte blockNumber, byte[] sectorData) | |||
/// <param name="accessSector">The access desired</param> | |||
/// <param name="accessTypes">An array of 3 AccessType determining access of each block</param> | |||
/// <returns>The 3 bytes encoding the rights</returns> | |||
public (byte B6, byte B7, byte B8) EncodeSectorAndClockTailer(AccessSector accessSector, AccessType[] accessTypes) | |||
/// This is a synonym of EncodeSectorAndBlockTailer (for backward compatibility) | |||
public (byte B6, byte B7, byte B8) EncodeSectorAndClockTailer(AccessSector accessSector, AccessType[] accessTypes) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you mark this one as obsolete? So we can remove it in a further release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now marked as obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for those fixes, looks all good to me
@krwq maybe you want to have a quick look so we can merge this one shortly? |
Thanks @jdbruner! |
MifareCard.cs notes that it only supports 1K cards. There is some code for 2K and 4K cards, but there are some shortcomings (particularly for 4K cards). This change introduces a number of fixes to resolve those shortcomings.
Background
Mifare Classic comes in two sizes: 1K and 4K. However, when Mifare Plus cards are operated at security level 1, they look like Mifare Classic cards. Mifare Plus comes in 2K and 4K sizes.
Memory is organized into sectors, which comprise data blocks and a sector trailer block. All blocks are 16 bytes.
The sector trailer is the last block in the sector. It contains the two authentication keys (A and B), and access bytes. The access bytes encode eight different configurations for data blocks and the sector trailer. There are four sets of 3-bit access conditions. For sectors less than 32, each set of access conditions applies to one block within the sector. For sectors 32 and above, the first three sets of access conditions apply to 5-block groups (0-4, 5-9, 10-14), and the final set of access conditions applies to the sector trailer.
Changes
The pre-existing code assumed that there were always 4 blocks per sector and the sector trailer was always the fourth one. To handle larger sectors, this change introduces the concept of a "block group" within a sector, which represents the blocks that are associated with the access conditions. There are four block groups per sector. For sectors less than 32 there is one block per group. For sectors above 32, there are five data blocks per group; the last group is the sector trailer.
Convenience methods to convert between block numbers and block groups were added, and they replace previously hardcoded calculations (such as
blockNumber % 4
) that are only correct for the first 32 blocks.The
FormatNdef
method formats the entire card for tags according to the NDEF specification. In general, the blocks on a Mifare Classic card may be assigned to multiple applications. The assignment is indicated by the Mifare Application Directory, which resides in sector 0 in all cards and also in sector 16 for cards larger than 1K. The previous code wasn't writing sector 16 correctly, and it wasn't using the correct checksum for the Mifare Application Directory in sector 16.In addition, there are some miscellaneous fixes.
EncodeSectorAndClockTailer
was an unfortunate misspelling. It is renamed toEncodeSectorAndBlockTailer
, but for backward compatibility there is now a method with the old name that simply calls the new one.Renamed the name of the first parameter of SetCapacity from ATAQ to ATQA.
Changed
AuthenticateBlockKeyA
to treatKeyA
in the same way thatAuthenticateBlockKeyB
treatsKeyB
.Verification
Verified that both 1K and 4K cards are now correctly formatted and that read/write access to the second half of 4K cards (with large sectors) work correctly.
Microsoft Reviewers: Open in CodeFlow