[Spice-devel] [PATCH libcacard v2 26/35] tests: Make sure we do not crash on bad data to sign
Jakub Jelen
jjelen at redhat.com
Tue Aug 21 16:52:28 UTC 2018
On Tue, 2018-08-21 at 17:35 +0200, Christophe Fergeau wrote:
> Hey,
>
> On Thu, Aug 02, 2018 at 11:43:58AM +0200, Jakub Jelen wrote:
> > diff --git a/tests/hwtests.c b/tests/hwtests.c
> > index 7beebac..bd8e439 100644
> > --- a/tests/hwtests.c
> > +++ b/tests/hwtests.c
> > @@ -268,6 +268,85 @@ static void test_get_response_hw(void) {
> > test_get_response();
> > }
> >
> > +/* Try to pass bad formatted PKCS#1.5 data and make sure the
> > libcacard does not
> > + * crash while handling them
> > + */
> > +static void test_sign_bad_data_x509(void)
> > +{
> > + VReader *reader = vreader_get_reader_by_id(0);
> > + VReaderStatus status;
> > + int dwRecvLength = APDUBufSize;
> > + uint8_t pbRecvBuffer[APDUBufSize];
> > + uint8_t sign[] = {
> > + /* VERIFY [p1,p2=0 ] [Lc ] [2048b keys: 256
> > bytes of non PKCS#1.5 data] */
> > + 0x80, 0x42, 0x00, 0x00, 0x00, 0x01, 0x00,
> > +0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +/* ^--- the second byte of data should be 0x01 for signatures
> > */
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > 0xff, 0xff, 0xff, 0xff, 0xff,
> > +0xff, 0xff, 0x00, 0x64, 0x61, 0x74, 0x61, 0x20, 0x74, 0x6f, 0x20,
> > 0x73, 0x69, 0x67, 0x6e, 0x20,
> > +0x28, 0x6d, 0x61, 0x78, 0x20, 0x31, 0x30, 0x30, 0x20, 0x62, 0x79,
> > 0x74, 0x65, 0x73, 0x29, 0x0a,
> > +0x00 /* <-- [Le] */
> > + };
> > + int sign_len = sizeof(sign);
> > + int key_bits = getBits();
> > +
> > + g_assert_nonnull(reader);
> > +
> > + /* Skip the HW tests without physical card */
> > + if (vreader_card_is_present(reader) != VREADER_OK) {
> > + vreader_free(reader);
> > + g_test_skip("No physical card found");
> > + return;
> > + }
> > +
> > + /* run the actual test */
> > +
> > + key_bits = getBits();
> > + /* Adjust the buffers to match the key lengths, if already
> > retrieved */
> > + if (key_bits && key_bits < 2048) {
> > + sign[4] = key_bits/8; /* less than 2048b will fit the
> > length into one byte */
> > + sign[5] = 0x00;
> > + /*sign[6] = 0x01; <- this should be 0x01 for PKCS#1.5
> > signatures */
> > + memcpy(&sign[6], &sign[sign_len-key_bits/8 + 3],
> > key_bits/8 - 1);
>
> This memcpy is overflowing 'sign' and is causing make check with
> -fsanitize=address to fail. We try to copy key_bits/8 - 1 bytes
> starting
> at offset sign_len-key_bits/8 + 3, which means we'll end up at offset
> key_bits/8 - 1 + sign_len - key_bits/8 + 3, which computes to
> sign_len +
> 2, while the 'sign' array only has a size of 'sign_len'.
>
> I'm replying to this email as I don't know what the right fix for
> this
> is.
Hello,
thank you for noticing this. It looks like I increased the number here
instead of decreasing as I should have. I agree that this test is a bit
wild so I will try to explain what is going on there and where is the
error:
* The sign array is the while APDU.
* For payload of less than 256 bytes (for RSA keys of less than 2k),
we fit Lc part in one byte (instead of 3 as in template), which means
the "header" is only 5 bytes long. The Le value is adjusted on line
298.
* On the sixth byte (line 299), there already start PKCS#1.5-padded
data [1]. This byte needs to be zero from [1]
* The line 300 marks where the error is -- we are trying to create bad
PKCS#1.5 padding -- we overwrite it with 0xff padding later.
* The memcpy line is moving the tail of padded data by shrinking the
padding in the middle
* The padding (for simplicity, 0xff bytes are used) starts on the 7th
byte and spans before the next 0x00 byte.
* The memcpy, in theory, should copy data up to the end of the
original, 256 bytes payload, but it can move also the last byte which
specifies the Le (handled separately). This means, we are 3 bytes off.
* target (&sign[6]) -- this is the place where the padding starts in
the new buffer
* length (key_bits/8 - 1) -- the length of payload is 128 bytes, but
we already wrote the first byte (line 299).
* the source is wrong (&sign[sign_len-key_bits/8]) -- we need to find
a place where to start copying data from to match the ends of payloads
in the buffer (128 B and 256 B).
* The lines 302 and 303 adjust the buffer lengths and appends 0x01
(Le) of APDU.
I tried to improve this part a bit with comments and local variable,
but if the above helped you to understand the issue, you can improve
the code and comments.
diff --git a/tests/hwtests.c b/tests/hwtests.c
--- a/tests/hwtests.c
+++ b/tests/hwtests.c
@@ -295,11 +295,12 @@ static void test_sign_bad_data_x509(void)
key_bits = getBits();
/* Adjust the buffers to match the key lengths, if already
retrieved */
if (key_bits && key_bits < 2048) {
- sign[4] = key_bits/8; /* less than 2048b will fit the length
into one byte */
- sign[5] = 0x00;
+ int payload_len = key_bits/8; /* RSA signature has the same
length as the key */
+ sign[4] = payload_len; /* less than 2048b will fit the length
into one byte */
+ sign[5] = 0x00; /* PKCS#1.5 padding first byte */
/*sign[6] = 0x01; <- this should be 0x01 for PKCS#1.5
signatures */
- memcpy(&sign[6], &sign[sign_len-key_bits/8 + 3], key_bits/8 -
1);
- sign_len = 5 + key_bits/8 + 1;
+ memcpy(&sign[6], &sign[sign_len - payload_len], payload_len -
1);
+ sign_len = 5 /* [APDU header] */ + payload_len + 1 /* [Le] */;
sign[sign_len-1] = 0x00; /* [Le] */
}
[1] https://tools.ietf.org/html/rfc2313#section-8.1
Thanks,
--
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.
More information about the Spice-devel
mailing list