Hey,
On Tue, Aug 21, 2018 at 06:52:28PM +0200, Jakub Jelen wrote:
> 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.
Yes, this is clearer with this explanation.. And the patch looks
good/fixes my -fsanitize=address issues. I'd change the memcpy to
memmove though. Feel free to push this with a commit log :)
Christophe
>
> 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.
>
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel