RfidResearchGroup / proxmark3

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

Add T5577 downlink modes #282

Closed mwalker33 closed 5 years ago

mwalker33 commented 5 years ago

Is your feature request related to a problem? Please describe. No

Describe the solution you'd like I am adding the T5577 downlink modes (as i did for the proxmark3 repo.)

Describe alternatives you've considered None, as the downlink modes need to be added to provide the feature.

Additional context

As pre a request from Iceman, I am adding the downlink modes as I did for the Proxmark/proxmark3 repo. More then happy to do the work and will submit the PR when ready.

I have opened this thread for a place I can ask some questions as needed to help me port over the changes.

I have 2 questions to start with.

  1. In the cmdlft55xx.c file (client) the AquireData function uses

    struct p {
        uint32_t password;
        uint8_t  blockno;
        uint8_t  page;
        bool     pwdmode;
    } PACKED;
    struct p payload;

    I need to add the argument to ID the downlink mode requested (to pass to the pm3). Can I just extend that struct ? i.e. I have done it and it works, so checking if I need to check something else as well (size?).

  2. In the lfops.c file, there is the option to allow the t55xx config to the saved to flash. As it needs to erase part of the flash to re-write, I assume the allocated space is just for the T55xx config/timeings? How big is that space ?
    At the moment I am just saving the default mode settings (i.e. not changed), but since It will now have 4 sets of timings (one for each downlink mode) and if there is space, it would make sense to save those as well.

Thanks

doegox commented 5 years ago

Hi, yes you can just extend the struct, you get up to 512b for the whole struct. And of course adapt the corresponding struct in appmain.c CMD_T55XX_READ_BLOCK handler Flash: t55xx settings are stored in page 3 sector 12 ,so 0x3C000. So far, you've an entire sector (4k), so feel free. We'll see later how things will be adapted if it becomes part of the future filesystem.

mwalker33 commented 5 years ago

thanks

mwalker33 commented 5 years ago

I am trying to test the (firmware) T55xx_ChkPwds() which should try passwords from flash. It enters the flash section and completes a flash read

  isok = Flash_ReadData(DEFAULT_T55XX_KEYS_OFFSET, counter, sizeof(counter));
    if (isok != sizeof(counter))
        goto OUT;

    pwdCount = counter[1] << 8 | counter[0];

    if (pwdCount == 0 || pwdCount == 0xFFFF)
        goto OUT;

a debug shows pwd == 0xffff so will exit. I assume this is due to no passwords on flash.

How do we load passwords into flash so I can test ? Thanks.

doegox commented 5 years ago

See https://github.com/RfidResearchGroup/proxmark3/blob/master/doc/md/Use_of_Proxmark/2_Configuration-and-Verification.md

mwalker33 commented 5 years ago

thanks, missed that. I got it checking the passwords from the list in flash memory, so that bit is ok. Is this meant to work ? I get a different candidate every time (and I know the password is in there), so seems very hit and miss.

I tried this with the current version of proxmark3-rrg (i.e. pre my changes) and same random results. lf t55 chk i d:\pm3\t55_pwd - works every time. lf t55 chk m - very random results (but faster).

edit: I reset block 0 back to default : 000880F0 and it then seems to work every time. when set for an em4100 tag : 00148050 is when I had the above issue. (so less my changes and more its evaluation of based on the data/encoding) .

doegox commented 5 years ago

Yeah there are some issues in some of the modes, see https://github.com/RfidResearchGroup/proxmark3/issues/149

mwalker33 commented 5 years ago

I am at the point of having most of the downlink modes support on all needed functions roughed in. A quick background for those not aware of this. The T5577 supports 4 modes of sending data from the reader to the card. Newer cloners (for example) can set these to hide the t5577 from the reader, one I have seen set it to leading 0. To add this means I needed to touch nearly every T5577 function, so lots of change. Up to this point I have all changes working 100% with the old calls. Just adding an extra bit, that if not called will default to the old way.

While I need to complete testing and code cleanup; based on the default repo settings, the fullimage.elf has gone from 234,212 to 234,828 so about a 616 byte increase.

I have some ideas that may reduce this size increase but may have a flow on effect.

What i noticed is some calls between the client and the firmware uses a "ng" method. It looks like this is more a struct that can be simply cast. to/from the byte array for transfer. e.g.

// uses NG format
void T55xxWriteBlock(uint8_t *data) {
    t55xx_write_block_t *c = (t55xx_write_block_t *)data;
    T55xxWriteBlockExt(c->data, c->blockno, c->pwd, c->flags);
    // reply_ng(CMD_T55XX_WRITE_BLOCK, PM3_SUCCESS, NULL, 0);
}

