-
Notifications
You must be signed in to change notification settings - Fork 106
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
mifare_desfire_key: fix get/set_version for AES keys #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior looks odd indeed.
May I ask you add some simple unit-test that breaks without this change (and prove the code is wrong) and pass with the proposed patch? Extra point if you push the tests, Travis-CI complains that the code is broken, and then push this commit on top and Travis-CI is happy again 😉
No, but feel free to take my contribution as is. The defect is obvious. |
Is it really better to leave this bug in the code base than to take this simple fix for an obvious defect without a unit test? |
If this isn't resolved by the time I get a pull request for #77 submitted, I could also throw in a test for this as a part of that pull request. |
@darconeous please don't mix fixes in a single Pull-Request 😨 . Feel free to open a new PR that fix this issue, and reference this issue in the new Pull-Request… that way we do not forget to close one PR when merging the other one, and have a more readable code history 👍 . Thanks! |
No problem. |
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
#78 includes tests which validate this PR. |
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
Closing since #78 will be merged soon-ish! |
In addition to adding tests for the bugs addressed via nfc-tools#70, this commit also addresses a key corruption bug that would occur on 3DES keys when `mifare_desfire_key_set_version()` was called.
mifare_desfire_key_set_version incorrectly modified AES key bytes, while mifare_desfire_key_get_version returned some bits of the AES key.