Hey, > You are right, it should not have been touched. The new logic has > swapped buf and data so just revert all the changes to the method > (|data| should not be const...). > > I will investigate why this was not detected by the existing test cases.
OK, I managed to install a krb5+ftp server and tried cURL on it. It looks like the feature was broken even before my changes (I suspect a change in late 2009 but I did not bisect). This means that our coverage is inexistent (sorry for pushing untested changes I should have seen that! :-/). Is there some documentation about how to integrate new features into our test harness? (I really would like to see the code tested to prevent further regressions - including memory leaks) I have fixed my changes and made ftp + krb5-gssapi work again. Attached are the several changes needed to get everything working again. I am still investigating some memory leaks related to gssapi (we never free the gss_context_id_t). I will post the changes once I have figured out who is the culprit of some remaining leaks. Comments appreciated! Julien
From ca23be0a161593548158fb04780fbe07b5293a62 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 16:17:01 -0700 Subject: [PATCH 1/7] security.c: Fix ftp_send_command. My use of va_args was completely wrong. Fixed the usage so that we send the right commands! --- lib/security.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/security.c b/lib/security.c index 693be3f..5708078 100644 --- a/lib/security.c +++ b/lib/security.c @@ -128,9 +128,13 @@ static int ftp_send_command(struct connectdata *conn, const char *message, ...) int ftp_code; ssize_t nread; va_list args; + char print_buffer[50]; va_start(args, message); - if(Curl_ftpsendf(conn, message, args) != CURLE_OK) { + vsnprintf(print_buffer, sizeof(print_buffer), message, args); + va_end(args); + + if(Curl_ftpsendf(conn, print_buffer) != CURLE_OK) { ftp_code = -1; } else { @@ -139,7 +143,6 @@ static int ftp_send_command(struct connectdata *conn, const char *message, ...) } (void)nread; /* Unused */ - va_end(args); return ftp_code; } -- 1.7.0.4
From 6d026dc730aac21221be8f13647ecab67c6c0d0f Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 17:57:03 -0700 Subject: [PATCH 2/7] security.c: Fix typo (PSBZ -> PBSZ) --- lib/security.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/lib/security.c b/lib/security.c index 5708078..3fda8bb 100644 --- a/lib/security.c +++ b/lib/security.c @@ -432,7 +432,7 @@ int Curl_sec_set_protection_level(struct connectdata *conn) return 0; if(level) { - code = ftp_send_command(conn, "PSBZ %u", buffer_size); + code = ftp_send_command(conn, "PBSZ %u", buffer_size); if(code < 0) return -1; -- 1.7.0.4
From 85f164bfea7d420f64400d4e4cfa9a1d78d3c4a6 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 18:04:48 -0700 Subject: [PATCH 3/7] security.c: Readd the '\n' to the infof() calls. They are not automatically added and make the output of the verbose mode a lot more readable. --- lib/security.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/security.c b/lib/security.c index 3fda8bb..25cd483 100644 --- a/lib/security.c +++ b/lib/security.c @@ -305,7 +305,7 @@ static void do_sec_send(struct connectdata *conn, curl_socket_t fd, socket_write(conn, fd, cmd_buffer, bytes); socket_write(conn, fd, "\r\n", 2); - infof(conn->data, "Send: %s%s", prot_level == prot_private?enc:mic, + infof(conn->data, "Send: %s%s\n", prot_level == prot_private?enc:mic, cmd_buffer); free(cmd_buffer); } @@ -423,7 +423,7 @@ int Curl_sec_set_protection_level(struct connectdata *conn) if(!conn->sec_complete) { infof(conn->data, "Trying to change the protection level after the" - "completion of the data exchange."); + "completion of the data exchange.\n"); return -1; } @@ -492,7 +492,7 @@ static CURLcode choose_mech(struct connectdata *conn) /* We have no mechanism with a NULL name but keep this check */ DEBUGASSERT(mech_name != NULL); if(mech_name == NULL) { - infof(data, "Skipping mechanism with empty name (%p)", mech); + infof(data, "Skipping mechanism with empty name (%p)\n", mech); continue; } tmp_allocation = realloc(conn->app_data, (*mech)->size); @@ -506,12 +506,12 @@ static CURLcode choose_mech(struct connectdata *conn) if((*mech)->init) { ret = (*mech)->init(conn); if(ret != 0) { - infof(data, "Failed initialization for %s. Skipping it.", mech_name); + infof(data, "Failed initialization for %s. Skipping it.\n", mech_name); continue; } } - infof(data, "Trying mechanism %s...", mech_name); + infof(data, "Trying mechanism %s...\n", mech_name); ret = ftp_send_command(conn, "AUTH %s", mech_name); if(ret < 0) /* FIXME: This error is too generic but it is OK for now. */ @@ -521,15 +521,15 @@ static CURLcode choose_mech(struct connectdata *conn) switch(ret) { case 504: infof(data, "Mechanism %s is not supported by the server (server " - "returned ftp code: 504).", mech_name); + "returned ftp code: 504).\n", mech_name); break; case 534: infof(data, "Mechanism %s was rejected by the server (server returned " - "ftp code: 534).", mech_name); + "ftp code: 534).\n", mech_name); break; default: if(ret/100 == 5) { - infof(data, "The server does not support the security extensions."); + infof(data, "The server does not support the security extensions.\n"); return CURLE_USE_SSL_FAILED; } break; -- 1.7.0.4
From 590a3f40014f9f07d4dc00f7d440b64d3bc48835 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 19:14:50 -0700 Subject: [PATCH 4/7] security.c: Fix Curl_sec_login after rewrite. Curl_sec_login was returning the opposite result that the code in ftp.c was expecting. Simplified the return code (using a CURLcode) so to see more clearly what is going on. --- lib/ftp.c | 2 +- lib/krb4.h | 2 +- lib/security.c | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/ftp.c b/lib/ftp.c index 3ea1dd3..e3f9387 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -2424,7 +2424,7 @@ static CURLcode ftp_statemach_act(struct connectdata *conn) set a valid level */ Curl_sec_request_prot(conn, data->set.str[STRING_KRB_LEVEL]); - if(Curl_sec_login(conn) != 0) + if(Curl_sec_login(conn) != CURLE_OK) infof(data, "Logging in with password in cleartext!\n"); else infof(data, "Authentication successful\n"); diff --git a/lib/krb4.h b/lib/krb4.h index 29a2578..5dc3971 100644 --- a/lib/krb4.h +++ b/lib/krb4.h @@ -58,7 +58,7 @@ int Curl_sec_fprintf2(struct connectdata *conn, FILE *f, const char *fmt, ...); int Curl_sec_vfprintf2(struct connectdata *conn, FILE *, const char *, va_list); void Curl_sec_end (struct connectdata *); -int Curl_sec_login (struct connectdata *); +CURLcode Curl_sec_login (struct connectdata *); void Curl_sec_prot (int, char **); int Curl_sec_request_prot (struct connectdata *conn, const char *level); int Curl_sec_set_protection_level(struct connectdata *conn); diff --git a/lib/security.c b/lib/security.c index 25cd483..9e74eb2 100644 --- a/lib/security.c +++ b/lib/security.c @@ -566,11 +566,10 @@ static CURLcode choose_mech(struct connectdata *conn) return mech != NULL ? CURLE_OK : CURLE_FAILED_INIT; } -int +CURLcode Curl_sec_login(struct connectdata *conn) { - CURLcode code = choose_mech(conn); - return code == CURLE_OK; + return choose_mech(conn); } -- 1.7.0.4
From e718825a4874f654bfc11a8c888d3dbeed154919 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 19:16:38 -0700 Subject: [PATCH 5/7] security.c: We should always register the socket handler. Following a change in the way socket handler are registered, the custom recv and send method were conditionaly registered. We need to register them everytime to handle the ftp security extensions. Re-added the clear text handling in sec_recv. --- lib/security.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/security.c b/lib/security.c index 9e74eb2..5becb0c 100644 --- a/lib/security.c +++ b/lib/security.c @@ -245,6 +245,10 @@ static ssize_t sec_recv(struct connectdata *conn, int sockindex, *err = CURLE_OK; + /* Handle clear text response. */ + if(conn->sec_complete == 0 || conn->data_prot == prot_clear) + return read(fd, buffer, len); + if(conn->in_buffer.eof_flag) { conn->in_buffer.eof_flag = 0; return 0; @@ -550,12 +554,10 @@ static CURLcode choose_mech(struct connectdata *conn) conn->mech = *mech; conn->sec_complete = 1; - if (conn->data_prot != prot_clear) { - conn->recv[FIRSTSOCKET] = sec_recv; - conn->send[FIRSTSOCKET] = sec_send; - conn->recv[SECONDARYSOCKET] = sec_recv; - conn->send[SECONDARYSOCKET] = sec_send; - } + conn->recv[FIRSTSOCKET] = sec_recv; + conn->send[FIRSTSOCKET] = sec_send; + conn->recv[SECONDARYSOCKET] = sec_recv; + conn->send[SECONDARYSOCKET] = sec_send; conn->command_prot = prot_safe; /* Set the requested protection level */ /* BLOCKING */ -- 1.7.0.4
From e1919543bd88153406872957dd79a6969cd4bb42 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 22:35:00 -0700 Subject: [PATCH 6/7] security.c: Remove Curl_sec_fflush_fd. The current implementation would make us send wrong data on a closed socket. We don't buffer our data so the method can be safely removed. --- lib/ftp.c | 4 ---- lib/krb4.h | 1 - lib/security.c | 11 ----------- 3 files changed, 0 insertions(+), 16 deletions(-) diff --git a/lib/ftp.c b/lib/ftp.c index e3f9387..f1376d7 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -3076,10 +3076,6 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, /* free the dir tree and file parts */ freedirs(ftpc); -#if defined(HAVE_KRB4) || defined(HAVE_GSSAPI) - Curl_sec_fflush_fd(conn, conn->sock[SECONDARYSOCKET]); -#endif - /* shut down the socket to inform the server we're done */ #ifdef _WIN32_WCE diff --git a/lib/krb4.h b/lib/krb4.h index 5dc3971..5da1dc6 100644 --- a/lib/krb4.h +++ b/lib/krb4.h @@ -47,7 +47,6 @@ extern struct Curl_sec_client_mech Curl_krb5_client_mech; #endif CURLcode Curl_krb_kauth(struct connectdata *conn); -int Curl_sec_fflush_fd(struct connectdata *conn, int fd); int Curl_sec_fprintf (struct connectdata *, FILE *, const char *, ...); int Curl_sec_getc (struct connectdata *conn, FILE *); int Curl_sec_putc (struct connectdata *conn, int, FILE *); diff --git a/lib/security.c b/lib/security.c index 5becb0c..303a1be 100644 --- a/lib/security.c +++ b/lib/security.c @@ -345,17 +345,6 @@ static ssize_t sec_write(struct connectdata *conn, curl_socket_t fd, return tx; } -/* FIXME: fd should be a curl_socket_t */ -int Curl_sec_fflush_fd(struct connectdata *conn, int fd) -{ - if(conn->data_prot == prot_clear) - return 0; - - /* Force a flush by trying to send no data */ - do_sec_send(conn, fd, NULL, 0); - return 0; -} - /* Matches Curl_send signature */ static ssize_t sec_send(struct connectdata *conn, int sockindex, const void *buffer, size_t len, CURLcode *err) -- 1.7.0.4
From 06f432c53ea2336f4453d6aca919f20d5f45f2e6 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Sun, 26 Sep 2010 22:44:42 -0700 Subject: [PATCH 7/7] krb5-gssapi: Remove several memory leaks. Remove a leak seen on Kerberos/MIT (gss_OID is copied internally and we were leaking it). Now we just pass NULL as advised in RFC2744. |tmp| was never set back to buf->data. Cleaned up Curl_sec_end to take into account failure in Curl_sec_login (where conn->mech would be NULL but not conn->app_data or conn->in_buffer->data). --- lib/krb5.c | 4 ++-- lib/security.c | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/krb5.c b/lib/krb5.c index 9fb44f2..28c6a25 100644 --- a/lib/krb5.c +++ b/lib/krb5.c @@ -218,8 +218,8 @@ krb5_auth(void *app_data, struct connectdata *conn) continue; } { - gss_OID t; - gss_display_name(&min, gssname, &gssbuf, &t); + /* We pass NULL as |output_name_type| to avoid a leak. */ + gss_display_name(&min, gssname, &gssbuf, NULL); Curl_infof(data, "Trying against %s\n", gssbuf.value); gss_release_buffer(&min, &gssbuf); } diff --git a/lib/security.c b/lib/security.c index 303a1be..73a5540 100644 --- a/lib/security.c +++ b/lib/security.c @@ -216,6 +216,7 @@ static CURLcode read_data(struct connectdata *conn, if (tmp == NULL) return CURLE_OUT_OF_MEMORY; + buf->data = tmp; ret = socket_read(fd, buf->data, len); if (ret != CURLE_OK) return ret; @@ -567,12 +568,20 @@ Curl_sec_login(struct connectdata *conn) void Curl_sec_end(struct connectdata *conn) { - if(conn->mech != NULL) { - if(conn->mech->end) - conn->mech->end(conn->app_data); + if(conn->mech != NULL && conn->mech->end) + conn->mech->end(conn->app_data); + if(conn->app_data) { free(conn->app_data); conn->app_data = NULL; } + if(conn->in_buffer.data) { + free(conn->in_buffer.data); + conn->in_buffer.data = NULL; + conn->in_buffer.size = 0; + conn->in_buffer.index = 0; + /* FIXME: Is this really needed? */ + conn->in_buffer.eof_flag = 0; + } conn->sec_complete = 0; conn->data_prot = (enum protection_level)0; conn->mech = NULL; -- 1.7.0.4
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
