Safer safelok routines

[henrygab]

Generally, some bug fixes, and some syntactic sugar (with minimal or no impact to compiled binary).

• FIX: encoding of interval (aka expiration date) was incorrect
• FIX: const correctness allows more compiler optimizations.
• Rather than uint8_t * of unknown length, functions now take pointers to structures of the appropriate size.
  • Makes it easier to properly use the functions, as they self-document not only the type, but how many bytes might be accessed through that pointer.
  • Especially useful when code is later maintained by someone other than original author.
  • May improve some static analysis tools outputs.
• Wrappers for property getters / setters
  • Allows easier single-point-of-correction.
  • Example: When eltrick determined that the card creation year field is only 7-bits, the fix was localized.
• Very basic self-test option added.
  • Framework makes it trivial to add more test cases later.
• Bit set / get routines now reject invalid values
  • Values must fit within number of bits
  • Warning/error messages are output
  • Yes, this helped catch code errors
• Adding missing output for cards that do not have variable keys.
  • No, I don't know what "variable keys" means ... I just fixed the output.

[github-actions[bot]]

You are welcome to add an entry to the CHANGELOG.md as well

[iceman1001]

Interesting, just after a quick look I can see some issue.

1. make style, your setup doesn't have waveshare meaning we loose some commands now. Please revert.
2. uid can be 4,7,10 there is nothing saying a MFC for saflok can't be a UID 7 bytes.

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.

3. assert, as long as assert doesn't "exit the client" when triggered its ok, but if not the client isn't handled in a nice way. Then you have to redo the concept.

[henrygab]

1. make style, your setup doesn't have waveshare meaning we loose some commands now. Please revert.

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!

2. uid can be 4,7,10 there is nothing saying a MFC for saflok can't be a UID 7 bytes.

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.

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.

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.

3. assert, as long as assert doesn't "exit the client" when triggered its ok, but if not the client isn't handled in a nice way. Then you have to redo the concept.

There ~~are~~ were only two assert() in the code. ~~Yes, this exits the client. These two asserts are for programming errors (not user / data errors). There is no other option in the current code's architecture to prevent either silent data loss, silent data corruption, or silently returning bad data to the caller ... which is likely to be data corruption or data loss.~~

---

Are you sure you don't want *_any_* asserts?

I agree that user errors should not be assertions. I also agree that receiving corrupt or unexpected data from a card should not cause an assertion.

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:

• the error is a code bug (e.g., setting values that don't fit in the bitfield length)
• the error in the original code base would not be handled/propagated correctly
• the error in the original code base might silently corrupt data

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...

---

[iceman1001]

Take my opinions with the perspective of having a nice user experience. The client randomly exiting is not a nice user experience.
When an error is detected, clean up as much as we can after it and let the user enter new commands in the CLI. As robust as we can.

[henrygab]

I'm open to you changing the asserts in the saflok file. Of course, there are other assert() in the client code that should also be changed, if this is the goal:

• client/src/pm3_luawrap.c
• client/src/imgutils.c
• client/src/py3_pywrap.c
• client/src/fido/cbortools.c
• common/lz4/lz4.c
• ...

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.

[henrygab]

And ... I've added some regressions in date handling.

[henrygab]

@iceman1001 -- Here's the link to the KDF algorithm prior to my changes. Note it only uses / supports a 4-byte UID.

https://github.com/STIEBELJOSHUA/proxmark3/blob/15026f7b3506b75b47831d75fa66bc25efc5ca2e/client/src/cmdhfsaflok.c#L282-L297

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).

[henrygab]

@iceman1001 -- asserts removed, manually tested both valid and invalid edge cases for expiration dates. There's still differences between hf mf info and hf saflok read for certain expiration dates, but I believe the bug lies in hf mf info. Pending completed CI checks, this is ready.