New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddressSanitizer - oob read using curl w/gnutls 7.56.1 when opening http endpoint #2093

Closed
Nephyrin opened this Issue Nov 17, 2017 · 16 comments

Comments

Projects
None yet
5 participants
@Nephyrin

Nephyrin commented Nov 17, 2017

The following is an AddressSanitizer report of a rare crash we are seeing when opening a relatively benign HTTP endpoint. This is fairly difficult to trigger, and I have been unable to produce any sane STR. My hope is that the ASAN report points rather cleanly to a problem -- but if not, I can put more effort into providing a testcase.

I am unable to reproduce this on 7.56.0, but as this only reproduces sporadically in a many-minute long process, take that with a grain of salt.

=================================================================
==2527==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x4c33add8 at pc 0xea87e447 bp 0x49b50be8 sp 0x49b50bd8
READ of size 4 at 0x4c33add8 thread T18
    #0 0xea87e446 in close_one lib32-libcurl-gnutls/src/curl-7.56.1/lib/vtls/gtls.c:1576
    #1 0xea7b7cc0 in conn_free lib32-libcurl-gnutls/src/curl-7.56.1/lib/url.c:3183
    #2 0xea7c95c0 in conn_free lib32-libcurl-gnutls/src/curl-7.56.1/lib/url.c:3304
    #3 0xea8084e9 in multi_done lib32-libcurl-gnutls/src/curl-7.56.1/lib/multi.c:621
    #4 0xea80ff65 in multi_runsingle lib32-libcurl-gnutls/src/curl-7.56.1/lib/multi.c:2017
    #5 0xea816d00 in curl_multi_perform lib32-libcurl-gnutls/src/curl-7.56.1/lib/multi.c:2161
    #6 0xea7f3e9e in easy_transfer lib32-libcurl-gnutls/src/curl-7.56.1/lib/easy.c:708
    #7 0xe8fd13ef in DownloadThread(void*) /home/buildbot/buildslave/staging_ubuntu12_linux/build/src/engine/downloadthread.cpp:884
    #8 0xf7a35df4 in asan_thread_start(void*) _asan_rtl_
    #9 0xf79ee35b in start_thread pthread_create.c:?
    #10 0xf7838ff5 in __GI___clone :?

0x4c33add8 is located 0 bytes to the right of 1368-byte region [0x4c33a880,0x4c33add8)
allocated by thread T18 here:
    #0 0xf7aee335 in calloc _asan_rtl_
    #1 0xea7cbe0c in allocate_conn lib32-libcurl-gnutls/src/curl-7.56.1/lib/url.c:4360

Thread T18 created by T0 here:
    <removed>

SUMMARY: AddressSanitizer: heap-buffer-overflow (/usr/lib32/libcurl-gnutls.so.4+0x118446)
Shadow bytes around the buggy address:
  0x29867560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x29867570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x29867580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x29867590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x298675a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x298675b0: 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa fa fa
  0x298675c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x298675d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x298675e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x298675f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x29867600: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           Fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==2527==ABORTING

curl/libcurl version

curl 7.56.1 (x86_64-pc-linux-gnu) libcurl/7.56.1 GnuTLS/3.5.16 zlib/1.2.11 libpsl/0.18.0 (+libicu/59.1) libssh2/1.8.0 nghttp2/1.23.1
Release-Date: 2017-10-23
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL

operating system

Arch Linux 64-bit, running a 32-bit application.

@Nephyrin

This comment has been minimized.

Show comment
Hide comment
@Nephyrin

Nephyrin Nov 17, 2017

As a very naive attempt at debugging this, I came across 70f1db3 which adds some attempt to allocate extra regions on top of struct connectdata, but the comment doesn't seem to match what is going on:

  /*
   * To avoid multiple malloc() calls, the ssl_connect_data structures
   * associated with a connectdata struct are allocated in the same block
   * as the latter. This field forces alignment to an 8-byte boundary so
   * that this all works.
   */
  long long *align_data__do_not_use;

