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

Picopass nr mac improvements #108

Merged
merged 3 commits into from
Jan 20, 2024
Merged

Picopass nr mac improvements #108

merged 3 commits into from
Jan 20, 2024

Conversation

bettse
Copy link
Collaborator

@bettse bettse commented Jan 13, 2024

What's new

  • Trying to allow saving of non-SE NR-MAC auth'd cards
  • Also tries to correct a bad key check (pacs->key === 0 is a bad idea since that could be a valid key)

Verification

  • use NR-MAC to auth a non-SE card, see a regular "save" option

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

@bettse
Copy link
Collaborator Author

bettse commented Jan 13, 2024

@nvx @pcunning could you tell me what you think and try this against cards you have? The code looks really different, but I'm trying to make the various save options easier to understand.

@nvx
Copy link
Contributor

nvx commented Jan 13, 2024

Note the config block can always be read so using that as a sentinel for "did we succeed to read a secured chip" may not work. The more I think about it the more storing flags for which blocks were successfully read makes sense, or at least a flag for "successfully read AA1" and "successfully read AA2" (speaking of we should really rename AA1 in the structure to just data since AA1 only starts after the AIA technically)

Edit: and a flag for if we have each key

@bettse
Copy link
Collaborator Author

bettse commented Jan 13, 2024

Note the config block can always be read

This is checking the pacs config though, the first block of AA1 (right??)

The more I think about it the more storing flags for which blocks were successfully read makes sense, or at least a flag for "successfully read AA1" and "successfully read AA2"

Yeah, this was initially written before we'd talked about things and was supposed to be a simple clean up. I do think a bit-vector of read-blocks and/or auth method or known keys would be better and the direction we should go.

(speaking of we should really rename AA1 in the structure to just data since AA1 only starts after the AIA technically)

oh god, yeah

@nvx
Copy link
Contributor

nvx commented Jan 15, 2024

Note the config block can always be read

This is checking the pacs config though, the first block of AA1 (right??)

oh herp, I misread that as the picopass config block not the PACS config block, nevermind

@bettse bettse marked this pull request as ready for review January 15, 2024 15:55
@bettse
Copy link
Collaborator Author

bettse commented Jan 18, 2024

This is ready for review: I can't review it since I opened it

@skotopes
Copy link
Member

I'll merge it later today

@skotopes skotopes merged commit a3c5896 into flipperdevices:dev Jan 20, 2024
2 checks passed
@bettse bettse deleted the picopass_nr_mac_improvements branch January 20, 2024 04:43
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

Successfully merging this pull request may close these issues.

3 participants