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

incompatible_disallow_unverified_http_downloads: ignore plain http URLs (as opposed to https URLs) when downloading a file without given hash #8607

Closed
2 tasks
JLLeitschuh opened this issue Jun 11, 2019 · 12 comments
Assignees
Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.

Comments

@JLLeitschuh
Copy link

JLLeitschuh commented Jun 11, 2019

mitm_build
Want to take over the Java ecosystem? All you need is a MITM!

The details in the blog post above describe my research over the past 4 months into the use of HTTP to resolve dependencies across the JVM ecosystem.

This vulnerability impacted the open source build infrastructure for organizations like the Apache Foundation, Eclipse Foundation, Jenkins, JetBrains, RedHat, ect...

One of the cases where I discovered this vulnerability was in the build infrastructure for some of Stripe's open source projects. They were declaring jar dependencies without an SHA checksum. Thus a MITM of their Bazel build could have allowed for the malicious compromise of their artifacts. For this finding, I was awarded a bug bounty by their security team.

I believe that it's the responsibility of API authors, especially of build tools to help protect their users from careless security vulnerabilities like this.

As such, I've begun work on fixing this for all users of Gradle in this PR:
gradle/gradle#9419

Additionally, I've opened an issue with the Apache Maven team about fixing this there as well:
https://issues.apache.org/jira/browse/MNG-6673

Internal Report

The internal (security private) report of this industry-wide vulnerability can be found here:
https://issuetracker.google.com/issues/132071216

Deliverables

  • Next minor release: Begin warning users about their use of HTTP to download dependencies.
  • Next major release: Force users to explicitly opt-out of the security offered by using HTTPS.
@jin jin added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged labels Jun 11, 2019
@laurentlb
Copy link
Contributor

Klaus, what do you think?
Maybe we could forbid HTTP dependencies without a sha256? Any good alternative?

@aehlig
Copy link
Contributor

aehlig commented Jun 13, 2019

Maybe we could forbid HTTP dependencies without a sha256?

That should work. What we most likely cannot drop is the "plain http but known-good checksum" use case. And of course, such a change can only become active in a release that allows incompatible changes.

@aehlig aehlig removed the untriaged label Jun 14, 2019
@aehlig
Copy link
Contributor

aehlig commented Jun 14, 2019

Proposed implementation https://bazel-review.googlesource.com/c/bazel/+/103031 (plus a flip of that flag, once we're in the next window that allows incompatible changes).

@laurentlb
Copy link
Contributor

Thanks!
Please send an email to bazel-dev@ for feedback about this change.

@aehlig
Copy link
Contributor

aehlig commented Jun 17, 2019

Please send an email to bazel-dev@ for feedback about this change.

https://groups.google.com/forum/#!topic/bazel-dev/sa0mkKXouoQ

@aehlig aehlig added the incompatible-change Incompatible/breaking change label Jun 18, 2019
@aehlig
Copy link
Contributor

aehlig commented Jun 18, 2019

Once https://bazel-review.googlesource.com/c/bazel/+/103031 is submitted, a flag --incompatible_disallow_unverified_http_downloads will be available that, if set, disallows downloads from http URLs if no checksum is provided.

Migration: add a checksum or at least one https source for fetching the file. Note that if no checksum is given, the http URLs will be ignored but it is no error if they are present, provided at least one non-http URL remains for the actual fetching. In this way, it is still possible to have the following mixed workflow. The developer updating the dependencies fetches from the authoritative https URL to compute the good hash and commits it. Everyone else would use the quick http-mirrors and verify against the committed hash.

@aehlig
Copy link
Contributor

aehlig commented Jun 18, 2019

@laurentlb Do we need a separate design document for that incompatible change, or is the information linked from this issue enough?

@dslomov
Copy link
Contributor

dslomov commented Jun 24, 2019

Please set a priority for this feature request.

@laurentlb laurentlb added P1 I'll work on this now. (Assignee required) and removed untriaged labels Jun 24, 2019
@aehlig
Copy link
Contributor

