-
Notifications
You must be signed in to change notification settings - Fork 57
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
Script to inject key and/or auth counter directly into binary image #26
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Eliot Blennerhassett <[email protected]>
Signed-off-by: Eliot Blennerhassett <[email protected]>
Signed-off-by: Eliot Blennerhassett <[email protected]>
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.
Sorry for delay. I was traveling last week. Thank you for contribution! Please consider resolving my comments.
src/inject_key_bin.py
Outdated
ctr_blob = struct.pack("<I", args.ctr) * 256 | ||
|
||
with open(args.bin, 'r+b') as f: | ||
f.seek(args.offset + 1024) |
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.
if offset is "Offset within file to patch", why do we need to add 1024 here?
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.
args.offset is offset to start of patching when both key and ctr are patched. Yet I want to be able to patch either independently.
I made it a commandline parameter because I wasn't sure that it would be constant across all targets. Actually I think it is effectively a constant
src/inject_key_bin.py
Outdated
help='.bin file to inject keys into. Or "stdin"') | ||
parser.add_argument("--key", help="EC private key in DER format") | ||
parser.add_argument("--ctr", default=0, type=lambda x: int(x,0), help="Value of auth counter") | ||
parser.add_argument("--offset", default=0xB800, type=lambda x: int(x,0), help="Offset within file to patch") |
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.
Maybe we could guess by looking at the .bin file whether it is Tomu binary of stm32f1x? Or at least we could have two preset values one for each of them. Also I don't remember if playing with make flags can change the offset.
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.
All builds share same .ld file, so I think offsets are really constant. Maybe this argument can be removed in favour of a constant. In a pinch, a user can edit the .py to change the value?
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.
I'm wrong about all builds sharing same .ld. There is also efm32hg.ld
@@ -0,0 +1,18 @@ | |||
# Requires dfu-utils to be installed. | |||
# This runs through the steps described in https://github.com/Yubico/libu2f-host documentation |
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.
Usually I run this test suite for every change: https://github.com/google/u2f-ref-code/tree/master/u2f-tests, it is very complete. But having a simple one is a good thing as well.
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.
Ah ok I didn't know about that. I just wanted a simple 'is the key working' test that didn't involve interacting with a website...
test/yubico_test.sh
Outdated
echo "Touch key to confirm registration" | ||
u2f-host -a register -o 'https://demo.yubico.com' < regchallenge.json > regdata.json | ||
#cat regdata.json | ||
curl https://demo.yubico.com/wsapi/u2f/bind -d "username=tomu&password=test&data=$(cat regdata.json)" |
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.
can we generate random user/password here to avoid possible conflicts?
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.
Reasonable request.
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.
I've done this
curl 'https://demo.yubico.com/wsapi/u2f/enroll?username=tomu&password=test' > regchallenge.json | ||
cat regchallenge.json | ||
echo "Touch key to confirm registration" | ||
u2f-host -a register -o 'https://demo.yubico.com' < regchallenge.json > regdata.json |
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.
Shall we add .json files to .gitignore and clean them up at the end of this script?
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.
OK
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.
Now by default they are cleaned up at end, unless you add "-k" as first argument. Rather simplistic, but I think it is ok for a basic test script.
src/inject_key_bin.py
Outdated
import os | ||
|
||
parser = argparse.ArgumentParser() | ||
parser.add_argument("--bin", default="build/u2f.bin", |
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.
Can we combine inject_key and inject_key_bin into one? Or even replace inject_key with the inject_key_bin...
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.
I think once inject_key_bin is proven to work, it can replace inject_key.
also remove json output unless -k is given Note accounts are removed from demo.yubico.com after 24 hours
.gitignore
Outdated
@@ -2,3 +2,4 @@ | |||
*/.dep | |||
doc/chopstx.info | |||
.vs/ | |||
*.json |
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.
it is better to be more specific about which files to ignore. What about test/*.json
?
src/inject_key_bin.py
Outdated
help='.bin file to inject keys into. Or "stdin"') | ||
parser.add_argument("--key", help="EC private key in DER format") | ||
parser.add_argument("--ctr", default=0, type=lambda x: int(x,0), help="Value of auth counter") | ||
parser.add_argument("--offset", default=stm32f103_offset, type=lambda x: int(x,0), help="Offset within file to patch") |
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.
So, for stm32f103 offset can be 0xf400 or 0xf800 depending on build flags (CUSTOM_ATTESTATION_CERT), for efm32hg it can be 0xb400 or 0xb800. But it is more reliable to count backwards. It should always be 2048 bytes from the end of .bin file.
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.
Pushed new code that uses this calculation by default
test/yubico_test.sh
Outdated
@@ -0,0 +1,29 @@ | |||
# Basic test | |||
# Requires dfu-utils to be installed. |
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.
libu2f-host instead of dfu-utils?
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.
Package "u2f-host" on debian. Pushed this change
Any update on this? |
Signed-off-by: Eliot Blennerhassett <[email protected]>
Default is 0x800 (2048) bytes from the end of file Signed-off-by: Eliot Blennerhassett <[email protected]>
Signed-off-by: Eliot Blennerhassett <[email protected]>
Signed-off-by: Eliot Blennerhassett <[email protected]>
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.
Thank you for working on this! I am thinking about adding basic test functionality into certtool
which would not require installing of u2f-host and calling the yubico web site. I find inject_key_bin
useful and I would like it to replace inject_key
. Would you mind to submit just the inject_key_bin
part and replace existing inject_key
with it? I can do it myself if you have no time/not wiling to do it with giving you credit of course.
Co-Authored-By: eliotb <[email protected]>
Signed-off-by: Eliot Blennerhassett <[email protected]>
#25
I have changed the parameters slightly:
The --key must be given explicitly or key in .bin is not modified
The --ctr must be given (and non-zero) or counter are in .bin is not modified.
Thus it is possible to change just the key or the counter independently.
I have confirmed that this script produces the same modifications as the original inject_key.py