Yubico / yubikey-personalization

YubiKey Personalization cross-platform library and tool
https://developers.yubico.com/yubikey-personalization/
BSD 2-Clause "Simplified" License
299 stars 82 forks source link

Validate the length field we get from the hardware before using it #120

Closed nevun closed 5 years ago

nevun commented 5 years ago

In case of bad hardware, MITM, the length field we get from the yubikey should be sanity checked before use.

nevun commented 5 years ago

Below is a diff for a test to test this. First the test diff, then the output before the patch and then the output after the patch. One prints the entire stack and the other aborts like it should.

diff --git a/tests/test_yk_utilities.c b/tests/test_yk_utilities.c
index a8a8ada..e2bcc29 100644
--- a/tests/test_yk_utilities.c
+++ b/tests/test_yk_utilities.c
@@ -38,6 +38,19 @@
 #include <ykcore.h>
 #include <ykdef.h>

+static void report_yk_error(void)
+{
+       if (yk_errno) {
+               if (yk_errno == YK_EUSBERR) {
+                       fprintf(stderr, "USB error: %s\n",
+                               yk_usb_strerror());
+               } else {
+                       fprintf(stderr, "Yubikey core error: %s\n",
+                               yk_strerror(yk_errno));
+               }
+       }
+}
+
 struct versions {
        int major;
        int minor;
@@ -103,10 +116,52 @@ static void _test_yk_firmware(void)
        }
 }

+static void _test_yk_get_capabilities(void)
+{
+       YK_KEY *yk = 0;
+       bool error = true;
+       yk_errno = 0;
+
+       if (!yk_init()) {
+               goto err;
+       }
+
+       if (!(yk = yk_open_key(0))) {
+               goto err;
+       }
+
+       unsigned char buf[0x20];
+       unsigned int len = sizeof(buf);
+       unsigned int i;
+       if(!yk_get_capabilities(yk, 1, 0, buf, &len)) {
+               goto err;
+       }
+       printf("capabilities: ");
+       for(i = 0; i < len; i++) {
+               printf("%02x", buf[i]);
+       }
+       printf("\n");
+
+       error = false;
+
+err:
+       if (error != false) {
+               report_yk_error();
+       }
+
+       if (yk && !yk_close_key(yk)) {
+               report_yk_error();
+       }
+
+       if (!yk_release()) {
+               report_yk_error();
+       }
+}
+
 int main(void)
 {
        _test_yk_firmware();
+       _test_yk_get_capabilities();

        return 0;
 }
(gdb) b ykcore.c:200
Breakpoint 1 (ykcore.c:200) pending.
(gdb) r
Starting program: /home/gk/kode/yubikey-personalization/tests/test_yk_utilities 
Thread 1 "test_yk_utiliti" hit Breakpoint 1, yk_get_capabilities (yk=yk@entry=0x602960, slot=slot@entry=1 '\001', flags=flags@entry=0, 
    capabilities=capabilities@entry=0x7fffffffd670 "\f\001\001\377\002\004", len=len@entry=0x7fffffffd66c) at ykcore.c:203
203     *len = response_len;
Missing separate debuginfos, use: dnf debuginfo-install json-c-0.13.1-2.fc28.x86_64 libblkid-2.32.1-1.fc28.x86_64 libgcc-8.1.1-5.fc28.x86_64 libmount-2.32.1-1.fc28.x86_64 libselinux-2.8-1.fc28.x86_64 libusbx-1.0.22-1.fc28.x86_64 libuuid-2.32.1-1.fc28.x86_64 libyubikey-1.13-5.fc27.x86_64 pcre2-10.32-3.fc28.x86_64 systemd-libs-238-9.git0e0aa59.fc28.x86_64
(gdb) p capabilities[0]
$1 = 12 '\f'
(gdb) set capabilities[0]=255
(gdb) c
Continuing.
capabilities: ff0101ff02040074d4c003013ffa8a000000000000000000000000000000000080d7ffffff7f00000000000000000000100b4000000000001be1fdf6ff7f0000000000000000000088d7ffffff7f0000000004000100000000094000000000000000000000000000788a1bccc0a5031ac00940000000000080d7ffffff7f000000000000000000000000000000000000788a5b77bf5afce5788a9d1bbb48fce500000000000000000000000000000000000000000000000098d7ffffff7f000030e1fff7ff7f00001656def7ff7f000000000000000000000000000000000000c00940000000000080d7ffffff7f00000000000000000000ea09400000000000
[Thread 0x7ffff5c11700 (LWP 16997) exited]
[Inferior 1 (process 16987) exited normally]
$ cat ex
set breakpoint pending on
b ykcore.c:200
command
    bt
    p capabilities[0]
    set capabilities[0] = 255
    c
end
r
q
$
$ gdb -x ex ./test_yk_utilities 
Breakpoint 1 (ykcore.c:200) pending.
Thread 1 "test_yk_utiliti" hit Breakpoint 1, yk_get_capabilities (yk=yk@entry=0x603d70, slot=slot@entry=1 '\001', flags=flags@entry=0, 
    capabilities=capabilities@entry=0x7fffffffd660 "\f\001\001\377\002\004", len=len@entry=0x7fffffffd65c) at ykcore.c:201
201     response_len++;
#0  yk_get_capabilities (yk=yk@entry=0x603d70, slot=slot@entry=1 '\001', flags=flags@entry=0, capabilities=capabilities@entry=0x7fffffffd660 "\f\001\001\377\002\004", 
    len=len@entry=0x7fffffffd65c) at ykcore.c:201
#1  0x0000000000400b69 in _test_yk_get_capabilities () at test_yk_utilities.c:136
#2  main () at test_yk_utilities.c:164
$1 = 12 '\f'
Yubikey core error: wrong size
[Thread 0x7ffff5c11700 (LWP 31300) exited]
[Inferior 1 (process 31290) exited normally]