And this function is really just a place holder to convert and call the T55xxWriteBlockExt, so to alter the Ext function would mean you could do away with this one (which should save a little space) Also we might get a bit of space back by passing in and using a struct rather then extracting out to local variables.

Question Is this something we would like to do i.e. change all calls to use a more common struct "ng" method ? Pro : simple way to pass things in (a pointer to a stuct) so data only stored once, so should be smaller. Con: function call parameters change so 3rd party calls may need to be updated (but letting any future changes happen more transparently)

mwalker33 commented 5 years ago

Just a quick update: I spent today playing with different setups to pass date between functions. Most used more space not less.

doegox commented 5 years ago

If you think code can improve by passing some structs around, why not. But in this specific case, it seems it would make more complex the second usage of T55xxWriteBlockExt, within WriteT55xx loop.

mwalker33 commented 5 years ago

After testing, I agree as is was better. My view was that if the code was smaller and fully functional then complexity is ok (i.e. the more features we can fit on chip the better) . I did clean up a bit so that the read/write and read_ex/write_ex where merged. Since the hard work was moved to the send_cmd function, it was cleaner to only have the one read/write functions. Passed all tests my end.

PR submitted for review and comments.

iceman1001 commented 5 years ago

Lets see, since I am on a holiday I have a hard time keeping up.

  1. the bruteforce is different on device. I tried a faster detection for ASK configured tags, which samples the signal and get a average amplitude. For each pwd try it collects and see if the new amplitude is much larger than the baseline amplitude. aka found. or tag is responding.

Havent solved it for FSK/PSK yet.

  1. An Iceman based repo is different. You will have to compile each standalone modes for LF when you try refactoring this. Removing functions usually is a bad thing, all of the functions usually has a purpose. You will need to find every single instance of usage and understand their functions in order to remove stuff.

  2. The version you dropped in to official repo is nice, but I would prefer if you made the swapping protocols more seamless on device side. Letting the client have to try 4 different protocols automatic is doable there, but for standalone modes its just an extra complexity. Auto mode on device side would be preferable.

  3. size? When it comes to size , dont be too restrict by it. The flash mem has a large portion reserved to configurations. There will also be EM4305 configurations and more to come. Dont mico optimize it. the only restriction is our precious RAM.
    which leads me to

  4. All t55xx modes should implement NG format. Feel free to do so. All communications should use NG format. Its one of our bullits on the public roadmap.

mwalker33 commented 5 years ago

(3) Auto mode on device side would be preferable. On the client it relies on the acquire demodulation to be valid when deciding if the last mode tried is valid or not.

So, thinking about this, on chip side, are you thinking something like For a read/write

Is there a t55 detect code somewhere, or is this some thing we need to port up to the firmware?

iceman1001 commented 5 years ago

yeah, this is where I got stuck when I saw your PR on offical. Just letting the client do it was easy, but with all standalone mode supports etc, it became more complex and I decided to wait until offical repo PR was done :) How do you detect a good response today? You run the aquiredata and detect on client.
That isnt very good one but it does the trick. with password its easier, you can do the amplitude trick.

Write doesn't depend on modulation. So that is just a question of sending four writes. All kinds of read is the harder part since they can be anything. If we do detect on client, if found or manually configured, we could upload that to the device in a configuration. That would be use for all calls until change.

but the hard nut is detection. That would be great to find on device side.
Now that we have 256+ fullimages available its doable again.

mwalker33 commented 5 years ago

One of the big challenges I see with "blind" use, is you need good verification/validation to be sure you have the correct and current setting (for the card on the reader). e.g. if i run a lf detect r 4 and find the mode, then "save" (to the reader), the assumption is the user will re-run the detect if they change cards. One of my ideas was save the last mode used by the client or lf detect, and what every mode was found/used, use that for all calls (as the default) unless over ruled by the client command line. But all still relies on the user.

I agree that to auto detect and use is better, it will just take time to port the code over.

Note: I put a lot more into this one for the client, i.e. I beleive every command that could use it is supported. I started out with a "recovery" idea. i.e. just enough to recover my card. Here its more a use the mode of choice.

How do we want to proceed ? I see two ways forward.

  1. Push though the client side support as is. then re-work it if/when the device side is working.
  2. Withdraw the pull request unti we get the device side working.
iceman1001 commented 5 years ago

As mentioned before, I say merge and we find a solution for device side in next PR from you ;) Better to have functions meanwhile trying to improve.

mwalker33 commented 5 years ago

