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

Remove broken --par option from lf em 4x70 #2376

Open
henrygab opened this issue May 3, 2024 · 17 comments
Open

Remove broken --par option from lf em 4x70 #2376

henrygab opened this issue May 3, 2024 · 17 comments

Comments

@henrygab
Copy link
Contributor

henrygab commented May 3, 2024

Current usage

There are two variables used for parity:

  • command_parity - global variable, client sets false by default
  • with_parity - Parameter to em4x70_send_nibble() function

All seven entry points in ARM source set global variable command_parity per input etd->parity.

Client source defaults this value to false in all cases.

Therefore, unless explicitly provided via --par for a command, the value in the ARM source of command_parity will always be false.

ARM firmware reliance on command_parity

Details

The value of this variable is only read in two locations:

  • send_command_and_read()
  • em4x70_send_nibble()

send_command_and_read()

This function calls em4x70_send_nibble() with the provided
command, setting the function parameter with_parity equal
to the global variable command_parity, and then immediately
listens for a tag response.

em4x70_send_nibble()

This function's parameter with_parity does DIFFERENT things, using both parameter with_parity and the global variable command_parity.

When global command_parity is false, all four bits of the nibble are sent. This is the default behavior with the client code.

When global command_parity is true, only the three least significant bits of the nibble are sent. (Mask 0x07).

When parameter with_parity is true, and extra bit is sent, which is the parity for the 3-bits (command_parity == true) or 4-bits (command_parity == false) that was sent.

When parameter with_parity is false, no additional parity bit is sent.

# command_parity with_parity nibble bits parity bits total bits
0 false false 4 0 4
1 false true 4 1 5
2 true false 3 0 3
3 true true 3 1 4

Callers of em4x70_send_nibble()

This function is called by the following, with "Valid" indicating the code appears valid if command_parity is also set to true.

caller with_parity Valid Sends
em4x70_send_word() true N five-bits for each row of 4x4 data
em4x70_send_word() false N parity row for the 4x4 data
authenticate() true Y the command
authenticate() false N last four bits of frn
send_pin() true Y the command
write() true Y the command
write() true ? the address for the write
send_command_and_read() command_parity Y the command

In particular, if command_parity is true, then for each nybble of the
word of data, em4x70_send_word() will only send the three least significant
bits (+ two parity bits ... thus losing data). Then, when sending the parity
row, it will ALSO lost the most significant bit (and send a parity bit + zero bit).

In addition, the authenticate() command will similarly lose data from
the last four bits of frn.

Finally, the write() command will also lose data for much the same reason.

Conclusion

The --par option is fundamentally broken, and thus is "dead code" as it
fails to work in any situation where data is sent to the tag.

Therefore, removal of the --par option and related dead code will improve
the codebase.

@henrygab
Copy link
Contributor Author

henrygab commented May 5, 2024

The only unaffected ARM entry points are em4x70_info() and em4x70_unlock().

What gets corrupted

if command_parity (set by client --par option) is set, then
em4x70_send_nibble() will only send the three least significant bits.
Thus, ANY call to em4x70_send_nibble() where all four bits contain
data intended to reach the tag will result in corrupted data being sent.

Notably, em4x70_send_byte() unconditionally sends eight bits.

Arm Entry What Corrupted Info
em4x70_info() N/A OK because only reads data from tag
em4x70_write() block data + parity row

proxmark3/armsrc/em4x70.c

Lines 287 to 292 in 2bc7c50

for (int i = 0; i < 4; i++) {
em4x70_send_nibble(nibbles[i], true);
}
// send column parities (4 bit)
em4x70_send_nibble(nibbles[0] ^ nibbles[1] ^ nibbles[2] ^ nibbles[3], false);
em4x70_unlock() N/A OK because only uses em4x70_send_byte() for data
em4x70_auth() last four bits of frn
em4x70_send_nibble((frnd[3] >> 4) & 0xf, false);
em4x70_brute() last four bits of frn
em4x70_send_nibble((frnd[3] >> 4) & 0xf, false);
em4x70_write_pin() pin (!!!)

proxmark3/armsrc/em4x70.c

Lines 868 to 870 in 2bc7c50

