Safer safelok routines #3026
Safer safelok routines #3026
Conversation
This should have zero effect on an optimized build, as there are effectively zero changes to the instructions. Rather, this provides greater clarity as to the size of buffers being passed to various functions, which improves the long-term maintainability of code, especially when maintained by someone other than the original author.
…utines. Again, these should have zero impact on resulting instructions. At the same time, this will enable static analysis to more accurately validate a buffer (here `saflock_mfc_data_t`) points to sufficiently large allocated memory. Additional validation for `extract_bits()` and `insert_bits()` will prevent accidental corruptions (and assert, if assertions are enabled). These changes also makes the code easier to follow.
…ok formats * Small, testable functions * `set_` functions return boolean so they can indicate out-of-bounds values and similar failures. * const-correctness for all helper functions Note: The new functions are NOT used extensively yet ... they still need testing. But they should make the code more self-documenting / readable (if somewhat verbose).
run `hf saflok decode` twice ... and it will run old decoder one time, and new decoder the other time. Makes it easy to compare side-by-side in this interrim build.
|
You are welcome to add an entry to the CHANGELOG.md as well |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Saflok MIFARE Classic card handling code with a focus on type safety, const-correctness, and bug fixes. The changes introduce strongly-typed structures, comprehensive getter/setter functions for bitfields, and improved datetime handling.
Key Changes:
- Replaced raw byte arrays with typed structures (
saflok_mfc_data_t,saflok_mfc_key_t, etc.) for better type safety and self-documentation - Added comprehensive getter/setter wrappers with range validation for all card data fields
- Fixed interval (expiration date) encoding and added datetime manipulation functions
- Introduced a basic self-test framework (
hf saflok selftestcommand)
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/commands.md | Added hf saflok selftest command documentation; removed waveshare section |
| doc/commands.json | Updated command descriptions and added selftest command entry; removed waveshare commands |
| client/src/pm3line_vocabulary.h | Added selftest to vocabulary; changed bambukeys to keygen; removed waveshare commands |
| client/src/cmdhfsaflok.c | Major refactor: added typed structures, getter/setter functions, datetime handling, bounds checking, and self-test framework |
| client/src/cmdhfmf.c | Minor whitespace/formatting fix |
| client/src/cmdhf14b.c | Minor whitespace/formatting fixes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Discovered by github copilot. Verified actual bugs by a human. Fixed by a human.
|
Interesting, just after a quick look I can see some issue.
Some people love structs, as long as they fit all the scenarios it's ok. We tend to use uid[10] and uidlen in the struct to keep track of things.
|
Interesting... I cannot both test my changes and easily revert this, but as it's automated, perhaps you could simply run the automated command after the merge? I have no idea why the waveshare command isn't available for my builds. If you know why, I'd appreciate if you could share the secret!
While the MFC UID might be longer, the algorithm as used in the code I edited was limiting the UID to 4 bytes. This code replicated the original behavior, and I do not have a MFC Saflok with 7 or 10 byte UID to test/validate such an algorithmic change.
The saflok code that was checked in used only the first four bytes of the UID (even though it read more bytes from the card). I did not change the logic When this command is expanded to support >4 bytes of the UID (or UL-C, or any of a number of other extensions), adding an explicit length field for the UID makes total sense and absolutely should be done.
There Are you sure you don't want *_any_* asserts?
I agree that user errors should not be assertions. At the same time, asserts catch coder/programmer errors. Unfortunately, much of the code does not have proper error handling ... or explicitly ignores error return values. Thus, there is often no way to return an error cleanly from a deep stack, because intermediate functions will blindly ignore the error and continue, even when something went wrong. Prior to adding the validation, some of the behavior was (iirc) silent data corruption. If lucky, this would crash the client. If unlucky, it could corrupt the data on the card. The silent nature of that data corruption is not something I think should be left to occur. Therefore, what behavior would you suggest for the following situation:
In the end, you have final choice to accept or reject the PR, and can make changes to the PR (before or after merging). In other words... you have absolute choice... |
…that must cause client to exit.
|
Take my opinions with the perspective of having a nice user experience. The client randomly exiting is not a nice user experience. |
|
I'm open to you changing the asserts in the saflok file. Of course, there are other
I'm off to bed. I've fixed enough issues (esp. around interval / expiration) that I recommend merging, and then changing whatever remains that you don't like. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
And ... I've added some regressions in date handling. |
|
@iceman1001 -- Here's the link to the KDF algorithm prior to my changes. Note it only uses / supports a 4-byte UID. I simply wanted you to see that I wasn't trying to remove support for other UID sizes ... the algorithm simply didn't use / support anything else, and I don't have the knowledge on changes that would be needed for other UID sizes (or any ability to test them). |
|
@iceman1001 -- asserts removed, manually tested both valid and invalid edge cases for expiration dates. There's still differences between |
2 participants
Generally, some bug fixes, and some syntactic sugar (with minimal or no impact to compiled binary).
constcorrectness allows more compiler optimizations.uint8_t *of unknown length, functions now take pointers to structures of the appropriate size.