I have completed a full resync from the master with all new changes, so will run a full round of testing to double check everything with all the changes. Quick question: Does the flasher/boot loader support the 512K now ? I not the base image is 254,944 fullimage.elf Thanks

doegox commented 5 years ago

yes the flasher supports 512k since a couple of days ago and the current RDV4 is larger than 256k since this evening :)

mwalker33 commented 5 years ago

I have started working on a cutdown version of the tryDetectModulation (for standalone). This may take a while, but a good start with it detecting the clock rate from the buffer. I note that for it to work well, we need some memory to hold the adjusted samples. i.e. if we do an acquire for 12000 samples, then we need a buffer of that to work on. Given that bigbuffer is 40000 and if the acquire is 12000, what's the safe way to grab 12000 for temp use? can I just grab the address of bigbuffer and offset by ? e.g. In the current code it uses: uint8_t GraphBuffer = BigBuf_get_addr(); so if the sample we are working on was 12000, can I just say uint8_t WorkingBuffer = BigBuf_get_addr()+12000; (and ensure we don't read past the 40000 mark)

doegox commented 5 years ago

@iceman1001 you know more than I on that matter

mwalker33 commented 5 years ago

yes the flasher supports 512k since a couple of days ago and the current RDV4 is larger than 256k since this evening :)

Thanks, yep downloaded and updated. for comment: I did the os flash first, then went to do the boot loader and it safely failed. So I had to revert to the old flash, then update the bootloader (via the old flasher) then moved forwards. All good and working.

doegox commented 5 years ago

Hmm in principle, you can do everything with the new flasher:

If it was not the case, can you describe exactly what happened?

mwalker33 commented 5 years ago

Hmm in principle, you can do everything with the new flasher:

  • you try the new image, the flasher tells you in RED to first reflash the bootloader (and refuses to flash if image > 256k)
  • flash the new bootloader with the new flasher and old bootloader
  • flash the new image with the new flasher and new bootloader

If it was not the case, can you describe exactly what happened?

My bad, I was wrong. I read the message and missed the fault. msg: !!] Note: Your bootloader does not understand the new CMD_BL_VERSION command BUT the fault was my end, i had b and not -b (so actual issue was no file found).

doegox commented 5 years ago

ok good :)

doegox commented 5 years ago

@mwalker33 could you check that the changes didn't break anything in the LF standalone modes ? You can compile them easily with

make STANDALONE=LF_SAMYRUN

etc. Thanks!

mwalker33 commented 5 years ago

Happy to test. Where do I find a list of all the standalone modes ? make clean make STANDALONE=LF_SAMYRUN compiles without errors.

mwalker33 commented 5 years ago

This is the list I got from the Standalone folder. hf_bog
hf_colin
hf_young hf_mattyrun
lf_hidbrute
lf_icerun
lf_proxbrute lf_samyrun

All compiled ok, no errors or warnings.

doegox commented 5 years ago

thanks, I hope nothing broke in terms of functionalities ;)

iceman1001 commented 5 years ago

yup, the LF standalones has a tendency to use t55xx write to clone a card. These need to be checked if you modified the lfops.c file..

mwalker33 commented 5 years ago

I had a look through the code the of the lf_ standalone modes. lf_icerun - No calls, template. lf_samyrun HID Clone (read and write to new card) - Working (tested) via stand alone Command called : CopyHIDtoT55x7(0, high[selected], low[selected], 0); lf_hidbrute - same command used lf_proxbrute - same command used

I could not see any other calls that would be affected. Please let me know if their is anything else I need to look at and test.

mwalker33 commented 5 years ago

On a second not. I not that if there is no custom timings saved it is all good. If you already have timings saved, this will be an issue (as is). as I check if the "first" value is 0xffff. Short fix here (for users) will be to erase the timings (command provided in client) lf t55 deviceconfig z reboot, defaults will load then re-customize as needed

Now that we have a bit more space, there room to do this better.

How about I add a check for EACH timing set and only update/come into ram if it looks valid, else stick with the compiled defaults.

Thoughts ?

mwalker33 commented 5 years ago

Just a quick update on standalone modulation detection.

I will stress this up front.... my test code is a very long way off being clean and stable. And somethings just seem wrong :) I have been focused on the can we get it to work side.

for RF/64, not inverted, no ST it got every detection correct for every modulation type. I did notice that FSK1a, my code got it, while the pm3 client did not (offset was 33 not 32).

For reference what I did was copy the tryDetectModulation into an "include file" (yes not good, but I wanted to keep 100% out of my code base and not modify any compiler/linker settings. :) ) Then worked though it, copying code as needed from the other files. I then stripped and adjusted to read from bigbuffer with some other tweaks i needed to get it to function.

