Created attachment 244174 [details] [review]
eds patch

for evoluton-data-server;

This should make it on eds side, it'll re-ask for certificate trust as long as the response should have it set. I can successfully connect to Google and Yahoo calendars, thus it might not break anything for "good" servers.

Created attachment 244175 [details] [review]
evo patch

for evolution;

evo's part.

Are you able to test this, please (the eds part is enough)? I tried your test suite, but I often get that the eds cannot connect to destination, thus it's hard to test it fully for me. I also tried to see whether the password is exposed, it wasn't during my testing, but I didn't receive much response from the .ics file either.

Created attachment 244270 [details]
Test kit, connection errors believed to be fixed

> I tried your test
> suite, but I often get that the eds cannot connect to destination, thus it's
> hard to test it fully for me.

I think I see the cause: Evolution sometimes opens several connections at the same time and my test server accepts only one at a time.  I changed the test server (new kit attached) to accept one connection with test-cert1.pem, and once that finishes, an unlimited number with test-cert2.pem.  That seems to be sufficient to test the bug.

> I also tried to see whether the password is
> exposed, it wasn't during my testing

My test case was bad.  I was misled by the fact that Evolution prompts the user for a password unconditionally; nevertheless, Evolution sends the password to the server only if the server requires authentication.  So I changed my server to require authentication.  Update to the steps to reproduce:

3. Go to the Calendar window and create a new calendar: "On the Web", URL
https://localhost:4433/private/login-test/sample.ics (NOTE the addition of "login-test/"), username "user", and save.
4. When prompted for a password, enter "pass".

With the new test case, my original build of Evolution from the gnome-3-8 branch exposes the password ("Authorization: Basic dXNlcjpwYXNz", base64 -d <<<'dXNlcjpwYXNz' gives user:pass).  Evolution with your patches still exposes the password; the only difference is that it shows another trust prompt, and if I reject, the returned calendar data is not displayed.

To completely fix the problem, Evolution needs to verify that the connection is using the correct certificate _before_ sending the request containing the password over that connection.  I don't see any way to do this without a new API in libsoup.

Created attachment 246001 [details]
test-cal.c

My test-cal.c, to make it easier to test calendar opening without using evolution. It uses a source UID, which is configured in evolution. I'm currently with libsoup-2.42.2-1.fc19.x86_64, and I'm unable to reproduce this. I usually get a "Cancelled" error, which comes from libsoup itself, when the "server" changes to the second certificate. It's on the re-send of the message after initial "SLL handshake failed" error.

There is one possibility to check for SSL certificates before sending authentication, which would look like, in e-cal-backend-http.c:

static void
soup_authenticate (SoupSession *session,
                   SoupMessage *msg,
                   SoupAuth *auth,
                   gboolean retrying,
                   gpointer data)
{
	ECalBackendHttp *cbhttp;
	ESourceAuthentication *auth_extension;
	ESourceWebdav *webdav_extension;
	ESource *source;
	const gchar *extension_name;

	cbhttp = E_CAL_BACKEND_HTTP (data);

	source = e_backend_get_source (E_BACKEND (data));
	extension_name = E_SOURCE_EXTENSION_AUTHENTICATION;
	auth_extension = e_source_get_extension (source, extension_name);

	webdav_extension = e_source_get_extension (source, E_SOURCE_EXTENSION_WEBDAV_BACKEND);
	printf ("   %p: %s, trust matches:%d\n", cbhttp, __FUNCTION__, e_source_webdav_ssl_trust_matches (webdav_extension, msg));

	if (!retrying && cbhttp->priv->password != NULL) {
		gchar *user;

		user = e_source_authentication_dup_user (auth_extension);
		soup_auth_authenticate (auth, user, cbhttp->priv->password);
		g_free (user);
	}
}

It only prints on the evolution-calendar-factory console whether the trust on the source matches the certificate on the 'msg', while it can, if the result is '0', simply reject send of the credentials. Unfortunately, as I said above, I cannot reproduce this, probably due to libsoup changes.

I'm marking patches as review, to get them out of the unreviewed patches search. I'm also reasking for testing them, before committing.

Created attachment 270357 [details]
Test kit version 3

I had lost interest in this bug once I realized I wouldn't be able to use the SSL exception logic anyway because I don't want to allow the system CA file for the connection to my private calendar server, but coming back now to get it fixed for the rest of the world.

