[Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings
Hi, I fould a potential gotcha when playing with clang's code analysis tool. The concat_strings function silently stopped counting string lengths when given more than 5 arguments. clang warned about potential garbage values in the saved_lengths array, so I redid it with this approach. All tests working ok with this patch. This is my first patch to this list, by the way. I'd be happy to help out more in the future. Best regards, /Pär Karlsson, Sweden commit 2d855670e0e1fbe578506b376cdd40b0e465d3ef Author: Pär Karlsson Date: Thu Oct 16 21:41:36 2014 +0200 Updated ChangeLog diff --git a/src/ChangeLog b/src/ChangeLog index 1c4e2d5..1e39475 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,8 @@ +2014-10-16 Pär Karlsson + + * utils.c (concat_strings): fixed arbitrary limit of 5 arguments to + function + 2014-05-03 Tim Ruehsen * retr.c (retrieve_url): fixed memory leak commit 1fa9ff274dcb6e5a2dbbbc7d3fe2f139059c47f1 Author: Pär Karlsson Date: Wed Oct 15 00:00:31 2014 +0200 Generalized concat_strings argument length The concat_strings function seemed arbitrary to only accept a maximum of 5 arguments (the others were silently ignored). Also it had a potential garbage read of the values in the array. Updated with xmalloc/xrealloc/free diff --git a/src/utils.c b/src/utils.c index 78c282e..93c9ddc 100644 --- a/src/utils.c +++ b/src/utils.c @@ -356,7 +356,8 @@ char * concat_strings (const char *str0, ...) { va_list args; - int saved_lengths[5]; /* inspired by Apache's apr_pstrcat */ + size_t psize = sizeof(int); + int *saved_lengths = xmalloc (psize); char *ret, *p; const char *next_str; @@ -370,8 +371,8 @@ concat_strings (const char *str0, ...) for (next_str = str0; next_str != NULL; next_str = va_arg (args, char *)) { int len = strlen (next_str); - if (argcount < countof (saved_lengths)) -saved_lengths[argcount++] = len; + saved_lengths[argcount++] = len; + xrealloc(saved_lengths, psize * argcount); total_length += len; } va_end (args); @@ -393,7 +394,7 @@ concat_strings (const char *str0, ...) } va_end (args); *p = '\0'; - + free(saved_lengths); return ret; } ^L
[Bug-wget] [PATCH] V2 removed 'auto' SSLv3 also from OpenSSL code
patch V2 - removed SSLv3 from --secure-protocol=auto|pfs (GnuTLS code) - removed SSLv3 from --secure-protocol=auto (OpenSSL code) - amended the docs I am not an OpenSSL expert... please feel free to suggest improvements. Tim Am Donnerstag, 16. Oktober 2014, 20:50:32 schrieb Tim Rühsen: > Am Mittwoch, 15. Oktober 2014, 17:26:49 schrieb Daniel Kahn Gillmor: > > On 10/15/2014 03:10 PM, Tim Rühsen wrote: > > > I tried to make clear that Wget *explicitely* asks for SSLv2 and SSLv3 > > > in > > > the default configuration when compiled with OpenSSL. Whatever the > > > OpenSSL library vendor is doing... it won't affect Wget in this case. So > > > with your attitude, you won't ever be safe ever from Poodle (I guess). > > > > > > And again my question: should we change the default behaviour of future > > > versions of Wget ? > > > With other words: since we know, the library vendor wouldn't help in the > > > above case, what can we do to secure Wget ? > > > > hm, i think Tim is on to something here: by default, wget should use the > > default ciphersuites and protocol versions selected by the TLS library. > > > > Tweaking the default choices in wget itself tends to make wget more > > > > brittle than the underlying library. > > > > The only way that should work to try to improve security in wget via TLS > > implementation preference strings is if the preference string is > > explicitly a minor modification of some system default. This may or may > > not be possible depending on the preference string syntax of the > > selected TLS implementation. > > > > (e.g. [for OpenSSL] if the system default is always explicitly > > referenced as DEFAULT and we decide that we never want wget to use RC4, > > then DEFAULT:-RC4 is a sensible approach, because it allows OpenSSL to > > update DEFAULT and wget gains those improvements automatically) > > Here is a suggestion for a GnuTLS patch. > > I have a look at OpenSSL ciphers and make a similar patch soon. > > I also suggested (~1-2 years ago) an option to directly set priority strings > / ciphers for GnuTLS and OpenSSL. In situations like these, such an option > would allow for a quick reaction done by distribution maintainers and > users. > > What do you think ? > > Tim From bca3e7ea1e430de4fcbc15daad60e8a2953e3a61 Mon Sep 17 00:00:00 2001 From: Tim Ruehsen Date: Thu, 16 Oct 2014 20:44:56 +0200 Subject: [PATCH] do not use SSLv3 except explicitely requested --- doc/ChangeLog | 4 doc/wget.texi | 4 ++-- src/ChangeLog | 5 + src/gnutls.c | 5 +++-- src/openssl.c | 4 +--- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/doc/ChangeLog b/doc/ChangeLog index f055fa5..dd43162 100644 --- a/doc/ChangeLog +++ b/doc/ChangeLog @@ -1,3 +1,7 @@ +2014-10-16 Tim Ruehsen + + * wget.texi (Download Options): update --secure-protocol description + 2014-08-03 Giuseppe Scrivano * wget.texi (Download Options): Fix texinfo warning. diff --git a/doc/wget.texi b/doc/wget.texi index a31eb5e..1e1dd36 100644 --- a/doc/wget.texi +++ b/doc/wget.texi @@ -1643,8 +1643,8 @@ without SSL support, none of these options are available. Choose the secure protocol to be used. Legal values are @samp{auto}, @samp{SSLv2}, @samp{SSLv3}, @samp{TLSv1} and @samp{PFS}. If @samp{auto} is used, the SSL library is given the liberty of choosing the appropriate -protocol automatically, which is achieved by sending an SSLv2 greeting -and announcing support for SSLv3 and TLSv1. This is the default. +protocol automatically, which is achieved by sending an TLSv1 greeting. +This is the default. Specifying @samp{SSLv2}, @samp{SSLv3}, or @samp{TLSv1} forces the use of the corresponding protocol. This is useful when talking to old and diff --git a/src/ChangeLog b/src/ChangeLog index 1c4e2d5..db4cd04 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,8 @@ +2014-10-16 Tim Ruehsen + + * gnutls.c (ssl_connect_wget): do not use SSLv3 except explicitely requested + * openssl.c (ssl_init): do not use SSLv3 except explicitely requested + 2014-05-03 Tim Ruehsen * retr.c (retrieve_url): fixed memory leak diff --git a/src/gnutls.c b/src/gnutls.c index c09b7a2..75627e1 100644 --- a/src/gnutls.c +++ b/src/gnutls.c @@ -436,6 +436,7 @@ ssl_connect_wget (int fd, const char *hostname) switch (opt.secure_protocol) { case secure_protocol_auto: + err = gnutls_priority_set_direct (session, "NORMAL:%COMPAT:-VERS-SSL3.0", NULL); break; case secure_protocol_sslv2: case secure_protocol_sslv3: @@ -445,10 +446,10 @@ ssl_connect_wget (int fd, const char *hostname) err = gnutls_priority_set_direct (session, "NORMAL:-VERS-SSL3.0", NULL); break; case secure_protocol_pfs: - err = gnutls_priority_set_direct (session, "PFS", NULL); + err = gnutls_priority_set_direct (session, "PFS:-VERS-SSL3.0", NULL); if (err != GNUTLS_E_SUCCESS) /* fallback if PFS is not available */ -err = gnutls_priority_s
Re: [Bug-wget] please remove SSLv3 from being used until explicitly specified
Ángel González wrote: First of all, note that wget doesn't react to a disconnect with a downgraded retry thus it is mainly not vulnerable to poodle (you could only use CVE-2014-3566 against servers not supporting TLS). And curl is equally not affected (tested 7.38.0).
Re: [Bug-wget] please remove SSLv3 from being used until explicitly specified
Ángel González wrote: First of all, note that wget doesn't react to a disconnect with a downgraded retry thus it is mainly not vulnerable to poodle (you could only use CVE-2014-3566 against servers not supporting TLS). Note I tested both openssl and gnutls builds. Then I rebuilt 1.15¹ with both libraries using versions prior to poodle announcement. None of them was affected. ¹ I am having some problem with src/Makefile generation, so I didn't test with master, but that should be equivalent.
Re: [Bug-wget] please remove SSLv3 from being used until explicitly specified
On 16/10/14 19:01, Tim Rühsen wrote: Am Donnerstag, 16. Oktober 2014, 14:03:43 schrieb Christoph Anton Mitterer: Also, it wget seems to have this --secure-protocol=PFS, which seems a bit strange to me, since PFS is not a property of TLS/SSL itself but rather the algorithms used. Especially, when specifying --secure-protocol=PFS one shouldn't end up with SSLv2/3 accidentally :) Thanks for your input. We are just discussing that issue (and of course anybody is invited to take part here on the list). While we (developers) could change the code in a few minutes, there might be side effects that we (or others) don't want. At least we need an agreement with the maintainers on how the optimal strategy looks like. If you are *really* in a hurry, patch the source yourself. But I guess the distribution maintainers will provide patches in the next few days. How we change the default behaviour of Wget and maybe what additional features we want to give to the users still needs a bit of polishing. Regards, Tim First of all, note that wget doesn't react to a disconnect with a downgraded retry thus it is mainly not vulnerable to poodle (you could only use CVE-2014-3566 against servers not supporting TLS). Then, even in that case, as an attacker won't be able to dynamically connect in the background to another site, explotaition would be much harder (something like a recursive download on an attacker-controlled server (such as http) which is redirecting _some_ requests to the https target). For little gaining, as it's very unlikely that such wget would hold any secret for that server connection (I think you would need to use --load-cookies with a file shared with another -sensitive- batch processing). That said, I agree with the proposal of not connecting by default to SSL v3 servers and requiring it to be forced with --secure-protocol or --no-check-certificate.
Re: [Bug-wget] SSL Poodle attack
Am Mittwoch, 15. Oktober 2014, 17:26:49 schrieb Daniel Kahn Gillmor: > On 10/15/2014 03:10 PM, Tim Rühsen wrote: > > I tried to make clear that Wget *explicitely* asks for SSLv2 and SSLv3 in > > the default configuration when compiled with OpenSSL. Whatever the > > OpenSSL library vendor is doing... it won't affect Wget in this case. So > > with your attitude, you won't ever be safe ever from Poodle (I guess). > > > > And again my question: should we change the default behaviour of future > > versions of Wget ? > > With other words: since we know, the library vendor wouldn't help in the > > above case, what can we do to secure Wget ? > > hm, i think Tim is on to something here: by default, wget should use the > default ciphersuites and protocol versions selected by the TLS library. > Tweaking the default choices in wget itself tends to make wget more > brittle than the underlying library. > > The only way that should work to try to improve security in wget via TLS > implementation preference strings is if the preference string is > explicitly a minor modification of some system default. This may or may > not be possible depending on the preference string syntax of the > selected TLS implementation. > > (e.g. [for OpenSSL] if the system default is always explicitly > referenced as DEFAULT and we decide that we never want wget to use RC4, > then DEFAULT:-RC4 is a sensible approach, because it allows OpenSSL to > update DEFAULT and wget gains those improvements automatically) Here is a suggestion for a GnuTLS patch. I have a look at OpenSSL ciphers and make a similar patch soon. I also suggested (~1-2 years ago) an option to directly set priority strings / ciphers for GnuTLS and OpenSSL. In situations like these, such an option would allow for a quick reaction done by distribution maintainers and users. What do you think ? Tim From 582a887e61cea2dd0f64d462d919f8688fb7862f Mon Sep 17 00:00:00 2001 From: Tim Ruehsen Date: Thu, 16 Oct 2014 20:44:56 +0200 Subject: [PATCH] GnuTLS: do not use SSLv3 except explicitely requested --- src/ChangeLog | 4 src/gnutls.c | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 1c4e2d5..00d3c10 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2014-10-16 Tim Ruehsen + + * gnutls.c (ssl_connect_wget): do not use SSLv3 except explicitely requested + 2014-05-03 Tim Ruehsen * retr.c (retrieve_url): fixed memory leak diff --git a/src/gnutls.c b/src/gnutls.c index c09b7a2..75627e1 100644 --- a/src/gnutls.c +++ b/src/gnutls.c @@ -436,6 +436,7 @@ ssl_connect_wget (int fd, const char *hostname) switch (opt.secure_protocol) { case secure_protocol_auto: + err = gnutls_priority_set_direct (session, "NORMAL:%COMPAT:-VERS-SSL3.0", NULL); break; case secure_protocol_sslv2: case secure_protocol_sslv3: @@ -445,10 +446,10 @@ ssl_connect_wget (int fd, const char *hostname) err = gnutls_priority_set_direct (session, "NORMAL:-VERS-SSL3.0", NULL); break; case secure_protocol_pfs: - err = gnutls_priority_set_direct (session, "PFS", NULL); + err = gnutls_priority_set_direct (session, "PFS:-VERS-SSL3.0", NULL); if (err != GNUTLS_E_SUCCESS) /* fallback if PFS is not available */ -err = gnutls_priority_set_direct (session, "NORMAL:-RSA", NULL); +err = gnutls_priority_set_direct (session, "NORMAL:-RSA:-VERS-SSL3.0", NULL); break; default: abort (); -- 2.1.1 signature.asc Description: This is a digitally signed message part.
Re: [Bug-wget] please remove SSLv3 from being used until explicitly specified
Am Donnerstag, 16. Oktober 2014, 14:03:43 schrieb Christoph Anton Mitterer: > Hi. > > Could you please consider to remove SSLv3 (and if not done yet SSLv2 as > well) from being automatically used, while still leaving users the > choice to manually enable it (e.g. via --secure-protocol=SSLv2/3). > > I think it would be a bad idea to expect that these insecure versions > are dropped from the SSL backend libs, since they may be retained for > debugging purposes or people may just use outdated cipher preference > list. > > > Also, it wget seems to have this --secure-protocol=PFS, which seems a > bit strange to me, since PFS is not a property of TLS/SSL itself but > rather the algorithms used. > Especially, when specifying --secure-protocol=PFS one shouldn't end up > with SSLv2/3 accidentally :) Thanks for your input. We are just discussing that issue (and of course anybody is invited to take part here on the list). While we (developers) could change the code in a few minutes, there might be side effects that we (or others) don't want. At least we need an agreement with the maintainers on how the optimal strategy looks like. If you are *really* in a hurry, patch the source yourself. But I guess the distribution maintainers will provide patches in the next few days. How we change the default behaviour of Wget and maybe what additional features we want to give to the users still needs a bit of polishing. Regards, Tim signature.asc Description: This is a digitally signed message part.
[Bug-wget] please remove SSLv3 from being used until explicitly specified
Hi. Could you please consider to remove SSLv3 (and if not done yet SSLv2 as well) from being automatically used, while still leaving users the choice to manually enable it (e.g. via --secure-protocol=SSLv2/3). I think it would be a bad idea to expect that these insecure versions are dropped from the SSL backend libs, since they may be retained for debugging purposes or people may just use outdated cipher preference list. Also, it wget seems to have this --secure-protocol=PFS, which seems a bit strange to me, since PFS is not a property of TLS/SSL itself but rather the algorithms used. Especially, when specifying --secure-protocol=PFS one shouldn't end up with SSLv2/3 accidentally :) Cheers, Chris. smime.p7s Description: S/MIME cryptographic signature
Re: [Bug-wget] Issue with --content-on-error and --convert-links
On 13 October 2014 10:25, Joe Hoyle wrote: > Hi All, > > > I’m having issues using "--convert-links” in conjunction with > "--content-on-error”. Though "--content-on-error” is forcing wget to download > the pages, the links to that “errored” page is not update in other pages that > link to it. > > > This seems to be hinted at in the man page: > > > "Because of this, local browsing works reliably: if a linked file was > downloaded, the link will refer to its local name; if it was not downloaded, > the link will refer to its full Internet address rather than presenting a > broken link. The fact that the former links are converted to relative links > ensures that you can move the downloaded hierarchy to another directory.” > > > However, it would seem in the case of using —content-on-error it should > ignore this rule and do all the link substation anyhow. > > > If anyone knows if this *should* work then I’d be eager to hear it, or any > other way I can get any 404 pages downloaded and also linked to in the wget > mirror. > Currently, wget thought pages with 404 status code were not RETROKF (retrieval was OK) though the 404 page itself was actually downloaded successfully with `--content-on-error` option enabled. This behaviour is mostly acceptable I guess. But you can try the attached the patch for the moment. The other option would be serving the 404 page by manually setting it up with your web server. Regards. yousong 0001-Let-convert-links-work-with-content-on-error.patch Description: Binary data