the interesting bit around the FSK1a was bug (now a feature, maybe, late night coding).

in the getFromGraphBuf function , the original code looked like

       if (GraphBuffer[i] > 127) GraphBuffer[i] = 127;
       if (GraphBuffer[i] < -127) GraphBuffer[i] = -127;
       buff[i] = (uint8_t)(GraphBuffer[i] + 128);

which I re-wrote to

         if (GraphBuffer[i] >  127) buff[i] = (uint8_t)(127 + 128); 
         if (GraphBuffer[i] < -127) buff[i] = (uint8_t)(-127 + 128);

This was a bug, but worked: The bit missing was whatever was in memory would have been kept when neither of the two if statements had a hit. e..g GraphBuffer[i] >= -127 and <= 127 Also being that everything i was doing was a uint8_t, no negative numbers,

I then added the bit of code to set the data = sample and then adjusted. At this point everything but NRZ was working for initial test runs.

So after some work to track down the issue with NRZ if found this fixed the issue, so all test then worked.

current code with the NRZ adjustment. i.e. for all but NRZ "Square the data" set to 0 unless > 127

size_t getFromGraphBuf(uint8_t *buff,bool SquareData) {
    if (buff == NULL) return 0; 
    uint8_t *GraphBuffer = BigBuf_get_addr();
    size_t i;

    for (i = 0; i < GraphTraceLen; ++i) {
        if (SquareData)   buff[i] = 0;  // Seems to work better for all but NRZ
    else                buff[i] =  GraphBuffer[i]; // Needed for NRZ;
        if (GraphBuffer[i] >  127) buff[i] = (uint8_t)(127 + 128); 
        if (GraphBuffer[i] < -127) buff[i] = (uint8_t)(-127 + 128); 
 }
 return i;
}

Now to get my head around why it worked, as it just sees wrong.

iceman1001 commented 5 years ago

I think you missed the instructions for first use on your RDV4.. https://github.com/RfidResearchGroup/proxmark3/blob/master/doc/md/Use_of_Proxmark/2_Configuration-and-Verification.md

It will set the t55xx device configuration. There is no need to "wipe" the device configuration, never should really. It should always in last case fall back to our default settings. Looks a bit strange maybe, both defines and flash config setting saved. But its there to allow you to set a config, run in standalone and it should still be dynamic.


next steps with your graphbuff ... your conversions seems wrong. Graphbuffer is INT32, but we limit the values to be between 127 and -127 in order to easiliy convert to bigbuffer which is UINT8. In order to have support for older signal trace files which uses different INT size, we should convert those down to 127 - -127 range when loading it.
A tracefile today uses INT8.

You seem also to mix names like graphbuffer == bigbuff, which is just plain wrong. If its like this in current code before your test, it should be renamed. Its just adds to the confusing. Graphbuffer exists on clientside, BigBuffer exists on Deviceside.

iceman1001 commented 5 years ago

This however doesnt exclude the existence of bugs in the different places of FSK / NRZ

mwalker33 commented 5 years ago

With the timings I set the defaults as the base. I extended it to hold 4 sets of values. But his could be a problem. as the "read gap" moved. When loading from flash it will only use the defaults IF the flash was "new" (hence why it worked for me every time, as I had not saved anything).

I think I need to change this. This is what I suggest pending comments.

I revert the device config options to for the SAME order as before so Write_0, Write_1 ReadGap then to extend for the new option Write_0 Write_1 ReadGap -Write_2 and Write_3 I made it the other way as it made sense (and grouped all the timings), but I think for less issues with existing saved settings, I update as to just extend.

Thoughts ?

mwalker33 commented 5 years ago

I have complete the updates to me dev and I think it will be better for existing users. I have complete the testings and it now works more transparent. I used my 2nd RDV4 (old firmware) and set and saved the timings to flash. I then upgraded this to the latest version, and the old timings were kept and worked as expected. The new times were invalid (as not yet set), but no harm. Then I could set the new timings with. Orig: lf t55xx deviceconfig a 29 b 17 c 15 d 50 e 15 p New: lf t55xx deviceconfig r 1 a 31 b 20 c 18 d 50 e 15 p lf t55xx deviceconfig r 2 a 31 b 20 c 18 d 40 e 15 p lf t55xx deviceconfig r 3 a 29 b 17 c 15 d 31 e 15 f 47 g 63 p

Once done, all the new timings were saved and working.

So if you are happy, let me know and I will put in the PR. Thanks for you patience and Sorry for the change.

doegox commented 5 years ago