(In reply to comment #4)
> I usually get a "Cancelled" error, which comes from libsoup itself, when the
> "server" changes to the second certificate. It's on the re-send of the message
> after initial "SLL handshake failed" error.

I saw a similar thing, and with the help of tcpdump I think I finally found the problem: Evolution tries the second connection before the second socat command has a chance to bind to the port again.  I fixed the test kit to bind the port once and, when it gets a connection, send it to one of two listening SSL servers.  Hopefully you can reproduce the behaviors now.

Using evolution and evolution-data-server from HEAD plus the attached patches against libsoup-2.44.2-1.fc20.x86_64, I see the following behavior:

1. First connection aborted without sending request
2. Trust prompt for first certificate (I clicked Accept Permanently)
3. Connection under second certificate with no authorization, returns 401
4. Password prompt
5. Another connection under second certificate with no authorization, returns 401
6. Connection under second certificate with authorization, returns data
7. Calendar event is displayed

I added some print statements, and it looks like in step 3, "SOUP_STATUS_IS_SUCCESSFUL (status_code)" with status_code == 401 is returning false and the code that checks for a changed certificate is bypassed.  I think the handling of password authorization changed since my test in comment #3.

> There is one possibility to check for SSL certificates before sending
> authentication, which would look like, in e-cal-backend-http.c: [...]

Clever.  I tried this code and it indeed prints 0.  This will at least prevent passwords from being leaked without requiring new API in libsoup.  It's still a bug that the URL is leaked, but users using SSL exceptions at least have the option to use a password instead of a secret URL, assuming they know about the issue.

(In reply to comment #6)
> Test kit version 3

I think I can reproduce it too with this kit.

> I think the handling of password authorization changed since my test
> in comment #3.

Hmm, it really seems to, because if I add the change as described in comment #4, and eventually reject password send if the trust doesn't match (supposing there is any filled), then with the test kit I never get past this point, because the SSL trust has set cert-1, but the rest of the attempts use cert-2. Your observation with !successful status_code when it's 401 seems to be correct, and I can fix the just described behaviour when also deal with 401 in a different way. It'll require more "backend-local-property" variables to mark the state as "this failed because the SSL failed when I was about to send a password", but that might not be a problem. I will update patches for the post 3.12.0 version (as you lost interest, and this brings a bit of risk with itself, I rather postpone the changes to 3.13.x).

Waiting for 3.13.x sounds good.

We may not be off so easy only omitting the password if the certificate is wrong.  The CardDAV and CalDAV backends can still upload an entire object to a malicious server.  There might be a way you could avoid passing the request body to libsoup until after you have a chance to check the certificate, but please reconsider the 100% solution of adding API to libsoup to let you halt sending the request if the certificate is wrong.  (Epiphany would use this too if it ever implements real SSL certificate enforcement with libsoup, see bug 639764.)  This would amount to adding an "accept-certificate" signal to the soup session and somehow plumbing soup_socket_accept_certificate to it.

(In reply to comment #8)
> We may not be off so easy only omitting the password if the certificate is
> wrong.  The CardDAV and CalDAV backends can still upload an entire object to a
> malicious server.  There might be a way you could avoid passing the request
> body to libsoup until after you have a chance to check the certificate, but
> please reconsider the 100% solution of adding API to libsoup to let you halt
> sending the request if the certificate is wrong.  (Epiphany would use this too
> if it ever implements real SSL certificate enforcement with libsoup, see bug
> 639764.)  This would amount to adding an "accept-certificate" signal to the
> soup session and somehow plumbing soup_socket_accept_certificate to it.

Well, better to open a bug for libsoup, or ask Dan (CC'ed now).

Yeah, it would be nice if libsoup had API for this.

For now, you can connect to "network-event" on the message, and do appropriate things to the GTlsConnection at the G_SOCKET_CLIENT_TLS_HANDSHAKING stage... eg, set its :database to NULL to force the handshake to fail, and connect to "accept-certificate" on it and do your own checking from there.

Hm... it would be nice if glib-networking had API to say "only accept this specific pinned certificate" too...

Created attachment 272375 [details] [review]
eds patch ][

for evolution-data-server;

An updated patch. It changes couple things:
a) the newly added e_source_webdav_ssl_trust_matches() returns TRUE also for
   cases when the used certificate on the connection is trusted (has no
   errors), without checking whether it's in the stored trust or not (it's
   because the stored trust is for incorrect certificates only).
b) if an incorrect certificate is used during the authentication signal, then
   the authentication doesn't change its state and the SoupMessage has
   set SOUP_STATUS_SSL_FAILED, which is the correct failure status, after all.
   The side-effect of this change is that the user is asked about his
   opinion on the second certificate trust again, thus he/she sees what is
   actually going on in the background.

Created attachment 272376 [details] [review]
evo patch ][

for evolution;

Updated version with corresponding changes.

(In reply to comment #10)
> For now, you can connect to "network-event" on the message, and do appropriate
> things to the GTlsConnection at the G_SOCKET_CLIENT_TLS_HANDSHAKING stage...
> eg, set its :database to NULL to force the handshake to fail, and connect to
> "accept-certificate" on it and do your own checking from there.

I tried the accept-certificate variant of the approach, moving the code from the "if (status_code == SOUP_STATUS_SSL_FAILED)" block in cal_backend_http_load to the accept-certificate callback.  I get the following results from the steps to reproduce:

1. The trust prompt with the first certificate appears: accept.
2. Password prompt: enter the password and click Continue.  The Continue button becomes disabled and the progress spinner appears next to it.
3. The trust prompt with the second certificate appears behind the system-modal password prompt.  Cancel the password prompt and then reject the trust prompt.
4. No request is sent to the server under the second certificate.

This solves the security problem.  The poor usability doesn't matter for an attack case.

One concern: we would need to make sure the accept-certificate callback is not called after the load attempt completes and the SoupMessage and so forth are destroyed.  It looks like everything runs in the same thread as cal_backend_http_load, so it may work to make a closure and invalidate the closure when the load attempt completes.

Milan, would you like me to provide a patch?  If so, what do you think about putting the code into e-source-webdav.c rather than duplicating it 6 times?



I did notice that in step 3, if instead of canceling the password prompt I wait 2 minutes, the prompt disappears by itself (I imagine this is the INACTIVITY_TIMEOUT in e-authentication-mediator.c), and then when I reject the trust prompt, evolution-calendar-factory crashes as follows:

Program received signal SIGSEGV, Segmentation fault.
g_wakeup_signal (wakeup=0x38) at gwakeup.c:235
235	  if (wakeup->fds[1] == -1)
(gdb) bt

+ Trace 233385

Looks like the following is happening:
1. After the 2-minute timeout, the authentication mediator signals a server error.
2. When the trust prompt is rejected, the load attempt in source_registry_authenticate_authenticate_cb completes and a call to source_registry_authenticate_respond_cb is scheduled.
3. The server error is processed and the auth_context is freed.
4. The "Make sure the main_context doesn't have pending operations" code calls source_registry_authenticate_respond_cb, which tries to access the freed auth_context.

I'm not sure how to fix this, but it doesn't bother me any more than the other crashing bugs in Evolution.  :/

> Hm... it would be nice if glib-networking had API to say "only accept this
> specific pinned certificate" too...

My opinion FWIW: Callers may know only the hash of the certificate (e.g., the existing code in Evolution stores the SHA-1) and it will be a mess to support different hash functions.  Better to just expose the accept-certificate signal, or conceivably provide a system-wide certificate exception manager like Mozilla PSM.

When I was testing this the "accept-certificate" didn't seem to be necessary, the behaviour seemed correct to me even without it. I mean, are you sure that is really needed?

The crash, hmm, that might need a new bug report.

Created attachment 272829 [details]
Test kit, switch to second certificate on fourth connection

> When I was testing this the "accept-certificate" didn't seem to be necessary,
> the behaviour seemed correct to me even without it. I mean, are you sure that
> is really needed?

I assumed this would be necessary to avoid the CardDAV and CalDAV backends uploading a contact or calendar object to a malicious server, as mentioned in comment #8.  It would take more work to write steps to reproduce for this case.

However, I found a simpler case in which the current patch fails: the test server switches to the second certificate starting with the fourth connection (see the new test kit).  IIUC, soup_authenticate runs after the third connection (so it sees the first certificate) and then the password is saved in the libsoup auth manager and sent automatically by libsoup on later requests, including on the fourth connection.

(In reply to comment #15)
> I assumed this would be necessary to avoid the CardDAV and CalDAV backends
> uploading a contact or calendar object to a malicious server, as mentioned in
> comment #8.  It would take more work to write steps to reproduce for this case.

OK

(In reply to comment #13)
> Milan, would you like me to provide a patch?  If so, what do you think about
> putting the code into e-source-webdav.c rather than duplicating it 6 times?

Yes, please :) I'm for the code movement into the e-source-webdav, code duplication is always bad (I didn't do that initially, because the code seemed to have some special handling and little differences to me, but I see now that I was wrong, so I'm definitely for the code duplication removal).

Created attachment 273567 [details] [review]
Patch using accept-certificate (evolution-data-server)

Finished the patches.  My employer, Google, may claim a copyright to the patches but has given permission for me to submit them.

Since the shared code has a dependency on the user prompter, I put it in a new file in libebackend instead of e-source-webdav.c.  I've tried to polish the patches in all the obvious ways, but I have likely missed some things.  Feel free to change anything that is not to your liking.

I included two orthogonal changes:
- It looked like owncloud-utils passed NULL for the registry but e_source_webdav_prepare_ssl_trust_prompt would fail in that case; I didn't try it though.  Changed e_source_webdav_prepare_ssl_trust_prompt to just not look up the parent source in that case.
- Remove the setting of "Accept Temporarily" as the default button in the trust prompt, so there is now no default.  I know there are politics here; I'd argue that the current state does not allow users who want security to get it, since the prompt could steal the focus while the user was attempting to press Enter in another window and be unintentionally accepted.  (At least the prompt steals focus in my test environment with Xfce and Fedora 20.)

Do you know whether the non-threadsafe invalidation of the accept-certificate closure is OK?  I.e., could there be a race between the disposal of the SoupMessage and an operation on one of the GTlsConnections?



In stress testing, I saw another crash in evolution-calendar-factory.  Rough steps to reproduce:

1. Modify distributor-cmd to always go to the first certificate.
2. Start the test server and create the calendar source.
3. "Accept temporarily" and enter the password.
4. Unset trust.
5. Refresh the calendar and "Accept temporarily".
6. Repeat steps 4-5 several times.

One thread called g_main_context_push_thread_default, which failed with the following stack trace, but it later called g_main_context_pop_thread_default anyway, corrupting the main context's owner count:

+ Trace 233432

If/when my patches are committed, I'll open separate bugs for the two crashes.

Created attachment 273568 [details] [review]
Patch using accept-certificate (evolution)

Thanks for the patches, it's very neat and makes the code much simpler. I didn't do many changes in it, though some are "significant", like I changed the new function name (to conform to its source file name) and I moved the SoupMessage parameter as the first, because that's the important parameter. I also removed debug prints, fixed license information block and some other really minor changes. None of my changes to your code isn't anything bigger than minor change. The commits are:

Created commit 64875ea in eds master (3.13.1+) [1]
Created commit 192a2f7 in evo master (3.13.1+) [2]

[1] https://git.gnome.org/browse/evolution-data-server/commit/?id=64875ea
[2] https://git.gnome.org/browse/evolution/commit/?id=192a2f7

(In reply to comment #17)
> - It looked like owncloud-utils passed NULL for the registry but
> e_source_webdav_prepare_ssl_trust_prompt would fail in that case; I didn't try
> it though.  Changed e_source_webdav_prepare_ssl_trust_prompt to just not look
> up the parent source in that case.

Oops, thanks for the fix, that's my fault, an overlook.

> 4. Unset trust.
> 5. Refresh the calendar and "Accept temporarily".
> 6. Repeat steps 4-5 several times.

It sounds like a bug in the Refresh handling.

> Do you know whether the non-threadsafe invalidation of the accept-certificate
> closure is OK?  I.e., could there be a race between the disposal of the
> SoupMessage and an operation on one of the GTlsConnections?

I do not have any idea, let's ask Dan.

Dan?

(In reply to comment #20)
> > Do you know whether the non-threadsafe invalidation of the accept-certificate
> > closure is OK?  I.e., could there be a race between the disposal of the
> > SoupMessage and an operation on one of the GTlsConnections?
> 
> I do not have any idea, let's ask Dan.
> 
> Dan?

Assuming you're not illegally using objects from multiple threads, I don't think there are any thread safety issues.

But it's possible you could end up with two messages both connected to the same GTlsConnection in some cases if messages don't get destroyed promptly for some reason. Eg, if you send one request, do accept-certificate handling, complete the request, and then send a second request, and the server requests a rehandshake at that point (before the first message has been destroyed). Then both messages would try to process the accept-certificate.

Another theoretical possibility is that if the message gets cancelled before the TLS handshake occurs, the connection might get assigned to another message later, and then get handshaked for that message. (That can't currently happen, but maybe it could in the future.)

I think that doing the handler teardown from SoupMessage::finished as well as from the weak notify should cover both of those cases.

(In reply to comment #19)
> Created commit 64875ea in eds master (3.13.1+) [1]
> Created commit 192a2f7 in evo master (3.13.1+) [2]

Follow-up eds commit fa74532, the given connection is not always a GTlsConnection, thus check for it before adding the signal handler for "accept-certificate".

(In reply to comment #22)
> Follow-up eds commit fa74532, the given connection is not always a
> GTlsConnection, thus check for it before adding the signal handler for
> "accept-certificate".

That would be a bug in glib... the connection should always be a GTlsClientConnection if event is G_SOCKET_CLIENT_TLS_HANDSHAKING.

(and I can't see how that would happen... there are two points where gsocketclient.c emits the signal with event=HANDSHAKING, and in both cases, the connection is the return value from g_tls_client_connection_new().)