RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
4k stars 1.05k forks source link

mf_nonce_brute fails to find first bits of key #1289

Closed datatags closed 3 years ago

datatags commented 3 years ago

Describe the bug mf_nonce_brute is not able to find the first 16 bits of the key in "phase 2"

To Reproduce Steps to reproduce the behavior:

  1. CD into tools/mf_nonce_brute/ and run the second command example given by ./mf_nonce_brute
  2. When it completes, it will say "failed to find key"

Expected behavior mf_nonce_brute to find the correct key.

Screenshots mf_nonce_brute failed to find key

Desktop (please complete the following information):

(*) Q factor must be measured without tag on the antenna

[+] Displaying LF tuning graph. Divisor 88 (blue) is 134.83 kHz, 95 (red) is 125.00 kHz.


**Additional context**
The issue seems to be caused by [this line](https://github.com/RfidResearchGroup/proxmark3/blob/54153153b8e7e63eb02334cdd1b0cf4af43717f8/tools/mf_nonce_brute/mf_nonce_brute.c#L474):

key |= (count << 32);

The issue being that the upper bits of `key` are set from `count` but never un-set. Adding a debug message to the loop confirmed this, where there were many, many repeated lines like this:

... Checking key fffc... Checking key fffe... Checking key fffd... Checking key ffff... Checking key fffc... Checking key fffe... Checking key fffd... Checking key ffff... ...

where those four digits are just the top four digits of `key`. I was able to fix the issue by changing the previously mentioned line to this:

key = args->part_key | (count << 32);


which gave the expected output:
`Valid Key found [ 3b7e4fd575ad ]`

Thanks!
iceman1001 commented 3 years ago

Interesting, I thought I pushed that fix. Good finding it and good write up.

doegox commented 3 years ago

Thanks @datatags I added your fix with some others along the way, in https://github.com/RfidResearchGroup/proxmark3/commit/13ae226a601bf6745fbbc52db532acc69714c5b7

Note that it's currently in a "future" branch because of temporary freeze of master. It'll go to master as soon as @iceman is ok with it.

doegox commented 3 years ago

now merged in master