aehlig commented Jul 12, 2019

Once https://bazel-review.googlesource.com/c/bazel/+/103031 is submitted, a flag --incompatible_disallow_unverified_http_downloads will be available that, if set, disallows downloads from http URLs if no checksum is provided.

Migration: add a checksum or at least one https source for fetching the file. Note that if no checksum is given, the http URLs will be ignored but it is no error if they are present, provided at least one non-http URL remains for the actual fetching. In this way, it is still possible to have the following mixed workflow. The developer updating the dependencies fetches from the authoritative https URL to compute the good hash and commits it. Everyone else would use the quick http-mirrors and verify against the committed hash.

This incompatible change has been approved. I will rebase the change and send it for review soon.

@aehlig aehlig changed the title Let's stop resolving dependencies over HTTP and require HTTPS incompatible_disallow_unverified_http_downloads: ignore plain http URLs (as opposed to https URLs) when downloading a file without given hash Jul 12, 2019
bazel-io pushed a commit that referenced this issue Jul 12, 2019
Add a new incompatible flag. Once flipped, we disallow downloads of files
via plain HTTP unless a checksum is provided. Downloads via HTTPS as well
as downloads with a specified hash will still be allowed.

Background #8607

Change-Id: I95f6fa9e230401117de050173f3105ccc7fa7bb4
PiperOrigin-RevId: 257815568
bazel-io pushed a commit that referenced this issue Jul 17, 2019
…om localhost

The repository cache tests (among other things) that a download of a file adds it
to the cache under its sha256 hash, even if no hash was specified on the download.
To make the test certificate-independent, this is done via plain http on the
loopback device. Explicitly allow this use of unverified http download.

Related to #8607

Change-Id: I0140356b1d952c3ccdea78d5f35a2fa9d5926c84
PiperOrigin-RevId: 258557255
bazel-io pushed a commit that referenced this issue Jul 17, 2019
Since we run our tests for downloads via plain http, provide a checksum,
even in the case where we expect a timeout. In this way, prepare
our own code base for the #8607 flag flip.

Change-Id: I7ca9afb49008325975dff38b38aa1c596f0e9f83
PiperOrigin-RevId: 258565184
bazel-io pushed a commit that referenced this issue Jul 17, 2019
In preparation of the flag flip in #8607, use a checksum for
our plain http-downloads.

Change-Id: I834abaccdb4b727f48c3dc2df8cd119839a85992
PiperOrigin-RevId: 258569601
bazel-io pushed a commit that referenced this issue Jul 22, 2019
…fficient

The function download{,_and_extract} of a repository context allow to provide
checksums that the downloaded file has to have in order for the downlaod considered
to be successful. Traditionally, this was done by providing a parameter 'sha256';
however, it is also possible through the recently provided 'integrity' parameter,
that can take checksums in different form (currently supporting sha256 and sha512).
Consider such a checksum also sufficient for the requirement of not downloading
from plain http URLs without a given checksum introduced in #8607.

Change-Id: I164e1afcdb65124a361321603c7277dc635c05b9
PiperOrigin-RevId: 259323443
bazel-io pushed a commit that referenced this issue Jul 23, 2019
This test was trying to verify that if two download locations are provided,
both are recorded in the workspace_rules_log. It also tested that the
build completed successfully; however, the latter only happened by accident,
as the testing server would blindly return 200 and only then try to read
the file---and then crash, as file1.txt does not exist; so the file returned
had to relation at all to the file served at the second (valid, but never
accessed) URL. This was uncovered when adding a checksum in preparation of
the upcoming #8607 flag flip.

As the test is only about logging, put the use of download to the intended
form that all provided URLs are valid and return a file with the correct
hash.

Change-Id: I208ebffc5a724df7a75b2bced6fafbd8f50417eb
PiperOrigin-RevId: 259486544
bazel-io pushed a commit that referenced this issue Jul 23, 2019
...so that our tests are compatible with the #8607 flag flip.

Change-Id: Ie2164b97c6be60cb58520352f8f7bcd7eb694fa8
PiperOrigin-RevId: 259507136
bazel-io pushed a commit that referenced this issue Jul 24, 2019
…cksum

...over plain http to work. With the #8607 flag flip we
will be explicitly disallowing them.

Change-Id: I67e33cef57389656200b49e2d87fc372b813e3f6
PiperOrigin-RevId: 259687765
@JLLeitschuh
Copy link
Author

JLLeitschuh commented Jul 25, 2019

Thanks Bazel team for prioritizing this and working to protect your users and the JVM ecosystem.

bazel-io pushed a commit that referenced this issue Aug 5, 2019
…ownload from 127.0.0.1

The loopback device is usually safe against man-in-the-middle attacks,
so allow these downloads in said test in preparation of #8607.

Change-Id: I9bc7c61719aea5a19350ea5f7af7ecce99387b5a
PiperOrigin-RevId: 261656498
bazel-io pushed a commit that referenced this issue Aug 5, 2019
…true

As discussed in #8607, downloading files over plain http without reasonable
verification afterwards (e.g., checking the sha256 sum) is a security risk
and therefore should be discouraged. Flip the the corresponding flag disallowing
such downloads to true. The flag was available with default false already in 0.29,
and migration was possible even before that, simply by adding known-good checksums.

Change-Id: Ia3d46115996bf7b7c4aed56dcd15fa7317b5d4fa
PiperOrigin-RevId: 261662705
@aehlig
Copy link
Contributor

aehlig commented Aug 5, 2019

Closing, as the flag has been flipped in 299655e.

@aehlig aehlig closed this as completed Aug 5, 2019
bazel-io pushed a commit that referenced this issue Aug 26, 2019
…erver

Force usage of either HTTPS or HTTP w/ SHA-1. Note that SHA-1 is still susceptible to collision attacks, but this should reduce the exploitable surface of the current implementation that allows plain HTTP without checksums.

Also see #6799 (comment)

Closes #9235.