Sounds good for PR :) Don't forget to update doc/md/Use_of_Proxmark/2_Configuration-and-Verification.md. I've the feeling the inline help of all these t55xx modes will be a bit too short, wouldn't it be worth to write a specific doc about it? It's not like everybody is reading the datasheet... Sth like doc/t55xx_notes.md ?

mwalker33 commented 5 years ago

Yeah its bigger then you think. After seeing a lot of people struggle as they start, I thought the t5577 was a good place to start, its more about the process then the commands. So I started on an introduction to the t5577, its still growing.

doegox commented 5 years ago

Great ! BTW maybe in hw status you can detect FFFF and replace 8191*8 (65535) by unconfigured ;)

mwalker33 commented 5 years ago

I will work on that. I did think about that in the original, then I got a little too focused on image size. Now that we have crossed that, we have a bit of room to play. As a quick "help the user" I added the z option to the hardware. it writes back ALL FF to the timing flash area. And THAT is detected as "no settings saved", which will apply the defaults. With this request I was trying to fix a potential customer issue. i.e. now (with the pr) the old settings will work, just cant get the new "downlink" modes to work. since its new nothing broken :) just run the commands needed to add.

iceman1001 commented 5 years ago

I added the output for HW STATUS to print unconfigured. Which is still not quite true, since there should be hardcoded fallback values.
I also change the upper case for defines and pluralis for arrays coding style.

What more is it to this issue now?

mwalker33 commented 5 years ago

Pending any issue, I think this bit is done. Happy to close if no one else has any questions.

iceman1001 commented 5 years ago

So the r 0 ( fixed length dl mode) has been adapted to take in consideration for the slow power disipation of rdv4 antenna. Usually 1-2 fc, Hence 29 instead of datasheet 31. These values was tested by @TomHarkness thoroughly,

pm3 --> lf t55xx deviceconfig a 29 b 17 c 15 d 47 e 15 p
pm3 --> lf t55xx deviceconfig r 1 a 31 b 20 c 18 d 50 e 15 p
pm3 --> lf t55xx deviceconfig r 2 a 31 b 20 c 18 d 40 e 15 p
pm3 --> lf t55xx deviceconfig r 3 a 29 b 17 c 15 d 31 e 15 f 47 g 63 p
mwalker33 commented 5 years ago

the r 0 should be what was there for the recommended default. the reset was what I had ported over. The worked for me, but if not good can be adjusted. Most of the other modes will have set values based on the 'Write_0' but left them open to change so people could test different things. If you feel they need adjusting let me know. I can to the math to make the all based on Write_0 29

iceman1001 commented 5 years ago

if the variables is set and all cals using the var Write_0 then which value it has shouldn't affect. Setting the default hardcoded values, and these recommendations in docs, we should be fine.

I am wait for @TomHarkness to show up and test things :)

mwalker33 commented 5 years ago

I noticed after some tidy up works and reformatting two small things.

  1. The lf t55 detect and lf t55 chk both support an r 4 option. The 4, on these two options will try ALL 4 downlink modes to find a match. This still works, but is now missing from the help.

  2. In my original output, I had the (r x) comment in the in the output. e.g. If you run the lf t55 detect r 4 and it found a card using downlink mode 1 of 4, it would let you know to use r 3 in the output , so they would know what option to add for other commands.

While happy to go with what ever you think is fair, I did feel it was more user friendly to let the user know the option to use, rather then referring back to the help for the command.

iceman1001 commented 5 years ago

r 4, hm... r 0 1 2 3 denotes a single mode, while r 4 denots an autodetect mode.
I am not in favour of mixing meaning of command parameters like that.
Maybe even lf t55xx detect could have it as the default mode. and maybe lf t55xx chk could have an option a or d as in autodetect aswell. However lf t55xx detect should set a config in the client, so first you always run detect, then you run chk. Chk should use that setting previously found by detect. .. That is how I would like to have it.

Looking forward to your PR

iceman1001 commented 5 years ago

btw, time to close this issue?

mwalker33 commented 5 years ago

OK. Leave it with me. I wanted to move the detect to "auto" and update the t55 config record with the mode found, so happy that is a better way forward. I will move the "user all modes" option in the chk to a letter as suggested. Side note: since the chk is about finding the correct password and you cant detect without that password, the detect would not know, so the chk would not know what mode to use. I will close this and we can open a new one if needed (of move the discussion to the PR when ready. Thanks

iceman1001 commented 5 years ago

hm,.. lf t55xx chk true. very true. I would say test R0 / R1 in chk by default? since it seems to be better hit-ratio with those two modes.