[gnutls-devel] [PATCH 1/6] Explicitly marked some variables const in _gnutls_buffer_append_data().

[Jaak Ristioja]

On 17.12.2014 14:55, Nikos Mavrogiannopoulos wrote:
> Thanks. I've applied everything except the 2/4. While allowed in C99, I
> prefer to define variables in the beginning of the function. I've also
> committed an overflow check in 32-bit systems.

In my opinion, commit 8b592bc ("Added 32-bit overflow protection in
_gnutls_buffer_append_data()") only fixes some overflows present in
_gnutls_buffer_append_data(). These statements may still overflow:

    size_t const tot_len = data_size + dest->length;

    size_t const new_len =
        MAX(data_size, MIN_CHUNK) + MAX(dest->max_length,
                                        MIN_CHUNK);

Although unlikely on x86_64, they are still an issue. On systems with
32-bit size_t this means that dealing with more than 4GB of data will be
an issue.

I'm too lazy to lookup the x86_64 ABI, but on my 64-bit Linux machine
sizeof(int) is 4 and sizeof(size_t) is 8. Hence we still have a problem
with _gnutls_buffer_append_data, because its return type is signed int.
This means that the statement

  return tot_len;

will first truncate the 64-bit value of size_t tot_len to a 32-bit
signed integer value. And this is probably the issue we painfully bumped
into here at Cybernetica. The result of this was that
gnutls_record_send() returned an invalid result and caused our
application to call gnutls_record_send() again with the same data (in
part). Hence GnuTLS buffered some application data twice in the cork
buffer. The remote peer, upon receiving this invalid data, noticed an
invalid application message header at the next message boundary and
threw an exception.

Therefore we had to drop support for gnutls_record_cork() and
gnutls_record_uncork() because it is still unreliable when buffering
more than 2^31-1 bytes of data on 64-bit platforms.


Regards,
Jaak Ristioja
Cybernetica AS