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

NFC: Add parser for CSC Service Works Reloadable Cash Card #137

Merged
merged 17 commits into from
Jul 10, 2024

Conversation

zinongli
Copy link
Contributor

@zinongli zinongli commented May 26, 2024

What's new

  • A parser plugin that can parse CSC Service Works cards. This type of card uses Mifare Classic 1k to store balance-related data in plain text, allowing for reading.
  • I used keys I got from nonces from washers in my building. The keys are then used for verification and reading in this parser. This could limit its potential to read card of the same type that uses a different key. But also prevents false positives.
  • I have no prior experience in C but I am down to spend more time on polishing this. Currently this parser works very slowly and I suspect it's due to the lack of optimization on my part. Any suggestions would be appreciated.
  • I can't seem to upload the dumps from both of my cards here for the reviewers to test on their own. Please let me know what would be the best way I can get that to you.
  • The wordings I chose like "Refill Times" can sound unobvious because this is not my first language. If you have better word-choice ideas please let me know.

If it's a normal card:

Screenshot-20240526-023832

Screenshot-20240526-023844

If it's a new card. The card price is read from the card itself. If there were no signature of refill from a vending machine, then the last refill value in the card would reflect the price for getting this card.

Screenshot-20240526-025514


For the reviewer

  • I've uploaded the firmware with this patch to a device and verified its functionality
  • I've confirmed the bug to be fixed / feature to be stable

@Willy-JL
Copy link
Member

thank you! ill get to reviewing the code shortly

I can't seem to upload the dumps from both of my cards here for the reviewers to test on their own. Please let me know what would be the best way I can get that to you.

you should be able to drag and drop into the github text box. if not, open up a ticket on discord :D (i think you already have one, if so let us know there and we'll open a new one)

The wordings I chose like "Refill Times" can sound unobvious because this is not my first language. If you have better word-choice ideas please let me know.

i assume "refill times" means how many times the card balance was refilled? and if it was never refilled, it shows "new card"? if so, maybe "Topped up X times" or "Top-up Count: X" could work

on the other hand, im not sure what the difference between "balance" and "refilled balance" is, and what "card usage left" means

@zinongli
Copy link
Contributor Author

Sure. I will send that to you on Discord.

I see. Top-up count does sound better. Balance is the remaining balance while refilled balance is how much there were when last refilled. So for example if I refilled 20 dollars and spent 10, then I have balance of 10 and refilled balance of 20. Card usage left is a counter that I found on the card memory. It decrements by one each time I use it.

@Willy-JL
Copy link
Member

Willy-JL commented May 26, 2024

nice, so maybe "Top-up Count", "Last Top-up", "Card Usages Left", and "Card Value" for new card, those sound good to me )

gonna get to reviewing properly tomorrow, and thanks for the files. also, the section with the checkboxes in the PR template is for the reviewer checking your code :D leave them unchecked, we will check them once we try the code

@zinongli
Copy link
Contributor Author

Cool! I just changed the wordings accordingly. Thanks and sorry about the checkboxes. This is my first time being involved in a public repo. Still have a lot to learn.

Copy link
Member

@Willy-JL Willy-JL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good to me, only need some improvements on key verification, check the individual comments. i can add the key b verification if you want, but need more info from you on the value(s) of key b. also if possible check if key is same for others in same building? having only 1 key could be limiting if its not universal

applications/main/nfc/plugins/supported_cards/csc.c Outdated Show resolved Hide resolved
applications/main/nfc/plugins/supported_cards/csc.c Outdated Show resolved Hide resolved
applications/main/nfc/plugins/supported_cards/csc.c Outdated Show resolved Hide resolved
@Willy-JL
Copy link
Member

undraft when ready

@Willy-JL Willy-JL marked this pull request as draft May 30, 2024 21:22
@zinongli
Copy link
Contributor Author

zinongli commented May 31, 2024

I completely agree about hardcoding key values affecting universal applicability. I have been learning more about the parser verification process and about how to avoid false positives & misses. From my understanding, inside the _verify function one can't access card memory. But the most robust way to verify is checking the format of the memory because they are ususally the most custom designed for the specific company. So I will move all verification steps into the _parser function and leave the task of decoding keys to the users. It's a MFC 1k anyway.

To answer your question about Key B, I don't have the value of Key B but Key A suffices reading 16/16 blocks. I tried flipper nested but it didn't support this kind of card. And I don't have a pm3 at hand so... It will be a mystery to be solved.

I will send in my commit shortly. Thanks for the review!

- Deleted key-based verification
- Added memory based checksum and backup block checks to ensure better verification performance
@zinongli zinongli marked this pull request as ready for review May 31, 2024 06:55
Added card type checks and verify for csc specific memory format
Formatted the code (indentation etc.)
@octopus8321
Copy link

I am not a developer but I greatly admire the work that all developers do. It's a great job they do

@zinongli zinongli requested a review from Willy-JL June 10, 2024 00:28
@Willy-JL
Copy link
Member

Willy-JL commented Jul 9, 2024

@zinongli sorry for the delay, i finally looked better at the implementation and it seems meaningful. i cleaned it up a little, remvoed read and verify rather than returning true. can you please verify if it still works correctly? thanks :D

@Willy-JL Willy-JL changed the title NFC parser: Added parse for CSC Service Works Reloadable Cash Card NFC: Add parser for CSC Service Works Reloadable Cash Card Jul 9, 2024
Copy link
Member

@Willy-JL Willy-JL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just waiting for confirmation of functionality

@zinongli
Copy link
Contributor Author

Works great on my end too. Ready for merge!

@Willy-JL Willy-JL merged commit 79d8b12 into Next-Flip:dev Jul 10, 2024
2 checks passed
@Willy-JL
Copy link
Member

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants