Opensc: sc_hex_to_bin() allows odd number of hexchars
sc_hex_to_bin() allows decoding hexadecimal strings with an odd number of characters, e.g. 12345 is decoded as 0x123405.
While this seems reasonable if the hexadecimal string contains delimiters, it poses a problem if a character was accidentally left out or added.
We could change the code to check that two nibbles are required to form a byte, but this would break existing code that allows or uses 1 digit hex values.
CardContact
All 18 comments
Here is a better solution: allow for odd numbers of hexchars, while putting the leading zero required in this case in the first (most-significant) byte, not in the last one. For the given examle, 12345 should be encoded as 0x01, 0x23, 0x45. Here's a variant of sc_hex_to_bin() with the respective fix:
int sc_hex_to_bin(const char *in, u8 *out, size_t *outlen)
{
int err = SC_SUCCESS;
size_t left, count = 0;
int nybbles = 2;
assert(in != NULL && out != NULL && outlen != NULL);
left = *outlen;
if (strlen(in) % 2)
nybbles = 1; // any leading zero should be in most-significant byte, not last one!
while (*in != '\0') {
int byte = 0;
while (nybbles-- && *in && *in != ':' && *in != ' ') {
char c;
byte <<= 4;
c = *in++;
if ('0' <= c && c <= '9')
c -= '0';
else
if ('a' <= c && c <= 'f')
c = c - 'a' + 10;
else
if ('A' <= c && c <= 'F')
c = c - 'A' + 10;
else {
err = SC_ERROR_INVALID_ARGUMENTS;
goto out;
}
byte |= c;
}
if (*in == ':' || *in == ' ')
in++;
if (left <= 0) {
err = SC_ERROR_BUFFER_TOO_SMALL;
break;
}
out[count++] = (u8) byte;
left--;
nybbles = 2;
}
out:
*outlen = count;
return err;
}
BTW, the same holds for hex_to_bin() in pkcs11-tools.c.
DDvO
on 21 Apr 2016
Here is one more related (minor) fix: add
while (*in == '0' && *(++in) != '\0') ; // strip leading zeros in input
just before
if (strlen(in) % 2)
nybbles = 1; // any leading zero in output should be in most-significant byte, not last one!
@DDvO throwing an error is better than applying too much magic. If you want to add more bug fixes please use a pull request.
frankmorgner
on 3 May 2016
Here is one more related (minor) fix: add
while (*in == '0' && *(++in) != '\0') ; // strip leading zeros in input
just before
if (strlen(in) % 2)
nybbles = 1; // any leading zero in output should be in most-significant byte, not last one!
I see no harm in allowing an odd number of hex digits and any number of leading zeros in hex input, and I wouldn't call the proposed solution magical.
In any case, the current situation where a zero nibble gets inserted in the wrong place in the binary output is buggy.
DDvO
on 4 May 2016
@DDvO @CardContact
Also see https://github.com/OpenSC/OpenSC/issues/751#issuecomment-216842172 as sc_hex_to_bin is used by sc_pkcs15_format_id and may be used in other functions too.
dengert
on 4 May 2016
Apart from potentially unintended dependencies due to shared code, there is also the opposite problem: various functionality is duplicated (with the usual negative effects such redundancy has on code management), at least between pkcs11-tool.c and other related code. In particular, pkcs11-tool.c and libp11 have their own implementation of accessing objects via PKCS#11 functions like C_GetAttributeValue.
DDvO
on 4 May 2016
This code/fix breaks the currently working good PIV and CAC cards. it must be reworked or removed.
mouse07410
on 4 May 2016
I would suggest that commit c63ba858e32950b975bc6817c4dd9276ceea8715 be reverted and not included in 0.16.0.
There are to many side effects and there are to many proposed changes to this routine that are trying to address what I would call is a minor error.
The "error" of having only one nybble is used to advantage in working code.
Binary data created using old method may have been written to cards, and
a search using any new code should be able to match the data stored on existing cards.
sc_hex_to_bin is called from many places and some to not check the return code.
Some routines that call sc_hex_to_bin check the return code, but do not handle it correctly.
For example:
"1" in old code returns len=1, {0x01}
In c63ba858 this is considered error.
Any proposed change should also be able to handle something like:
"00 :1::2:345 6"
Which might return:
len = 6 {0x00, 0x01, 0x02, 0x03, 0x45, 0x06}
(Or what should it return?)
based on old code I think would return:
len = 6 {0x00, 0x01, 0x02, 0x34, 0x05, 0x06}
which looks strange, but that is the old code.
Leading 0x00 should not be dropped.
Discussion and keeping compatibility with existing code and previously created data is important.
dengert
on 4 May 2016
The fix https://github.com/OpenSC/OpenSC/issues/737#issuecomment-212824627 I proposed first (without dropping leading 0s in the input) fulfills the requirements stated by @dengert.
As mentioned, in case of uneven number of input hex digits the old code inserts the padding 0 nibble in the output in the wrong place.
DDvO
on 6 May 2016
@DDvO your proposal is incomplete 1:2:34 would be translated to 0x1234 instead of 0x010234... Also, your approach
if (strlen(in) % 2)
nybbles = 1; // any leading zero should be in most-significant byte, not last one!
does not take any whitespace characters into account.
frankmorgner
on 9 May 2016
As said before, getting this magic right is hard. That's why our code should stay simple!
frankmorgner
on 9 May 2016
Oops, yes, so far I did not take whitespace and colons into account.
Here is a completed version of my fix: add the following three lines:
char *p; for(p = (char*)in; *p && *p != ':' && *p != ' '; p++);
if ((p-in) % 2)
nybbles = 1; // any leading zero in output should be in most-significant byte, not last one!
just before
while (nybbles-- && *in && *in != ':' && *in != ' ') {
On input 00:1::2:345:6:78 one now gets 00 01 00 02 03 45 06 78 which I think is what was intended.
DDvO
on 13 May 2016
we have agreed about one solution in #752. If you want to make an other suggestion, please make a pull request
frankmorgner
on 13 May 2016
If I am correct, these two solutions are complementary? I.e., this one can enhance what was made to work in #752 ?
mouse07410
on 13 May 2016
AFAICS, #752 'solves' the problem by disallowing an uneven number of hex digits on input.
I find this needlessly restrictive. Like anywhere else when giving numeric literals, one should be allowed to use any number of leading zeros. Otherwise users are bossed around by the incapability of that piece of software.
DDvO
on 17 May 2016
sc_hex_to_bin does not necessarily produce a "numeric literals". In addition to the bin output it also returns the length,
thus leading zeros are significant. Its used mostly for ATRs, symmetric keys and PKCS#11/15 IDs.
Do you have a example in the OpenSC code where you would want to enter "any number of leading zeros"?
On 5/17/2016 2:07 AM, David von Oheimb wrote:
AFAICS, #752 https://github.com/OpenSC/OpenSC/pull/752 'solves' the problem by disallowing an uneven number of hex digits on input.
I find this needlessly restrictive. Like anywhere else when giving numeric literals, one should be able to use any number of leading zeros.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://github.com/OpenSC/OpenSC/issues/737#issuecomment-219636473
Douglas E. Engert [email protected]
dengert
on 17 May 2016
I was referring there to general numeric literals (in this case, hexadecimal) as input, not to the output side of (sc_)hex_to_bin().
Maybe we need to clarify how exactly the input side is defined. Is there a proper definition? PKCS#11 only defines that the output side is a byte array. In this sense I think you are right that the number of leading zeros matters modulo padding to byte size. That is, '01de' should be seen as different from '0001de', but I'd say '123de' should be seen as equivalent to '0123de', right?
This is what my proposed solution respects/implements.
In any case, simply forbidding an uneven number of input digits is a pretty unnatural restriction which, as people already started noticing, has awkward effects (also on OpenSC code itself, see all the artificial changes to ID constants, e.g., in https://github.com/OpenSC/OpenSC/pull/752/commits/e46bfea8b96eb9cd036ba13814bce760729e4220)
DDvO
on 17 May 2016
Related issues
alex-nitrokey
·
11Comments
RaymiiOrg
·
4Comments
gdsotirov
·
15Comments
dengert
·
19Comments
alex-nitrokey
·
13Comments
Most helpful comment
I would suggest that commit c63ba858e32950b975bc6817c4dd9276ceea8715 be reverted and not included in 0.16.0.
There are to many side effects and there are to many proposed changes to this routine that are trying to address what I would call is a minor error.
The "error" of having only one nybble is used to advantage in working code.
Binary data created using old method may have been written to cards, and
a search using any new code should be able to match the data stored on existing cards.
sc_hex_to_bin is called from many places and some to not check the return code.
Some routines that call sc_hex_to_bin check the return code, but do not handle it correctly.
For example:
"1" in old code returns len=1, {0x01}
In c63ba858 this is considered error.
Any proposed change should also be able to handle something like:
"00 :1::2:345 6"
Which might return:
len = 6 {0x00, 0x01, 0x02, 0x03, 0x45, 0x06}
(Or what should it return?)
based on old code I think would return:
len = 6 {0x00, 0x01, 0x02, 0x34, 0x05, 0x06}
which looks strange, but that is the old code.
Leading 0x00 should not be dropped.
Discussion and keeping compatibility with existing code and previously created data is important.