vs allocate_conn:

#ifdef USE_SSL
#define SSL_EXTRA + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
#else
#define SSL_EXTRA 0
#endif
  struct connectdata *conn = calloc(1, sizeof(struct connectdata) + SSL_EXTRA);

If I'm understanding this right, long long *align_data__do_not_use; is meant to force 8-byte alignment for the last member of this structure, but sizeof(long long *) will be 4 on x86, and the subsequent math in allocate_conn could actually underestimate our total size. It looks like we're allocating a total of 1368 bytes, but starting at the offset of align_data__do_not_use, 4x sizeof_ssl_backend_data would require 1372 bytes:

[gdb] p sizeof(struct connectdata) - sizeof(long long *)
$44 = 1324
[gdb] p (int)(&(((struct connectdata*)0)->align_data__do_not_use))
$45 = 1324
[gdb] p 1324 + 4 * Curl_ssl->sizeof_ssl_backend_data
$46 = 1372
[gdb] p sizeof(struct connectdata) + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
$47 = 1368

Indeed, asan seems to suggest we're reading a few bytes off the end of this 1368 byte region in gnutls's close_one. I'm not sure where exactly these extra bits are packed into the end of the structure though, or where the 4x multiplier comes from, so I could be overlooking something.

Also, I would expect this to reproduce 100% of the time, but instead it takes many attempts even under address sanitizer, which seems to suggest some other condition must be met to trigger this.

Edit: Removed accidental copy+paste of half of my comment

Nephyrin commented Nov 17, 2017

As a very naive attempt at debugging this, I came across 70f1db3 which adds some attempt to allocate extra regions on top of struct connectdata, but the comment doesn't seem to match what is going on:

  /*
   * To avoid multiple malloc() calls, the ssl_connect_data structures
   * associated with a connectdata struct are allocated in the same block
   * as the latter. This field forces alignment to an 8-byte boundary so
   * that this all works.
   */
  long long *align_data__do_not_use;

vs allocate_conn:

#ifdef USE_SSL
#define SSL_EXTRA + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
#else
#define SSL_EXTRA 0
#endif
  struct connectdata *conn = calloc(1, sizeof(struct connectdata) + SSL_EXTRA);

If I'm understanding this right, long long *align_data__do_not_use; is meant to force 8-byte alignment for the last member of this structure, but sizeof(long long *) will be 4 on x86, and the subsequent math in allocate_conn could actually underestimate our total size. It looks like we're allocating a total of 1368 bytes, but starting at the offset of align_data__do_not_use, 4x sizeof_ssl_backend_data would require 1372 bytes:

[gdb] p sizeof(struct connectdata) - sizeof(long long *)
$44 = 1324
[gdb] p (int)(&(((struct connectdata*)0)->align_data__do_not_use))
$45 = 1324
[gdb] p 1324 + 4 * Curl_ssl->sizeof_ssl_backend_data
$46 = 1372
[gdb] p sizeof(struct connectdata) + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
$47 = 1368

Indeed, asan seems to suggest we're reading a few bytes off the end of this 1368 byte region in gnutls's close_one. I'm not sure where exactly these extra bits are packed into the end of the structure though, or where the 4x multiplier comes from, so I could be overlooking something.

Also, I would expect this to reproduce 100% of the time, but instead it takes many attempts even under address sanitizer, which seems to suggest some other condition must be met to trigger this.

Edit: Removed accidental copy+paste of half of my comment

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 17, 2017

Member