// Write new PIN
if ((write((etd->pin) & 0xFFFF, EM4X70_PIN_WORD_UPPER) == PM3_SUCCESS) &&
(write((etd->pin >> 16) & 0xFFFF, EM4X70_PIN_WORD_LOWER) == PM3_SUCCESS)) {
em4x70_write_key() key (!!!)

proxmark3/armsrc/em4x70.c

Lines 482 to 483 in 2bc7c50

// send data word
em4x70_send_word(word);

@iceman1001
Copy link
Collaborator

Great write-up and findings!

Is there a way to do a regression test for these things? So we can capture it with the pm3_test.sh script?

@henrygab
Copy link
Contributor Author

A regression test is a good idea. At present, I don't see where em4x70 could have much testing without a tag present? Or, maybe I'm not understanding the regression test yet.

@iceman1001
Copy link
Collaborator

if we have some dumps, we can do some tests to make sure the offline commands works.
if crypto involved, we can add a self test for it.

@henrygab
Copy link
Contributor Author

henrygab commented May 15, 2024

Ok. All current client commands require a tag.

Ah... lf em 4x70 recover is available without a tag. Ok.

Maybe also add a command to directly expose calculation of challenge / response?

@henrygab
Copy link
Contributor Author

... and added lf em 4x70 calc to allow calculating challenge (frn) and response (grn) for a given key + rnd combination
... and added tests to tools\pm3_tests.sh ... just waiting for GitHub Actions to build successfully before pushing that commit, and checking its result.

Should be ready to merge within a few hours, if all goes well.

@iceman1001
Copy link
Collaborator

Nice, some heads up,

We use two different styles when adding a self test function.

  1. Parameter --test
  2. Command lf em 4070 test

which both should give a outcome line like:

PrintAndLogEx(SUCCESS, "------------------- Selftest %s", (testresult == NUM_OF_TEST) ? _GREEN_("ok") : _RED_("fail"));

If added like this we can now easily add a run of it in the tools/pm3_tests.sh script

@henrygab
Copy link
Contributor Author

I will leave the PR as-is, as it is functionally correct, mirrors existing tests, is validated to work, and improves the validation as per your request.

The PR modeled the added tests based on those already in tools\pm3_tests.sh. I don't think there's significant value in rewriting the tests in PR #2385 to be embedded in the client source code (translating to C). Of course, if you disagree, please feel free to do that. There are higher-value changes to be made.

higher-value work

Some items that will provide greater value and are on my radar:

  1. Fix ARM code for lf em 4x70. Code never worked on two of my three PM3 Easy devices. Recently, I discovered one potential cause ... various PM3 Easy devices have a (slow) sawtooth DC bias in the traces. This may cause problems because the em4x70 codebase has hard-coded noise threshold.
  2. Add logging for lf em 4x70. As part of this, update ARM code to generate entire bitstream first, then send the pre-generated bitstream (rather than calculating + sending as one operation).
  3. Add decoder for lf em 4x70 sniffs. Manually decoding analog traces is still not a great experience. Existing data plot commands have issues. One example is when clock should be extracted from signal, and shifts during a long trace. Or, when sniffed trace loses samples, this accumulates over time, causing clock drift. Even if the decoder just spits out results on the console (not modifying plot window), that will still save hours of time manually decoding.

@henrygab
Copy link
Contributor Author

henrygab commented May 16, 2024

Update: After adding some logging to more easily see what bits are sent/received, it confirmed that ... this code is messy.

The following is what currently occurs (not using the --par option):

  • The three commands that read ID, UM1, and UM1 would send the command as a simple four-bit value ... no parity added.
  • The three commands that exchange data with the tag were hard-coded to always send a fifth parity bit after sending the four-bit command value.

Preliminary logs and notes are my em4x70_dev branch .

Todo:

  • Check what variations of commands (3 or bit) + parity bit (added or not) work for each command
  • Determine possible bug in logging of last four bits of authentication FRN Log too small, auth needed 98 bits
  • Simplify code (pre-generate bitstream to be sent)
  • Integrate and use the existing logging system (vs. my current hack)
  • Acquire at least one tag that would use the --par option to the commands?

@iceman1001
Copy link
Collaborator

@cmolson was the one who created the commands, maybe he has some input ?

@cmolson
Copy link
Contributor

cmolson commented May 16, 2024

Hi, Thanks for all the investigation, and sorry about the mess/confusion.

The tag I was looking into (ID48?) was a slightly modified version of the EM4170 which is why I tried to make this code work for both. I think the differences were around sending parity with the commands.

I still have all my tags and proxmark device, I need to get it set up again and re-familiarize myself with this to offer any useful input. I should be able to do this over the next few days.

@henrygab
Copy link
Contributor Author

Oh, nothing to apologize for; Quite the opposite. It's only because of your foundational work that I made progress myself ... so thank you for making this possible!

I have ID48 tags from my old vehicle, and XT27A tags that can be configured to ID48. What I've yet to acquire is any tag that requires use of the --par option.

If you're going to poke and prod, consider using my dev branch ... It has a functional trace built-in that will show all bits sent, and all bits received (+start/stop timing for each).

Or, if you have any extra V4070 tags (or an actual EM4170 tag ... both hinted as working differently), I'd be happy to do the poking and prodding, if you're open to loaning those out.

@cmolson
Copy link
Contributor

cmolson commented May 16, 2024

I would be more than happy to send you a few of the various tags I bought. I will also try your branch with the tags I have, thanks!

@iceman1001
Copy link
Collaborator

.... don't be a stranger, if you have some spares :)

@cmolson
Copy link
Contributor

cmolson commented May 16, 2024

I need to figure out what exactly I have.. but I have two bags with 10 tags each. One is "ID48" and the other says "ID48 EM4170"... I only want to keep 2 of each. Let me know how to get your mailing addresses and I'll send you both some.

@iceman1001
Copy link
Collaborator

if you on the discord server, do DM

@henrygab
Copy link
Contributor Author

Let me know how to get your mailing addresses and I'll send you both some.

Ping me on Iceman's RFID discord server? I have a hunch what your discord id is, but not conclusive.

(Mine is obvious. 🙂)

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

No branches or pull requests

3 participants