From 9a64e62f0f20c9cf9b2e1609f037060eb2d8eb22 Mon Sep 17 00:00:00 2001 From: Etienne Samson Date: Wed, 21 Dec 2016 21:24:33 +0100 Subject: [PATCH 1/7] http: check certificate validity before clobbering the error variable --- src/transports/http.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/transports/http.c b/src/transports/http.c index ad28c5889..155fd7b30 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -624,13 +624,12 @@ static int http_connect(http_subtransport *t) if ((!error || error == GIT_ECERTIFICATE) && t->owner->certificate_check_cb != NULL && git_stream_is_encrypted(t->io)) { git_cert *cert; - int is_valid; + int is_valid = (error == GIT_OK); if ((error = git_stream_certificate(&cert, t->io)) < 0) return error; giterr_clear(); - is_valid = error != GIT_ECERTIFICATE; error = t->owner->certificate_check_cb(cert, is_valid, t->connection_data.host, t->owner->message_cb_payload); if (error < 0) { From 98d66240ecb7765e191da19b535c75c92ccc90fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jan 2017 10:51:31 +0000 Subject: [PATCH 2/7] http: perform 'badssl' check also via certificate callback Make sure that the callbacks do also get a 'valid' value of zero when the certificate we're looking at is in valid and assert that within the test. --- tests/online/badssl.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/online/badssl.c b/tests/online/badssl.c index 66b090df4..f3470f6d7 100644 --- a/tests/online/badssl.c +++ b/tests/online/badssl.c @@ -10,37 +10,66 @@ static bool g_has_ssl = true; static bool g_has_ssl = false; #endif +static int cert_check_assert_invalid(git_cert *cert, int valid, const char* host, void *payload) +{ + GIT_UNUSED(cert); GIT_UNUSED(host); GIT_UNUSED(payload); + + cl_assert_equal_i(0, valid); + + return GIT_ECERTIFICATE; +} + void test_online_badssl__expired(void) { + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + opts.fetch_opts.callbacks.certificate_check = cert_check_assert_invalid; + if (!g_has_ssl) cl_skip(); cl_git_fail_with(GIT_ECERTIFICATE, git_clone(&g_repo, "https://expired.badssl.com/fake.git", "./fake", NULL)); + + cl_git_fail_with(GIT_ECERTIFICATE, + git_clone(&g_repo, "https://expired.badssl.com/fake.git", "./fake", &opts)); } void test_online_badssl__wrong_host(void) { + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + opts.fetch_opts.callbacks.certificate_check = cert_check_assert_invalid; + if (!g_has_ssl) cl_skip(); cl_git_fail_with(GIT_ECERTIFICATE, git_clone(&g_repo, "https://wrong.host.badssl.com/fake.git", "./fake", NULL)); + cl_git_fail_with(GIT_ECERTIFICATE, + git_clone(&g_repo, "https://wrong.host.badssl.com/fake.git", "./fake", &opts)); } void test_online_badssl__self_signed(void) { + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + opts.fetch_opts.callbacks.certificate_check = cert_check_assert_invalid; + if (!g_has_ssl) cl_skip(); cl_git_fail_with(GIT_ECERTIFICATE, git_clone(&g_repo, "https://self-signed.badssl.com/fake.git", "./fake", NULL)); + cl_git_fail_with(GIT_ECERTIFICATE, + git_clone(&g_repo, "https://self-signed.badssl.com/fake.git", "./fake", &opts)); } void test_online_badssl__old_cipher(void) { + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + opts.fetch_opts.callbacks.certificate_check = cert_check_assert_invalid; + if (!g_has_ssl) cl_skip(); cl_git_fail(git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", NULL)); + cl_git_fail(git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", &opts)); } From 66e3774d279672ee51c3b54545a79d20d1ada834 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 15 Nov 2016 11:36:27 +0100 Subject: [PATCH 3/7] smart_pkt: verify packet length exceeds PKT_LEN_SIZE Each packet line in the Git protocol is prefixed by a four-byte length of how much data will follow, which we parse in `git_pkt_parse_line`. The transmitted length can either be equal to zero in case of a flush packet or has to be at least of length four, as it also includes the encoded length itself. Not checking this may result in a buffer overflow as we directly pass the length to functions which accept a `size_t` length as parameter. Fix the issue by verifying that non-flush packets have at least a length of `PKT_LEN_SIZE`. --- src/transports/smart_pkt.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 2297cc94f..6fe53b931 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -427,6 +427,14 @@ int git_pkt_parse_line( if (bufflen > 0 && bufflen < (size_t)len) return GIT_EBUFS; + /* + * The length has to be exactly 0 in case of a flush + * packet or greater than PKT_LEN_SIZE, as the decoded + * length includes its own encoded length of four bytes. + */ + if (len != 0 && len < PKT_LEN_SIZE) + return GIT_ERROR; + line += PKT_LEN_SIZE; /* * TODO: How do we deal with empty lines? Try again? with the next From 2fdef641fd0dd2828bd948234ae86de75221a11a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 15 Nov 2016 11:44:51 +0100 Subject: [PATCH 4/7] smart_pkt: treat empty packet lines as error The Git protocol does not specify what should happen in the case of an empty packet line (that is a packet line "0004"). We currently indicate success, but do not return a packet in the case where we hit an empty line. The smart protocol was not prepared to handle such packets in all cases, though, resulting in a `NULL` pointer dereference. Fix the issue by returning an error instead. As such kind of packets is not even specified by upstream, this is the right thing to do. --- src/transports/smart_pkt.c | 10 +++++----- src/transports/smart_protocol.c | 11 ----------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 6fe53b931..e05196cd8 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -437,13 +437,13 @@ int git_pkt_parse_line( line += PKT_LEN_SIZE; /* - * TODO: How do we deal with empty lines? Try again? with the next - * line? + * The Git protocol does not specify empty lines as part + * of the protocol. Not knowing what to do with an empty + * line, we should return an error upon hitting one. */ if (len == PKT_LEN_SIZE) { - *head = NULL; - *out = line; - return 0; + giterr_set_str(GITERR_NET, "Invalid empty packet"); + return GIT_ERROR; } if (len == 0) { /* Flush pkt */ diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c index 53c0b089e..db6a8b9c8 100644 --- a/src/transports/smart_protocol.c +++ b/src/transports/smart_protocol.c @@ -763,14 +763,6 @@ static int add_push_report_sideband_pkt(git_push *push, git_pkt_data *data_pkt, line_len -= (line_end - line); line = line_end; - /* When a valid packet with no content has been - * read, git_pkt_parse_line does not report an - * error, but the pkt pointer has not been set. - * Handle this by skipping over empty packets. - */ - if (pkt == NULL) - continue; - error = add_push_report_pkt(push, pkt); git_pkt_free(pkt); @@ -825,9 +817,6 @@ static int parse_report(transport_smart *transport, git_push *push) error = 0; - if (pkt == NULL) - continue; - switch (pkt->type) { case GIT_PKT_DATA: /* This is a sideband packet which contains other packets */ From a5cf255b471ad7113247d552d5695db0cb720882 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Fri, 6 Jan 2017 17:15:53 +0000 Subject: [PATCH 5/7] Bump version to 0.25.1 --- include/git2/version.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/git2/version.h b/include/git2/version.h index 0df191f4f..d190893fe 100644 --- a/include/git2/version.h +++ b/include/git2/version.h @@ -7,10 +7,10 @@ #ifndef INCLUDE_git_version_h__ #define INCLUDE_git_version_h__ -#define LIBGIT2_VERSION "0.25.0" +#define LIBGIT2_VERSION "0.25.1" #define LIBGIT2_VER_MAJOR 0 #define LIBGIT2_VER_MINOR 25 -#define LIBGIT2_VER_REVISION 0 +#define LIBGIT2_VER_REVISION 1 #define LIBGIT2_VER_PATCH 0 #define LIBGIT2_SOVERSION 25 From 3829ba2e710553893faf6336cc6b2f3fc17a293e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 9 Jan 2017 17:50:17 +0000 Subject: [PATCH 6/7] http: correct the expected error for RC4 We must make sure that we're getting a certificate error from the library so we know that we're testing the right thing. --- tests/online/badssl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/online/badssl.c b/tests/online/badssl.c index f3470f6d7..6dacc18b6 100644 --- a/tests/online/badssl.c +++ b/tests/online/badssl.c @@ -70,6 +70,8 @@ void test_online_badssl__old_cipher(void) if (!g_has_ssl) cl_skip(); - cl_git_fail(git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", NULL)); - cl_git_fail(git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", &opts)); + cl_git_fail_with(GIT_ECERTIFICATE, + git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", NULL)); + cl_git_fail_with(GIT_ECERTIFICATE, + git_clone(&g_repo, "https://rc4.badssl.com/fake.git", "./fake", &opts)); } From 2ac57aa89bde788173b54bd153430369deec64c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 9 Jan 2017 17:53:21 +0000 Subject: [PATCH 7/7] https: don't test that RC4 is invalid None of our crypto backends actually reject RC4 as a cipher so don't test for it and instead keep it as something we'd like to do. --- tests/online/badssl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/online/badssl.c b/tests/online/badssl.c index 6dacc18b6..aa4c24d9c 100644 --- a/tests/online/badssl.c +++ b/tests/online/badssl.c @@ -67,6 +67,9 @@ void test_online_badssl__old_cipher(void) git_clone_options opts = GIT_CLONE_OPTIONS_INIT; opts.fetch_opts.callbacks.certificate_check = cert_check_assert_invalid; + /* FIXME: we don't actually reject RC4 anywhere, figure out what to tweak */ + cl_skip(); + if (!g_has_ssl) cl_skip();