(See also #2083 and #2089)

Are you by any chancing switching off the CURL_GLOBAL_SSL bit in your call to curl_global_init() ?

Member

bagder commented Nov 17, 2017

(See also #2083 and #2089)

Are you by any chancing switching off the CURL_GLOBAL_SSL bit in your call to curl_global_init() ?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 17, 2017

Member

cc @dscho for the size logic

Member

bagder commented Nov 17, 2017

cc @dscho for the size logic

@Nephyrin

This comment has been minimized.

Show comment
Hide comment
@Nephyrin

Nephyrin Nov 18, 2017

Oddly enough, we're passing, err, ~only~ CURL_GLOBAL_SSL. I fixed this to use CURL_GLOBAL_ALL, but was unfortunately still able to reproduce after a few attempts.

Nephyrin commented Nov 18, 2017

Oddly enough, we're passing, err, ~only~ CURL_GLOBAL_SSL. I fixed this to use CURL_GLOBAL_ALL, but was unfortunately still able to reproduce after a few attempts.

@dscho

This comment has been minimized.

Show comment
Hide comment
@dscho

dscho Nov 18, 2017

Contributor

The offset of the do_not_use field is supposed to be the offset of the first SSL backend data... Let me see.

Contributor

dscho commented Nov 18, 2017

The offset of the do_not_use field is supposed to be the offset of the first SSL backend data... Let me see.

@dscho

This comment has been minimized.

Show comment
Hide comment
@dscho

dscho Nov 18, 2017

Contributor

Indeed, my memory did not fail me:

curl/lib/url.c

Lines 1872 to 1885 in 715f1f5

/*
* To save on malloc()s, the SSL backend-specific data has been allocated
* at the end of the connectdata struct.
*/
{
char *p = (char *)&conn->align_data__do_not_use;
conn->ssl[0].backend = (struct ssl_backend_data *)p;
conn->ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data);
conn->proxy_ssl[0].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 2);
conn->proxy_ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 3);
}

@Nephyrin could you see what the offset of align_data__do_not_use is, and then what conn->ssl[0].backend, conn->ssl[1].backend and the proxy_ssl equivalents look like?

Contributor

dscho commented Nov 18, 2017

Indeed, my memory did not fail me:

curl/lib/url.c

Lines 1872 to 1885 in 715f1f5

/*
* To save on malloc()s, the SSL backend-specific data has been allocated
* at the end of the connectdata struct.
*/
{
char *p = (char *)&conn->align_data__do_not_use;
conn->ssl[0].backend = (struct ssl_backend_data *)p;
conn->ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data);
conn->proxy_ssl[0].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 2);
conn->proxy_ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 3);
}

@Nephyrin could you see what the offset of align_data__do_not_use is, and then what conn->ssl[0].backend, conn->ssl[1].backend and the proxy_ssl equivalents look like?

@Nephyrin

This comment has been minimized.

Show comment
Hide comment
@Nephyrin

Nephyrin Nov 18, 2017

@dscho

[gdb] p conn
$72 = (struct connectdata *) 0xc8fa8080

# Offset of align_data
[gdb] p (int)(&(((struct connectdata*)0)->align_data__do_not_use))
$73 = 1324

# SSL_EXTRA math from allocate_conn
[gdb] p sizeof(struct connectdata) + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
$74 = 1368

# Location of all the backends
[gdb] p conn->ssl[0].backend
$75 = (struct ssl_backend_data *) 0xc8fa85ac
[gdb] p conn->ssl[1].backend
$76 = (struct ssl_backend_data *) 0xc8fa85b8
[gdb] p conn->proxy_ssl[0].backend
$77 = (struct ssl_backend_data *) 0xc8fa85c4
[gdb] p conn->proxy_ssl[1].backend
$78 = (struct ssl_backend_data *) 0xc8fa85d0

# Sizeof backend
[gdb] p sizeof(struct ssl_backend_data)
$79 = 12

# Conn + SSL_EXTRA (end of allocation)
[gdb] p (char *)conn + 1368
$80 = 0xc8fa85d8 ""

# proxy_ssl[1].backend + sizeof(struct ssl_backend_data)
[gdb] p (char *)conn->proxy_ssl[1].backend + 12
$81 = 0xc8fa85dc ""

