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
base: master
from

Conversation

Projects
None yet
4 participants
@Karlson2k
Contributor

Karlson2k commented Nov 15, 2017

This patch properly initialise TLS backend size before use.

@Karlson2k Karlson2k force-pushed the Karlson2k:fix_mssl branch from 048a83b to 12cd041 Nov 15, 2017

@Karlson2k Karlson2k force-pushed the Karlson2k:fix_mssl branch from 12cd041 to ed975e3 Nov 15, 2017

@jay

This comment has been minimized.

Member

jay commented Nov 15, 2017

/cc @dscho

@dscho

I think that the change that removes the + is good, but needs a much better commit message to be convincing. I am fairly certain that the second commit is unnecessary.

lib/url.c Outdated
@@ -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.

@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.

@Karlson2k

Karlson2k Nov 16, 2017

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.

@Karlson2k

Karlson2k Nov 16, 2017

Contributor

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

This comment has been minimized.

@Karlson2k

Karlson2k Nov 16, 2017

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.

@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.

@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.

@Karlson2k

Karlson2k Nov 17, 2017

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.

@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.

@Karlson2k

Karlson2k Nov 18, 2017

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.

@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.

lib/url.c Outdated
@@ -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))

This comment has been minimized.

@dscho

dscho Nov 15, 2017

Contributor

So the commit message should say something along the lines:

url.c: remove superfluous `+` in `SSL_EXTRA`

When one or more TLS backends is compiled in, `allocate_conn()` wants to reserve enough space for four slots of TLS backend-specific data. To that end, the `SSL_EXTRA` macro is defined which simply reports the delta, so that the four slots as well as the `connectdata` structure can be allocated (and deallocated) in one fell swoop.

The previous version of this code had a leading `+` in the definition of `SSL_EXTRA` (inconsistent with the value `0` in case no TLS backend is compiled in), which this commit removes.

This fixes ...

The most important part is the "This fixes ..." which I really would love to see filled in. Because the way I see it, the C compilers I know will handle 1 + + 2 as if you wanted to add +2 to 1, and return 3.

Is there maybe a compiler that mishandles this?

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Nov 17, 2017

Alternative PR #2089

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

This comment has been minimized.

@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.

@Karlson2k

Karlson2k Nov 18, 2017

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.

@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.

@bagder

This comment has been minimized.

Member

bagder commented Nov 20, 2017

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

As mentioned in documentation:

If you find a bug or problem in curl or libcurl that you think has a security impact, for example a bug that can put users in danger or make them vulnerable if the bug becomes public knowledge, then please report that bug using our security development process.

Security related bugs or bugs that are suspected to have a security impact, should be reported by email to curl-security@haxx.se so that they first can be dealt with away from the public to minimize the harm and impact it will have on existing users out there who might be using the vulnerable versions.

Is that an unclear description?

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Nov 21, 2017

@bagder Think it is clear. Nice that you know where to look to this description. Hope others will read documentation too and will not blame me about short commit descriptions.

@bagder So, what's the decision for bug qualification? Is it security bug or not? Should I publish more information about it?
If you are OK with publishing more information, which PR should I update?

@bagder

This comment has been minimized.

Member

bagder commented Nov 23, 2017

@Karlson2k for security sensitive bugs, nothing should be posted about it in public (until the publication date) so that we don't risk hurt existing users unnecessarily. So as long as a bug is considered a security issue, it should not be posted on github and there should be no public PR about it. Explained in our security development process document.

PRs that aren't security issues should have as good commit messages as possible. They should therefor never be shortened or held back for security reasons, since if they were security sensitive they shouldn't be public in the first place!

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Nov 23, 2017

@bagder OK!
So should I recognise this bug as not-security bug?
Which PR should I update with details?

bagder added a commit that referenced this pull request Nov 23, 2017

global_init: ignore CURL_GLOBAL_SSL's absense
This bit is no longer used. It is not clear what it meant for users to
"init the TLS" in a world with different TLS backends and since the
introduction of multissl, libcurl didn't properly work if inited without
this bit set.

Not a single user responded to the call for users of it:
https://curl.haxx.se/mail/lib-2017-11/0072.html

Reported-by: Evgeny Grin

Fixes #2089
Fixes #2083

jay added a commit to jay/curl that referenced this pull request Nov 25, 2017

SSL: Call SSL initialization and de-initialization always
- Call SSL initialization even when CURL_GLOBAL_SSL is not set as a
  global init flag. For backwards compatibility the SSL library-specific
  part of the initialization (ie initialization routines not in libcurl)
  will only be run if CURL_GLOBAL_SSL is set.

Prior to this change if CURL_GLOBAL_SSL was not set then no SSL
initialization or de-initialization would take place in libcurl,
regardless of whether it was a libcurl initialization routine (ie code
we always must run) or an SSL library-specific initialization routine
(ie code the user can choose to run themselves by not setting
CURL_GLOBAL_SSL). By passing the global init flags to the ssl init
function we can now handle those things separately.

Note for two of the SSL backends SChannel (WinSSL) and NSS, libcurl must
handle the SSL library-specific part of the initialization. If libcurl
is built against one of those backends and CURL_GLOBAL_SSL is not set
then curl_global_init will return error CURLE_FAILED_INIT.

Closes curl#2083
Closes curl#2089
Closes curl#2107
Closes curl#2112

jay added a commit to jay/curl that referenced this pull request Nov 25, 2017

SSL: Call SSL initialization and de-initialization always
- Call SSL initialization even when CURL_GLOBAL_SSL is not set as a
  global init flag. For backwards compatibility the SSL library-specific
  part of the initialization (ie initialization routines not in libcurl)
  will only be run if CURL_GLOBAL_SSL is set.

Prior to this change if CURL_GLOBAL_SSL was not set then no SSL
initialization or de-initialization would take place in libcurl,
regardless of whether it was a libcurl initialization routine (ie code
we always must run) or an SSL library-specific initialization routine
(ie code the user can choose to run themselves by not setting
CURL_GLOBAL_SSL). By passing the global init flags to the ssl init
function we can now handle those things separately.

Note for two of the SSL backends SChannel (WinSSL) and NSS, libcurl must
handle the SSL library-specific part of the initialization. If libcurl
is built against one of those backends and CURL_GLOBAL_SSL is not set
then curl_global_init will return error CURLE_FAILED_INIT.

Closes curl#2083
Closes curl#2089
Closes curl#2107
Closes curl#2112
lib/url.c Outdated
@@ -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.

@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.

@bagder bagder closed this in d661b0a Nov 27, 2017

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