RELNOTES: `maven_jar` and `maven_server` now disallow using plain HTTP URLs without a specified checksum. If you are still using `maven_jar`, consider migrating to [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_external) for transitive dependency management. See [#8607](#8607) for more information.

Change-Id: I61b96b1d797071dc84291fecbf05a45d927240a5
PiperOrigin-RevId: 265442213
@JLLeitschuh
Copy link
Author

Sanity check: Does this check that any redirects are HTTPS based as well, or only the declared host is HTTPS based?

In Gradle, we are checking both that the declared host is HTTPS based, and all redirects are HTTPS based as well.
gradle/gradle#10065

bazel-io pushed a commit that referenced this issue Oct 10, 2019
Baseline: 97a8264

Cherry picks:

   + a0e3bb2:
     Remove support for authentication and .netrc
   + ada2c55:
     Add explicit --sdk argument to xcrun calls
   + 847df72:
     toolchain_vanilla: Unset source and target language level
     versions
   + 5cfa030:
     Update java_tools version to javac11-v5.1.

Incompatible changes:

  - Python, Windows: the
    --[no]incompatible_windows_escape_python_args is no longer
    supported. (It was flipped to true in Bazel 0.27.0)
  - --incompatible_use_native_patch is enabled by default
  - Windows: --incompatible_windows_bashless_run_command is now true
    by default, meaning "bazel run //foo:bin" will run the binary as
    a subprocess of the Bazel client. (When the flag is false, the
    binary is executed as a subprocess of Bash.)
  - Windows: --incompatible_windows_native_test_wrapper is enabled by
    default

New features:

  - Genrule now supports `cmd_bash`, `cmd_ps`, `cmd_bat` attributes.
    More details at
    https://docs.bazel.build/versions/master/be/general.html#genrule.c
    md
  - config_setting can now check multiple values on "--foo=firstVal
    --foo=secondVal ..."-style flags
  - tags: use `--experimental_allow_tags_propagation` flag to
    propagate tags to the action's execution requirements from
    targets. Such tags should start with: `no-`, `requires-`,
    `supports-`, `block-`, `disable-`, `cpu:`. See #8830 for details.
  - Users can now get generated def file from cc_library via
    "def_file" output group on Windows.
  - Platform-specific bazelrc: with --enable_platform_specific_config
    you can
    enable flags in bazelrc according to your host platform.
  - tags: use `--experimental_allow_tags_propagation` flag to
    propagate tags to the action's execution requirements from
    cc_library or cc_binary targets. Such tags should start with:
    `no-`, `requires-`, `supports-`, `block-`, `disable-`, `cpu:`.
    See #8830 for details.
  - tags: use --experimental_allow_tags_propagation flag to propagate
    tags to the action's execution requirements from java targets.
    Such tags should start with: no-, requires-, supports-, block-,
    disable-, cpu:. See #8830 for details.

Important changes:

  - Bazel Android builds now use aapt2 by default. To revert to aapt,
    set `--android_aapt=aapt`.
  - Make either --subcommands or --verbose_failures imply
    --materialize_param_files
  - Bazel Android builds now use aapt2 by default. To revert to aapt,
    set `--an...
    RELNOTES: None
  - by default all remote connections considered to be via `gRPC`
    with TLS enabled, unless other specified. To disable TLS use
    `grpc://` prefix for you endpoints. All remote connections via
    `gRPC` affected - `--remote_cache`, `--remote_executor` or
    `--bes_backend`. http cache/executor is not affected. See #8061
    for details.
  - cc_* rules support non-transitive defines through a
    'local_defines' attribute.
  - Enable
    incompatible_disallow_rule_execution_platform_constraints_allowed
    by default (#8136).
  - incompatible_disallow_split_empty_separator is enabled by default
  - Fixed Android build issues with aapt2 on Windows. See the [GitHub
    issue](#9102) for more
    information.
  - --incompatible_disable_static_cc_toolchains has been flipped. See
    #8546.
  - --remote_default_platform_properties has been deprecated in favor
    of --remote_default_exec_properties.
  - The --incompatible_make_thinlto_command_lines_standalone flag has
    been flipped, see #6791
    for more information.
  - The --incompatible_use_specific_tool_files flag has been flipped.
    See #9126 for more
    information.
  - Clarify default visibility.
  - Enables incompatible_auto_configure_host_platform
  - New incompatible flag --incompatible_disable_depset_items
    disables the "items" parameter in the Starlark depset
    constructor. Use "direct" and "transitive" parameters instead.
  - --incompatible_assignment_identifiers_have_local_scope is enabled
  - incompatible_disable_partition_default_parameter is enabled by
    default ()
  - incompatible_restrict_attribute_names is enabled
    (#6437)
  - The --incompatible_disable_nocopts flag has been flipped. See
    #8546 for more
    information.
  - Deprecated Java-Starlark API java_common.create_provider is
    removed. JavaInfo() legacy args (actions, sources, source_jars,
    use_ijar, java_toolchain, host_javabase) are removed.
  - The flag incompatible_disallow_hashing_frozen_mutables is enabled
    (#7800)
  - `maven_jar` and `maven_server` now disallow using plain HTTP URLs
    without a specified checksum. If you are still using `maven_jar`,
    consider migrating to
    [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_ext
    ernal) for transitive dependency management. See
    [#8607](#8607) for more
    information.
  - Added `sha256` and `sha256_src` attributes to `maven_jar`. Please
    consider migrating to SHA-256 as SHA-1 has been deemed
    cryptographically insecure ([https://shattered.io]()). Or, use
    [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_ext
    ernal) to manage your transitive Maven dependencies with artifact
    pinning and SHA-256 verification support.
  - introducing per-target exec_properties
  - Bazel now supports ThinLTO builds on Linux for Clang versions >=
    6.0. ThinLTO can be enabled through --features=thin_lto
  - The Target.output_group field in Starlark is removed. Use
    OutputGroupInfo instead. See
    #7949 for details.
  - Make a number of parameters of Starlark builtin functions
    positional-only (as opposed to specifiable by keyword). See
    #8147 for details.
  - incompatible_skip_genfiles_symlink is enabled by default (#8651)
  - Change Pruned events will fire immediately after being checked.
  - --incompatible_remove_legacy_whole_archive has been flipped. See
    #7362 for more
    information

This release contains contributions from many people at Google, as well as Adam Liddell, Alessandro Patti, Arshabh Kumar Agarwal, Artem Pelenitsyn, Artem Zinnatullin, Benjamin Peterson, David Ostrovsky, Emmanuel Goh, Farhim Ferdous, George Gensure, iirina, Keith Smiley, Kiril Videlov, Laurent Le Brun, Mantas Sakalauskas, Marwan Tammam, Matt Mukerjee, panzhongxian, Shachar Anchelovich, Stepan Koltsov, Stephan Wolski, Travis Clarke, Yannic Bonenberger, Yuta Saito.
dslomov pushed a commit that referenced this issue Oct 11, 2019
Baseline: 97a8264

Cherry picks:

   + a0e3bb2:
     Remove support for authentication and .netrc
   + ada2c55:
     Add explicit --sdk argument to xcrun calls
   + 847df72:
     toolchain_vanilla: Unset source and target language level
     versions
   + 5cfa030:
     Update java_tools version to javac11-v5.1.

Incompatible changes:

  - Python, Windows: the
    --[no]incompatible_windows_escape_python_args is no longer
    supported. (It was flipped to true in Bazel 0.27.0)
  - --incompatible_use_native_patch is enabled by default
  - Windows: --incompatible_windows_bashless_run_command is now true
    by default, meaning "bazel run //foo:bin" will run the binary as
    a subprocess of the Bazel client. (When the flag is false, the
    binary is executed as a subprocess of Bash.)
  - Windows: --incompatible_windows_native_test_wrapper is enabled by
    default

New features:

  - Genrule now supports `cmd_bash`, `cmd_ps`, `cmd_bat` attributes.
    More details at
    https://docs.bazel.build/versions/master/be/general.html#genrule.c
    md
  - config_setting can now check multiple values on "--foo=firstVal
    --foo=secondVal ..."-style flags
  - tags: use `--experimental_allow_tags_propagation` flag to
    propagate tags to the action's execution requirements from
    targets. Such tags should start with: `no-`, `requires-`,
    `supports-`, `block-`, `disable-`, `cpu:`. See #8830 for details.
  - Users can now get generated def file from cc_library via
    "def_file" output group on Windows.
  - Platform-specific bazelrc: with --enable_platform_specific_config
    you can
    enable flags in bazelrc according to your host platform.
  - tags: use `--experimental_allow_tags_propagation` flag to
    propagate tags to the action's execution requirements from
    cc_library or cc_binary targets. Such tags should start with:
    `no-`, `requires-`, `supports-`, `block-`, `disable-`, `cpu:`.
    See #8830 for details.
  - tags: use --experimental_allow_tags_propagation flag to propagate
    tags to the action's execution requirements from java targets.
    Such tags should start with: no-, requires-, supports-, block-,
    disable-, cpu:. See #8830 for details.

Important changes:

  - Bazel Android builds now use aapt2 by default. To revert to aapt,
    set `--android_aapt=aapt`.
  - Make either --subcommands or --verbose_failures imply
    --materialize_param_files
  - Bazel Android builds now use aapt2 by default. To revert to aapt,
    set `--an...
    RELNOTES: None
  - by default all remote connections considered to be via `gRPC`
    with TLS enabled, unless other specified. To disable TLS use
    `grpc://` prefix for you endpoints. All remote connections via
    `gRPC` affected - `--remote_cache`, `--remote_executor` or
    `--bes_backend`. http cache/executor is not affected. See #8061
    for details.
  - cc_* rules support non-transitive defines through a
    'local_defines' attribute.
  - Enable
    incompatible_disallow_rule_execution_platform_constraints_allowed
    by default (#8136).
  - incompatible_disallow_split_empty_separator is enabled by default
  - Fixed Android build issues with aapt2 on Windows. See the [GitHub
    issue](#9102) for more
    information.
  - --incompatible_disable_static_cc_toolchains has been flipped. See
    #8546.
  - --remote_default_platform_properties has been deprecated in favor
    of --remote_default_exec_properties.
  - The --incompatible_make_thinlto_command_lines_standalone flag has
    been flipped, see #6791
    for more information.
  - The --incompatible_use_specific_tool_files flag has been flipped.
    See #9126 for more
    information.
  - Clarify default visibility.
  - Enables incompatible_auto_configure_host_platform
  - New incompatible flag --incompatible_disable_depset_items
    disables the "items" parameter in the Starlark depset
    constructor. Use "direct" and "transitive" parameters instead.
  - --incompatible_assignment_identifiers_have_local_scope is enabled
  - incompatible_disable_partition_default_parameter is enabled by
    default ()
  - incompatible_restrict_attribute_names is enabled
    (#6437)
  - The --incompatible_disable_nocopts flag has been flipped. See
    #8546 for more
    information.
  - Deprecated Java-Starlark API java_common.create_provider is
    removed. JavaInfo() legacy args (actions, sources, source_jars,
    use_ijar, java_toolchain, host_javabase) are removed.
  - The flag incompatible_disallow_hashing_frozen_mutables is enabled
    (#7800)
  - `maven_jar` and `maven_server` now disallow using plain HTTP URLs
    without a specified checksum. If you are still using `maven_jar`,
    consider migrating to
    [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_ext
    ernal) for transitive dependency management. See
    [#8607](#8607) for more
    information.
  - Added `sha256` and `sha256_src` attributes to `maven_jar`. Please
    consider migrating to SHA-256 as SHA-1 has been deemed
    cryptographically insecure ([https://shattered.io]()). Or, use
    [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_ext
    ernal) to manage your transitive Maven dependencies with artifact
    pinning and SHA-256 verification support.
  - introducing per-target exec_properties
  - Bazel now supports ThinLTO builds on Linux for Clang versions >=
    6.0. ThinLTO can be enabled through --features=thin_lto
  - The Target.output_group field in Starlark is removed. Use
    OutputGroupInfo instead. See
    #7949 for details.
  - Make a number of parameters of Starlark builtin functions
    positional-only (as opposed to specifiable by keyword). See
    #8147 for details.
  - incompatible_skip_genfiles_symlink is enabled by default (#8651)
  - Change Pruned events will fire immediately after being checked.
  - --incompatible_remove_legacy_whole_archive has been flipped. See
    #7362 for more
    information

This release contains contributions from many people at Google, as well as Adam Liddell, Alessandro Patti, Arshabh Kumar Agarwal, Artem Pelenitsyn, Artem Zinnatullin, Benjamin Peterson, David Ostrovsky, Emmanuel Goh, Farhim Ferdous, George Gensure, iirina, Keith Smiley, Kiril Videlov, Laurent Le Brun, Mantas Sakalauskas, Marwan Tammam, Matt Mukerjee, panzhongxian, Shachar Anchelovich, Stepan Koltsov, Stephan Wolski, Travis Clarke, Yannic Bonenberger, Yuta Saito.
bazel-io pushed a commit that referenced this issue Jan 30, 2020
#8607

RELNOTES: The flag --incompatible_disallow_unverified_http_downloads is removed.
PiperOrigin-RevId: 289675443

Change-Id: I8c75545ad9b0997c197e7280577a5725bfc05b84
PiperOrigin-RevId: 292346444
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@philwo philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

No branches or pull requests

7 participants