The $74 line is the math from allocate_conn - it looks like adding that to &conn gives 0xc8fa85d8 while conn->proxy_ssl[1].backend + sizeof(struct ssl_backend_data) is 0xc8fa85dc (beyond that)

Nephyrin commented Nov 18, 2017

@dscho

[gdb] p conn
$72 = (struct connectdata *) 0xc8fa8080

# Offset of align_data
[gdb] p (int)(&(((struct connectdata*)0)->align_data__do_not_use))
$73 = 1324

# SSL_EXTRA math from allocate_conn
[gdb] p sizeof(struct connectdata) + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
$74 = 1368

# Location of all the backends
[gdb] p conn->ssl[0].backend
$75 = (struct ssl_backend_data *) 0xc8fa85ac
[gdb] p conn->ssl[1].backend
$76 = (struct ssl_backend_data *) 0xc8fa85b8
[gdb] p conn->proxy_ssl[0].backend
$77 = (struct ssl_backend_data *) 0xc8fa85c4
[gdb] p conn->proxy_ssl[1].backend
$78 = (struct ssl_backend_data *) 0xc8fa85d0

# Sizeof backend
[gdb] p sizeof(struct ssl_backend_data)
$79 = 12

# Conn + SSL_EXTRA (end of allocation)
[gdb] p (char *)conn + 1368
$80 = 0xc8fa85d8 ""

# proxy_ssl[1].backend + sizeof(struct ssl_backend_data)
[gdb] p (char *)conn->proxy_ssl[1].backend + 12
$81 = 0xc8fa85dc ""

The $74 line is the math from allocate_conn - it looks like adding that to &conn gives 0xc8fa85d8 while conn->proxy_ssl[1].backend + sizeof(struct ssl_backend_data) is 0xc8fa85dc (beyond that)

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 18, 2017

Member

isn't the bug that we have a pointer to long long but we subtracted sizeof long long and not long long *.
in other words -8 instead of -4 and hence this problem

