USB serial number parsing has problems with leading zeros

[invd]

I recently noticed that leading zeros in the device serial number throw off the command line parsing when specifying -C "yhusb://serial= to connect to a particular local device.

Good behaviour:

./yubihsm-shell -C "yhusb://serial=4242424"
yubihsm> debug all
Debug messages enabled
yubihsm> connect
[LIB - INF ...] yubihsm.c:4069 (yh_init_connector): Loading usb backend
[LIB - INF ...] lib_util.c:166 (parse_usb_url): USB url parsed with serial 4242424.

Unexpected behaviour:

./yubihsm-shell -C "yhusb://serial=0004242424"
yubihsm> debug all
Debug messages enabled
yubihsm> connect
[LIB - INF ...] yubihsm.c:4069 (yh_init_connector): Loading usb backend
[LIB - INF ...] lib_util.c:166 (parse_usb_url): USB url parsed with serial 1131796.

1131796 is not a reasonable serial number in this context.

This is relevant for people that use the serial number as delivered by lsusb -v, example output:

lsusb -v
[...]
  iManufacturer           1 Yubico
  iProduct                2 YubiHSM
  iSerial                 3 0004242424

[nevun]

The code in https://github.com/Yubico/yubihsm-shell/blob/master/lib/lib_util.c#L156 uses 0 as third argument to strtoul which means prepending 0x and 0 to a number automatically means it is parsed as hex or octal.
I agree it is not so obvious since it not documented (nor tested in the unit tests).

Simple solution is to just change the last argument in this instance to 10, i.e.:

-- a/lib/lib_util.c
+++ b/lib/lib_util.c
@@ -153,7 +153,7 @@ bool parse_usb_url(const char *url, unsigned long *serial) {
         str += strlen("serial=");
 
         errno = 0;
-        *serial = strtoul(str, &endptr, 0);
+        *serial = strtoul(str, &endptr, 10);
         if ((errno == ERANGE && *serial == ULONG_MAX) || endptr == str ||
             (errno != 0 && *serial == 0)) {
           *serial = 0;

..which leads to the behavior you expected:

$ ./src/yubihsm-shell -C yhusb://serial=0004242424
yubihsm> debug all
Debug messages enabled
yubihsm> connect
[LIB - INF 12:49:45.418410] yubihsm.c:4069 (yh_init_connector): Loading usb backend
[LIB - INF 12:49:45.430295] lib_util.c:166 (parse_usb_url): USB url parsed with serial 4242424.

I do not have a strong opinion here as it does not have any clear security implications but it feels natural that it should be documented and be reflected in the unit test (lib/tests/test_usb_url.c).

[invd]

@nevun: as you mention in the linked commit, it is theoretically possible that some scripts or other usage of the CLI tool are based on the hex/octal behaviour. I assume it is a rarely used edge case and wasn't documented as such.
The new variant is more friendly to users, so I would personally prefer it, but understand if the old behaviour is kept for compatibility reasons.

[a-dma]

I'm OK with the change, but I think that at that point it's worth returning false from that function, at least in the 0x case.

I believe the behavior now is quite surprising in that instance: as shown in the test if a serial number of 0x1234 is given the function will succeed but set serial to 0, i.e., match any serial number. Sure it will print a debug message but that's probably not enough if the function doesn't fail.

The octal case is a bit trickier since a leading zero doesn't make strtoul fail. Maybe it's worth adding the word decimal somewhere on line 166?