Good evening Denys, I agree with you that this patch is unacceptable, I also agree that everyone who is complaining about the situation should send patches, but I strongly disagree that it is valid to break security to keep "common use cases" working. Using security-techniques like https should never give a false impression of being secure while implementing it wrong.
I remember that I've missed HTTPS-Support in wget for years and was very happy to see it being implemented back in 2014/2015. I've never had a look at the code nor its documentation, because I had trust that busybox is doing things right. I regret this was a personal mistake by myself. Most encryption is worth nothing without authentication. If authentication is skipped, encryption is broken by design and should not be implemented at all. It's valid to add a parameter to switch off authentication, but the default should verify that everything is correct. If you think it's ok to skip authentication, we could also skip checking bounds of a string because for common use cases there is everything okay and no checks need to be applied. To be constructive I've taken Jakub's Patch and rewrote it a bit to lead to a small but fast improvement: Added verification-options to openssl s_client helper and fail hard if the helper could not be spawned. This may be switched off using "--no-check-certificate". Internal wrapper and FTPS keep the old broken state and continue to work without authentication while s_client-helper will fail whenever verification fails. I remember that GNU wget also switched to this behavior some years ago, so it should be suitable for any common case. The broken state regarding the internal wrapper should be documented, but I'm too tired to do this. The attached patch fixes this issue for me and would restore some of my trust if being applied. Kind regards, Bernd Am 26.05.2018 um 19:34 schrieb Denys Vlasenko: > wget should work for common use cases. > Such as downloading sources of kernels, gcc and such. > From build scripts, not only by hand. > Without having to modify said scripts. > Your patch breaks that. > NAK. > > I don't care that security people are upset. > They are paranoid, it's part of their profession. > It does not mean everybody else have to be as paranoid. > > If you have a patch which adds actual cert checking > and thus does not introduce regressions, please post it. > > > On Sat, May 26, 2018 at 6:38 PM, <ja...@jirutka.cz> wrote: >>> //config: If you still think this is unacceptable, send patches. >> >> >> That’s exactly what I did. >> http://lists.busybox.net/pipermail/busybox/2018-May/086444.html >> >> Jakub >> >> >> On 2018-05-26 17:54, Denys Vlasenko wrote: >>> >>> On Sat, May 26, 2018 at 5:39 PM, <ja...@jirutka.cz> wrote: >>>>>> >>>>>> That's a crime against security! >>>>> >>>>> >>>>> Say what? >>>> >>>> >>>> That’s a hyperbole. The thing is that when you don’t verify the peer’s >>>> certificate, then you’re vulnerable to MitM attack with fake certificate >>>> injection. The whole SSL/TLS is totally useless in that moment. It’s more >>>> or >>>> less like putting the door’s key under the carpet right in front of the >>>> door. >>>> >>>> Allowing to bypass/ignore certificate verification is ok-ish in some >>>> situations, but only when the user do it consciously, using explicit >>>> option >>>> such as --no-check-certificate, not silently as the default option. >>> >>> >>> wget.c: >>> >>> //config: If you still think this is unacceptable, send patches. >>> //config: >>> //config: If you still think this is unacceptable, do not want to >>> send >>> //config: patches, but do want to waste bandwidth explaining how >>> wrong >>> //config: it is, you will be ignored. > _______________________________________________ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox > -- \\\||/// \\ - - // ( @ @ ) -oOo--( )--oOo------------------------------------------------------- tiggersWelt.net www.tiggersWelt.net Inhaber Bernd Holzmüller i...@tiggerswelt.net Büro: 07 11 / 550 425-90 Marktstraße 57 Fax: 07 11 / 550 425-99 70372 Stuttgart Impressum: https://tiggerswelt.net/impressum
networking/wget.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/networking/wget.c b/networking/wget.c
index 30c339244..a5306d3a5 100644
--- a/networking/wget.c
+++ b/networking/wget.c
@@ -136,18 +136,21 @@
//usage:#define wget_full_usage "\n\n"
//usage: "Retrieve files via HTTP or FTP\n"
//usage: IF_FEATURE_WGET_LONG_OPTIONS(
-//usage: "\n --spider Only check URL existence: $? is 0 if exists"
+//usage: "\n --spider Only check URL existence: $? is 0 if exists"
+//usage: IF_FEATURE_WGET_OPENSSL(
+//usage: "\n --no-check-certificate Don't validate the server's certificate"
+//usage: )
//usage: )
-//usage: "\n -c Continue retrieval of aborted transfer"
-//usage: "\n -q Quiet"
-//usage: "\n -P DIR Save to DIR (default .)"
-//usage: "\n -S Show server response"
+//usage: "\n -c Continue retrieval of aborted transfer"
+//usage: "\n -q Quiet"
+//usage: "\n -P DIR Save to DIR (default .)"
+//usage: "\n -S Show server response"
//usage: IF_FEATURE_WGET_TIMEOUT(
-//usage: "\n -T SEC Network read timeout is SEC seconds"
+//usage: "\n -T SEC Network read timeout is SEC seconds"
//usage: )
-//usage: "\n -O FILE Save to FILE ('-' for stdout)"
-//usage: "\n -U STR Use STR for User-Agent header"
-//usage: "\n -Y on/off Use proxy"
+//usage: "\n -O FILE Save to FILE ('-' for stdout)"
+//usage: "\n -U STR Use STR for User-Agent header"
+//usage: "\n -Y on/off Use proxy"
#include "libbb.h"
@@ -271,6 +274,7 @@ enum {
WGET_OPT_HEADER = (1 << 10) * ENABLE_FEATURE_WGET_LONG_OPTIONS,
WGET_OPT_POST_DATA = (1 << 11) * ENABLE_FEATURE_WGET_LONG_OPTIONS,
WGET_OPT_SPIDER = (1 << 12) * ENABLE_FEATURE_WGET_LONG_OPTIONS,
+ WGET_OPT_NO_CHECK_CERT = (1 << 13) * ENABLE_FEATURE_WGET_LONG_OPTIONS,
};
enum {
@@ -654,7 +658,12 @@ static int spawn_https_helper_openssl(const char *host, unsigned port)
pid = xvfork();
if (pid == 0) {
/* Child */
+#if ENABLE_FEATURE_WGET_LONG_OPTIONS
+ char *argv[12];
+ int argc;
+#else
char *argv[8];
+#endif
close(sp[0]);
xmove_fd(sp[1], 0);
@@ -680,7 +689,26 @@ static int spawn_https_helper_openssl(const char *host, unsigned port)
if (!is_ip_address(servername)) {
argv[5] = (char*)"-servername";
argv[6] = (char*)servername;
+#if ENABLE_FEATURE_WGET_LONG_OPTIONS
+ argc = 7;
+ } else
+ argc = 5;
+
+ if (!(option_mask32 & WGET_OPT_NO_CHECK_CERT)) {
+ argv[argc++] = (char*)"-verify";
+ argv[argc++] = (char*)"16";
+ argv[argc++] = (char*)"-verify_return_error";
+
+ if (is_ip_address(servername))
+ argv[argc++] = (char*)"-verify_ip";
+ else
+ argv[argc++] = (char*)"-verify_hostname";
+
+ argv[argc++] = (char*)servername;
}
+#else
+ }
+#endif
BB_EXECVP(argv[0], argv);
xmove_fd(3, 2);
@@ -1108,6 +1136,10 @@ static void download_one_url(const char *url)
int fd = spawn_https_helper_openssl(server.host, server.port);
# if ENABLE_FEATURE_WGET_HTTPS
if (fd < 0) { /* no openssl? try internal */
+# if ENABLE_FEATURE_WGET_LONG_OPTIONS
+ if (!(option_mask32 & WGET_OPT_NO_CHECK_CERT))
+ bb_error_msg_and_die("unable to validate the server's certificate");
+# endif
sfp = open_socket(lsa);
spawn_ssl_client(server.host, fileno(sfp), /*flags*/ 0);
goto socket_opened;
@@ -1402,10 +1434,10 @@ IF_DESKTOP( "tries\0" Required_argument "t")
"header\0" Required_argument "\xff"
"post-data\0" Required_argument "\xfe"
"spider\0" No_argument "\xfd"
+IF_FEATURE_WGET_OPENSSL(
+ "no-check-certificate\0" No_argument "\xfc")
/* Ignored (we always use PASV): */
IF_DESKTOP( "passive-ftp\0" No_argument "\xf0")
- /* Ignored (we don't do ssl) */
-IF_DESKTOP( "no-check-certificate\0" No_argument "\xf0")
/* Ignored (we don't support caching) */
IF_DESKTOP( "no-cache\0" No_argument "\xf0")
IF_DESKTOP( "no-verbose\0" No_argument "\xf0")
--
2.16.3
signature.asc
Description: OpenPGP digital signature
_______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