diff --git a/lib/url.c b/lib/url.c
index d2208d5..2bea510 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1781,7 +1781,7 @@ static void llist_dtor(void *user, void *element)
 static struct connectdata *allocate_conn(struct Curl_easy *data)
 {
 #ifdef USE_SSL
-#define SSL_EXTRA + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
+#define SSL_EXTRA (4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long *))
 #else
 #define SSL_EXTRA 0
 #endif

?

edit: on second thought

diff --git a/lib/urldata.h b/lib/urldata.h
index 94f6922..10af6a6 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1012,7 +1012,7 @@ struct connectdata {
    * as the latter. This field forces alignment to an 8-byte boundary so
    * that this all works.
    */
-  long long *align_data__do_not_use;
+  long long align_data__do_not_use;
 #endif
 };

also i don't think we can guarantee 8/16 byte alignment for 32-bit systems even if it's long long so the wording will probably have to be adjusted

Member

jay commented Nov 18, 2017

isn't the bug that we have a pointer to long long but we subtracted sizeof long long and not long long *.
in other words -8 instead of -4 and hence this problem

diff --git a/lib/url.c b/lib/url.c
index d2208d5..2bea510 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1781,7 +1781,7 @@ static void llist_dtor(void *user, void *element)
 static struct connectdata *allocate_conn(struct Curl_easy *data)
 {
 #ifdef USE_SSL
-#define SSL_EXTRA + 4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long)
+#define SSL_EXTRA (4 * Curl_ssl->sizeof_ssl_backend_data - sizeof(long long *))
 #else
 #define SSL_EXTRA 0
 #endif

?

edit: on second thought

diff --git a/lib/urldata.h b/lib/urldata.h
index 94f6922..10af6a6 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1012,7 +1012,7 @@ struct connectdata {
    * as the latter. This field forces alignment to an 8-byte boundary so
    * that this all works.
    */
-  long long *align_data__do_not_use;
+  long long align_data__do_not_use;
 #endif
 };

also i don't think we can guarantee 8/16 byte alignment for 32-bit systems even if it's long long so the wording will probably have to be adjusted

@dscho

This comment has been minimized.

Show comment
Hide comment
@dscho

dscho Nov 19, 2017

Contributor

Ugh. @jay you are absolutely correct.

The confusion here comes from my initial idea to force 8-byte alignment, and using int64_t for that purpose. But IIRC that would have been the first use of int64_t in cURL and it is considered not necessarily portable.

So I first demoted it to long long, then thinking better about it and trying to use a pointer instead (as I figured that on 32-bit systems, a 4-byte alignment is plenty enough).

So I agree that the safest way is to change the field type to long long (as you suggested in your second thought), and to adjust the comment to say "to an 8-byte boundary (or however many bytes a long long occupies)".

Thanks for your excellent analysis! It proves once more that original authors can overlook crucial details because they glanced at their code for too long.

Contributor

dscho commented Nov 19, 2017

Ugh. @jay you are absolutely correct.

The confusion here comes from my initial idea to force 8-byte alignment, and using int64_t for that purpose. But IIRC that would have been the first use of int64_t in cURL and it is considered not necessarily portable.

So I first demoted it to long long, then thinking better about it and trying to use a pointer instead (as I figured that on 32-bit systems, a 4-byte alignment is plenty enough).

So I agree that the safest way is to change the field type to long long (as you suggested in your second thought), and to adjust the comment to say "to an 8-byte boundary (or however many bytes a long long occupies)".

Thanks for your excellent analysis! It proves once more that original authors can overlook crucial details because they glanced at their code for too long.

jay added a commit to jay/curl that referenced this issue Nov 19, 2017

url: fix alignment of ssl_connect_data struct
- Change 'align_data__do_not_use' in connectdata to type curl_off_t
  from long long * since we need at least 8 bytes when possible and
  long long may not be an available type.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes, which could lead to oob read/write
in a connection's secondary socket https proxy ssl_connect_data struct
(the secondary socket in a connection is used by ftp, others?).

Closes curl#2093
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 19, 2017

Member

Ok. That's actually something I forgot about which is since this is c89 we're a little limited there. We shouldn't be using long long either, we should use whatever 8 byte integer is available (if available) and we use curl_off_t for that.

next draft is here: https://github.com/curl/curl/compare/master...jay:fix_alignment?expand=1

Member

jay commented Nov 19, 2017

Ok. That's actually something I forgot about which is since this is c89 we're a little limited there. We shouldn't be using long long either, we should use whatever 8 byte integer is available (if available) and we use curl_off_t for that.

next draft is here: https://github.com/curl/curl/compare/master...jay:fix_alignment?expand=1

jay added a commit to jay/curl that referenced this issue Nov 19, 2017

url: fix alignment of ssl_backend_data struct
- Change 'align_data__do_not_use' in connectdata to type curl_off_t
  from long long * since we need at least 8 bytes when possible and
  long long may not be an available type.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes, which could lead to oob read/write
in a connection's secondary socket https proxy ssl_backend_data struct
(the secondary socket in a connection is used by ftp, others?).

Closes curl#2093

jay added a commit to jay/curl that referenced this issue Nov 20, 2017

url: fix alignment of ssl_backend_data struct
- Align the array of ssl_backend_data on a 16 byte boundary.

8 is likely to be ok but I went with 16 for posterity should one of the
ssl_backend_data structs change to contain a long double or something.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl#2093
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 20, 2017

Member

alternate take that uses 16 byte alignment for posterity: https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate?expand=1

edit: hm i might have to adjust the wording a little, calloc returns alignment suitable for any variable but if there's no 16 byte variable then it's not necessarily 16 byte alignment

edit again: another alternate take but with 32 byte max alignment:
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_2?expand=1

edit: another alternate take but with 64 byte max alignment:
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_3?expand=1

edit: another alternate take with ssl_backend_data alignment (most correct way it seems according to C89 4.10.3)
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_4?expand=1

Member

jay commented Nov 20, 2017

alternate take that uses 16 byte alignment for posterity: https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate?expand=1

edit: hm i might have to adjust the wording a little, calloc returns alignment suitable for any variable but if there's no 16 byte variable then it's not necessarily 16 byte alignment

edit again: another alternate take but with 32 byte max alignment:
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_2?expand=1

edit: another alternate take but with 64 byte max alignment:
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_3?expand=1

edit: another alternate take with ssl_backend_data alignment (most correct way it seems according to C89 4.10.3)
https://github.com/curl/curl/compare/master...jay:fix_alignment_alternate_4?expand=1

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 21, 2017

Member

I prefer the 8 byte aligned version, due to its relative simplicity.

Member

bagder commented Nov 21, 2017

I prefer the 8 byte aligned version, due to its relative simplicity.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 23, 2017

Member

@Nephyrin: can you verify that the issue is gone if you try @jay's fix in jay@9984412 ?

Member

bagder commented Nov 23, 2017

@Nephyrin: can you verify that the issue is gone if you try @jay's fix in jay@9984412 ?

jay added a commit to jay/curl that referenced this issue Nov 24, 2017

url: fix alignment of ssl_backend_data struct
- Align the array of ssl_backend_data on a max 32 byte boundary.

8 is likely to be ok but I went with 32 for posterity should one of
the ssl_backend_data structs change to contain a larger sized variable
in the future.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl#2093

jay added a commit to jay/curl that referenced this issue Nov 24, 2017

url: fix alignment of ssl_backend_data struct
- Align the array of ssl_backend_data on a max 32 byte boundary.

8 is likely to be ok but I went with 32 for posterity should one of
the ssl_backend_data structs change to contain a larger sized variable
in the future.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl#2093

jay added a commit to jay/curl that referenced this issue Nov 26, 2017

url: fix alignment of ssl_backend_data struct
- Align the array of ssl_backend_data on a max 64 byte boundary.

8 is likely to be ok but I went with 64 for posterity should one of
the ssl_backend_data structs change to contain a larger sized variable
in the future.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl#2093

jay added a commit to jay/curl that referenced this issue Nov 26, 2017

url: fix alignment of ssl_backend_data struct
- Use the size of ssl_backend_data for its alignment rather than guess
  the maximum alignment.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl#2093

@bagder bagder closed this in 9b5e12a Nov 27, 2017

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil commented Nov 29, 2017

@Nephyrin

This comment has been minimized.

Show comment
Hide comment
@Nephyrin

Nephyrin Nov 29, 2017

Sorry for the late reply -- I can no longer seem to reproduce this with 7.57.0. Thanks for digging into this!

Nephyrin commented Nov 29, 2017

Sorry for the late reply -- I can no longer seem to reproduce this with 7.57.0. Thanks for digging into this!

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 29, 2017

Member

np thanks for following up

Member

jay commented Nov 29, 2017

np thanks for following up

weltling added a commit to winlibs/cURL that referenced this issue Nov 29, 2017

url: fix alignment of ssl_backend_data struct
- Align the array of ssl_backend_data on a max 32 byte boundary.

8 is likely to be ok but I went with 32 for posterity should one of
the ssl_backend_data structs change to contain a larger sized variable
in the future.

Prior to this change (since dev 70f1db3, release 7.56) the connectdata
structure was undersized by 4 bytes in 32-bit builds with ssl enabled
because long long * was mistakenly used for alignment instead of
long long, with the intention being an 8 byte boundary. Also long long
may not be an available type.

The undersized connectdata could lead to oob read/write past the end in
what was expected to be the last 4 bytes of the connection's secondary
socket https proxy ssl_backend_data struct (the secondary socket in a
connection is used by ftp, others?).

Closes curl/curl#2093

CVE-2017-8818

Bug: https://curl.haxx.se/docs/adv_2017-af0a.html

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.