Skip to content
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

Fixed initialisation of sizeof_ssl_backend_data #2083

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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->get_sizeof_backend_data() - sizeof(long long))

This comment has been minimized.

Copy link
@dscho

dscho Nov 15, 2017

Contributor

The commit message says that something was fixed, but it is awfully scarce in saying what was fixed.

All I see is that the field of Curl_ssl that contained the size of the TLS backend-specific data was converted into a function, causing a lot of lines to be added (and worse, the code was also spread over the tls/*.c files, making it much easier for bugs to hide).

So in the least I would like to have an explanation in the commit message what this fixes.

Because I do not see that it fixes anything, and I really do not like the increased complexity of the code.

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 16, 2017

Author Contributor

Details were sent to security list.
I could put some details here.
To be short: main reason for functions is: ed975e3#diff-2a4943fddb36800ac9166e5450377984R1093
This way code will always initialise size before using value of size.
Currently SSL_EXTRA is resolved to + 4 * -1 - sizeof(long long), and pointers are set to wrong data at

curl/lib/url.c

Line 1880 in 6ce9845

(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data);
.

You could easily reproduce it with debugger. Set breakpoint at

curl/lib/url.c

Line 1788 in 6ce9845

struct connectdata *conn = calloc(1, sizeof(struct connectdata) + SSL_EXTRA);
, start any HTTP (not HTTPS!) connection and check value of Curl_ssl->sizeof_ssl_backend_data

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 16, 2017

Author Contributor

If you want to put more detailed description to commit message - let me know.

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 16, 2017

Author Contributor

Important note for checking with debugger.
You need to not initialise SSL part of libcurl by not using of curl_global_init(CURL_GLOBAL_SSL).
Usually application, which use only plain connection, do not request CURL_GLOBAL_SSL
I.e. you should call curl_global_init(CURL_GLOBAL_NOTHING).

This comment has been minimized.

Copy link
@bagder

bagder Nov 16, 2017

Member

I agree with @dscho that the fix is rather intrusive and unfortunate. I think we should try to come up with a different take that doesn't require this new function in all TLS backends.

I don't see how multissl libcurl can work without getting SSL initialized and it seems totally unlikely that an application would actually init the TLS library in a multissl situation. I'm trying to think of reasons why we shouldn't just make multissl libcurl ignore that flag and use that very simple fix...

This comment has been minimized.

Copy link
@dscho

dscho Nov 17, 2017

Contributor

Hmpf. I chose (size_t)-1 under the assumption that this would result in an insanely large integer, and fail. Apparently this is not the case?

The problem I tried to catch with that trick (and obviously failed) is to ensure that nobody calls this code without having initialized the TLS backend.

So here is an idea for a less intrusive fix: introduce a new function

void ensure_multissl_initialized(void)
{
    multissl_init(NULL);
}

at the end of lib/vtls/vtls.c, declare it in lib/vtls/vtls.h and then insert a call to that function exactly here:

curl/lib/url.c

Line 1872 in 6ce9845

/*

This comment has been minimized.

Copy link
@dscho

dscho Nov 17, 2017

Contributor

Ah, I just saw that you responded later with #2089, which I think is fine.

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 17, 2017

Author Contributor

@dscho You will need this to call new function before calling calloc.
However, potentially size could be used somewhere else without initialisation. And even if size of not used currently, your solution is not future-proof.

This comment has been minimized.

Copy link
@dscho

dscho Nov 18, 2017

Contributor

Sure, your method is safer.

However, I do not like at all that you smooshed the refactoring (turn the size field into a function) in with the real fix (call multissl_init(NULL) from Curl_ssl_multi's size function). My confusion over what the real fix is should be an eloquent hint to you why that was an unreasonable course of action.

Further, I do not like at all the terseness of the commit messages. The way they speak to me imitates a really grumpy and unfriendly developer basically saying "I won't tell you anything. You have to find out everything that is important about this commit yourself. I had to do the hard work, so I now make you do the same, out of sheer spite."

That is definitely not the way commit messages are expected to be. They should be friendly and helpful. And if the developer had to figure something out in order to come up with the changes, that better be described in the commit message.

You can see how negative I am about your changes (and I am not alone), and I am convinced that that would change very quickly once you up your game with respect to commit messages and explaining motiviation (and maybe implementation details, if they are not obvious). Just a little bit of effort to make others understand quicker and easier what you did will do wonders, I promise you that.

In short:

  • untangle refactoring from the actual fix, into two commits
  • substantially better commit messages (describing the why primarily, if necessary the how, this goes for all commits, not only the ones in this PR)
  • explain clearly from the beginning what is the issue that was addressed in the PR (this also goes for every PR, not just this one).

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 18, 2017

Author Contributor

@dscho Hiding details was intentional.
When I previously discovered crash point and addressing of out-of-bound data, I was requested to not provide details on public PR and write to specific ML.
That is what I did in this PR: I sent all details with explanation to ML and provide basic fix in this PR.
Moreover, in mailinglist I explained with all small details and even did it two (or three) times.
So don't blame about small details in commits.
Same reason for providing small details in PR description.

I'd really appreciate, if curl maintainers will give me concrete unambiguous advise what to do next time in similar situation.

I could easily copy-paste details from my messages in mailing list to PR description.
However, I thinks it is pointless to add commit without calling initialisation: such commit would be completely senseless.

Therefore, please:

  1. Tell me whether I really should add details to PR description.
  2. Choose which PR will be merged. I do not want to do double work.

This comment has been minimized.

Copy link
@dscho

dscho Nov 25, 2017

Contributor

Details were sent to security list.

I am not a member of that security list, nor would it be appropriate: I am a mere contributor to the project, just as you are.

However, I have a vested interest in the SSL backend code to stay healthy, which is best ensured by working together to keep bugs out. It is great that you are squashing a bug I introduced. It is less great that you simply ignore my review (or at least make all signs of waving off my concerns, and show the inclination of rejecting every single of my suggestions without satisfactory explanation).

If the cURL maintainers are fine with the PR as-is, I can't do anything about it.

My concerns still stand, though: the commit messages in the current form will come back to bite whoever has to work on the code in question in the future, and I still deem it absolutely necessary to split the logically-different steps of refactoring the code to change the field to a method from the patch to ensure that the SSL backend is initialized before the SSL backend data is allocated.

I am a little annoyed that these concerns (which I really deem to make sense) have not been addressed in any satisfying way.

#else
#define SSL_EXTRA 0
#endif
@@ -1877,11 +1877,11 @@ static struct connectdata *allocate_conn(struct Curl_easy *data)
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);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data());
conn->proxy_ssl[0].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 2);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data() * 2);
conn->proxy_ssl[1].backend =
(struct ssl_backend_data *)(p + Curl_ssl->sizeof_ssl_backend_data * 3);
(struct ssl_backend_data *)(p + Curl_ssl->get_sizeof_backend_data() * 3);
}
#endif

@@ -701,6 +701,11 @@ static void *Curl_axtls_get_internals(struct ssl_connect_data *connssl,
return BACKEND->ssl;
}

static size_t Curl_axtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_axtls = {
{ CURLSSLBACKEND_AXTLS, "axtls" }, /* info */

@@ -710,7 +715,7 @@ const struct Curl_ssl Curl_ssl_axtls = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_axtls_get_backend_size, /* get_sizeof_backend_data */

/*
* axTLS has no global init. Everything is done through SSL and SSL_CTX
@@ -977,6 +977,11 @@ static void *Curl_cyassl_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_cyassl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_cyassl = {
{ CURLSSLBACKEND_WOLFSSL, "WolfSSL" }, /* info */

@@ -990,7 +995,7 @@ const struct Curl_ssl Curl_ssl_cyassl = {
1, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_cyassl_get_backend_size, /* get_sizeof_backend_data */

Curl_cyassl_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -2976,6 +2976,11 @@ static void *Curl_darwinssl_get_internals(struct ssl_connect_data *connssl,
return BACKEND->ssl_ctx;
}

static size_t Curl_darwinssl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_darwinssl = {
{ CURLSSLBACKEND_DARWINSSL, "darwinssl" }, /* info */

@@ -2989,7 +2994,7 @@ const struct Curl_ssl Curl_ssl_darwinssl = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_darwinssl_get_backend_size, /* get_sizeof_backend_data */

Curl_none_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -1352,6 +1352,11 @@ static void *Curl_gskit_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_gskit_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_gskit = {
{ CURLSSLBACKEND_GSKIT, "gskit" }, /* info */

@@ -1362,7 +1367,7 @@ const struct Curl_ssl Curl_ssl_gskit = {
/* TODO: convert to 1 and fix test #1014 (if need) */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_gskit_get_backend_size, /* get_sizeof_backend_data */

Curl_gskit_init, /* init */
Curl_gskit_cleanup, /* cleanup */
@@ -1805,6 +1805,11 @@ static void *Curl_gtls_get_internals(struct ssl_connect_data *connssl,
return BACKEND->session;
}

static size_t Curl_gtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_gnutls = {
{ CURLSSLBACKEND_GNUTLS, "gnutls" }, /* info */

@@ -1814,7 +1819,7 @@ const struct Curl_ssl Curl_ssl_gnutls = {
0, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_gtls_get_backend_size, /* get_sizeof_backend_data */

Curl_gtls_init, /* init */
Curl_gtls_cleanup, /* cleanup */
@@ -1039,6 +1039,11 @@ static void *Curl_mbedtls_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ssl;
}

static size_t Curl_mbedtls_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_mbedtls = {
{ CURLSSLBACKEND_MBEDTLS, "mbedtls" }, /* info */

@@ -1048,7 +1053,7 @@ const struct Curl_ssl Curl_ssl_mbedtls = {
1, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_mbedtls_get_backend_size, /* get_sizeof_backend_data */

Curl_mbedtls_init, /* init */
Curl_mbedtls_cleanup, /* cleanup */
@@ -2342,6 +2342,11 @@ static void *Curl_nss_get_internals(struct ssl_connect_data *connssl,
return BACKEND->handle;
}

static size_t Curl_nss_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_nss = {
{ CURLSSLBACKEND_NSS, "nss" }, /* info */

@@ -2351,7 +2356,7 @@ const struct Curl_ssl Curl_ssl_nss = {
0, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_nss_get_backend_size, /* get_sizeof_backend_data */

Curl_nss_init, /* init */
Curl_nss_cleanup, /* cleanup */
@@ -3621,6 +3621,11 @@ static void *Curl_ossl_get_internals(struct ssl_connect_data *connssl,
(void *)BACKEND->ctx : (void *)BACKEND->handle;
}

static size_t Curl_ossl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_openssl = {
{ CURLSSLBACKEND_OPENSSL, "openssl" }, /* info */

@@ -3630,7 +3635,7 @@ const struct Curl_ssl Curl_ssl_openssl = {
1, /* have_ssl_ctx */
1, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_ossl_get_backend_size, /* get_sizeof_backend_data */

Curl_ossl_init, /* init */
Curl_ossl_cleanup, /* cleanup */
@@ -898,6 +898,11 @@ static void *Curl_polarssl_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ssl;
}

static size_t Curl_polarssl_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_polarssl = {
{ CURLSSLBACKEND_POLARSSL, "polarssl" }, /* info */

@@ -907,7 +912,7 @@ const struct Curl_ssl Curl_ssl_polarssl = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_polarssl_get_backend_size, /* get_sizeof_backend_data */

Curl_polarssl_init, /* init */
Curl_polarssl_cleanup, /* cleanup */
@@ -1816,6 +1816,11 @@ static void *Curl_schannel_get_internals(struct ssl_connect_data *connssl,
return &BACKEND->ctxt->ctxt_handle;
}

static size_t Curl_schannel_get_backend_size(void)
{
return sizeof(struct ssl_backend_data);
}

const struct Curl_ssl Curl_ssl_schannel = {
{ CURLSSLBACKEND_SCHANNEL, "schannel" }, /* info */

@@ -1825,7 +1830,7 @@ const struct Curl_ssl Curl_ssl_schannel = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

sizeof(struct ssl_backend_data),
Curl_schannel_get_backend_size, /* get_sizeof_backend_data */

Curl_schannel_init, /* init */
Curl_schannel_cleanup, /* cleanup */
@@ -220,7 +220,7 @@ ssl_connect_init_proxy(struct connectdata *conn, int sockindex)
conn->proxy_ssl[sockindex] = conn->ssl[sockindex];

memset(&conn->ssl[sockindex], 0, sizeof(conn->ssl[sockindex]));
memset(pbdata, 0, Curl_ssl->sizeof_ssl_backend_data);
memset(pbdata, 0, Curl_ssl->get_sizeof_backend_data());

conn->ssl[sockindex].backend = pbdata;
}
@@ -1088,6 +1088,13 @@ CURLcode Curl_none_md5sum(unsigned char *input UNUSED_PARAM,
}
#endif

static size_t Curl_multissl_get_backend_size(void)
{
if(multissl_init(NULL))
return 0;

This comment has been minimized.

Copy link
@dscho

dscho Nov 17, 2017

Contributor

Ah, see, that's why large patches are bad: I missed this particular part.

And here you suggest one possible alternative, less intrusive solution: replace the -1 in Curl_ssl_multi by 0.

However, I still would like the alternative I proposed earlier better, where multissl_init(NULL); is called from url.c just before we really need to know the size of the backend data. If you are truly concerned about performance, you can even guard that call behind a test whether the size is still equal to (size_t)-1.

This comment has been minimized.

Copy link
@Karlson2k

Karlson2k Nov 18, 2017

Author Contributor

@dscho Do not call this tiny patch "large patch". 😃 It simply have a lot of similar lines so it is easy to miss interesting points.

Yep, initialisation could be called from many points.
However, it is not very intuitive to check size value and call TLS initialisation from url.c. I'd better to stick to one of my PRs. Either call initialisation with global initialisation (no additional is required) or use automatic initialisation (like in this PR).

This comment has been minimized.

Copy link
@dscho

dscho Nov 25, 2017

Contributor

It simply have a lot of similar lines so it is easy to miss interesting points.

Exactly. It is easy to miss the interesting points. Corollary: it is hard to review. Corollary: it is hard to ensure that no bug was introduced.

The solution is easy, and I have mentioned it several times: split the commit into two, logically-separate patches. One to refactor, one to fix. That would be easy.

I am not a cURL maintainer, but if I were, I would not accept the PR before that split.

return Curl_ssl->get_sizeof_backend_data();
}

static int Curl_multissl_init(void)
{
if(multissl_init(NULL))
@@ -1134,7 +1141,7 @@ static const struct Curl_ssl Curl_ssl_multi = {
0, /* have_ssl_ctx */
0, /* support_https_proxy */

(size_t)-1, /* something insanely large to be on the safe side */
Curl_multissl_get_backend_size, /* get_sizeof_backend_data */

Curl_multissl_init, /* init */
Curl_none_cleanup, /* cleanup */
@@ -40,7 +40,7 @@ struct Curl_ssl {

unsigned support_https_proxy:1; /* supports access via HTTPS proxies */

size_t sizeof_ssl_backend_data;
size_t (*get_sizeof_backend_data)(void);

int (*init)(void);
void (*cleanup)(void);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.