Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable https verification for wget or disable https #80

Open
discordianfish opened this issue Mar 2, 2020 · 11 comments
Open

Enable https verification for wget or disable https #80

discordianfish opened this issue Mar 2, 2020 · 11 comments
Labels

Comments

@discordianfish
Copy link

busybox wget as included in the busybox base image doesn't verify TLS certificate:

$ docker run -ti --rm busybox wget -q https://expired.badssl.com && echo $?
wget: note: TLS certificate validation not implemented
0

While it prints an warning, most people won't realize this and are at risk downloading (and often executing).

See also #64: I also tend to agree that for full TLS, people should rather use another base image. But in this case I'd suggest disable https support altogether. Better no https than https without certificate validation and people not being aware of that.

@tianon
Copy link
Member

tianon commented Mar 2, 2020

We don't do anything specific to explicitly enable (or disable) https support in BusyBox's wget applet, so the fact that it's enabled is because it's enabled by default upstream. Given that and the notice being pretty clear about validation not being implemented (despite not exiting with a non-zero exit code), I'd argue we're faithfully representing BusyBox upstream's preferences here, which is our goal:

# CONFIG_LAST_SUPPORTED_WCHAR: see https://github.com/docker-library/busybox/issues/13 (UTF-8 input)
RUN set -eux; \
\
setConfs=' \
CONFIG_AR=y \
CONFIG_FEATURE_AR_CREATE=y \
CONFIG_FEATURE_AR_LONG_FILENAMES=y \
CONFIG_LAST_SUPPORTED_WCHAR=0 \
CONFIG_STATIC=y \
'; \
\
unsetConfs=' \
CONFIG_FEATURE_SYNC_FANCY \
'; \
\
make defconfig; \
\
for conf in $unsetConfs; do \
sed -i \
-e "s!^$conf=.*\$!# $conf is not set!" \
.config; \
done; \
\
for confV in $setConfs; do \
conf="${confV%=*}"; \
sed -i \
-e "s!^$conf=.*\$!$confV!" \
-e "s!^# $conf is not set\$!$confV!" \
.config; \
if ! grep -q "^$confV\$" .config; then \
echo "$confV" >> .config; \
fi; \
done; \
\
make oldconfig; \
\
# trust, but verify
for conf in $unsetConfs; do \
! grep -q "^$conf=" .config; \
done; \
for confV in $setConfs; do \
grep -q "^$confV\$" .config; \
done;

IMO there's a pretty decent argument for having BusyBox upstream implement --no-check-certificate and enforce using it for all https URLs when certificate validation is not implemented, but that's a discussion that should happen with upstream (https://bugs.busybox.net/).

@discordianfish
Copy link
Author

Well yeah it's the usual questions about how is responsible for the security of their users..
But you can disable it apparently: https://git.busybox.net/buildroot/tree/package/busybox/busybox.config?id=d5507262f37506d6b1b48eb409ed8bc3f08d3e47#n933

@discordianfish
Copy link
Author

Not very scientific but, as expected, most people are surprised by this: https://twitter.com/discordianfish/status/1234537962093740033
I'd suggest to disable it. It will break users, but better to break than putting them at risk, right?

@tianon
Copy link
Member

tianon commented Jun 26, 2020

IMO there's a pretty decent argument for having BusyBox upstream implement --no-check-certificate and enforce using it for all https URLs when certificate validation is not implemented, but that's a discussion that should happen with upstream (https://bugs.busybox.net/).

It turns out the exact change I've proposed was submitted at http://lists.busybox.net/pipermail/busybox/2018-May/086444.html, followed by quite a long discussion that amounts to "we can't change it, because it would break existing user scripts" (http://lists.busybox.net/pipermail/busybox/2018-May/086457.html) but it did result in both the warning we currently get (http://lists.busybox.net/pipermail/busybox/2018-May/086467.html), and as of today's 1.32.0 release, https://git.busybox.net/busybox/commit/?id=45fa3f18adf57ef9d743038743d9c90573aeeb91. Unfortunately, that patch only accounts for the case where there's a separate openssl binary available for busybox to shell out to.

@SuperQ

This comment has been minimized.

@tianon

This comment has been minimized.

@luckied
Copy link

luckied commented Oct 9, 2020

@tianon
Copy link
Member

tianon commented Oct 26, 2020

No, that change unfortunately only makes sure the warning is only printed once per invocation -- it doesn't turn the warning into an error (that's what https://git.busybox.net/busybox/commit/?id=45fa3f18adf57ef9d743038743d9c90573aeeb91 does, but only in the case of the OpenSSL-using implementation).

@zffocussss

This comment was marked as off-topic.

@thesuperzapper
Copy link

I am very confused by this issue, it seems like something is actually wrong with wget in the busybox container (but only when run inside of a Kubernetes cluster).

For example, when running wget "https://get.helm.sh/helm-v3.10.3-linux-amd64.tar.gz" in busybox:1.36.1 on Kubernetes 1.26, you will get the following error (and the file will NOT download):

wget: note: TLS certificate validation not implemented
wget: TLS error from peer (alert code 80): internal error
wget: error getting response: Connection reset by peer

However, this wget "https://get.helm.sh/helm-v3.10.3-linux-amd64.tar.gz" will succeed in an alpine image like alpine:3.18.2.

For now, I guess I recommend people use alpine until this is fixed.

@yosifkit
Copy link
Member

yosifkit commented Aug 5, 2023

However, this wget "https://get.helm.sh/helm-v3.10.3-linux-amd64.tar.gz" will succeed in an alpine image like alpine:3.18.2.

The busybox image is as close to upstream BusyBox releases as we can; the busybox binary in Alpine Linux is their own Alpine fork (i.e. patched and configured in a non-default way) as distributions tend to do. In other works, just because something works or is possible with Alpine's package does not mean it is part of the busybox image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants