[Spice-devel] [PATCH libcacard v2 26/35] tests: Make sure we do not crash on bad data to sign

[Christophe Fergeau]

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180821/052474c6/attachment.sig>