Ban HTTP by default in HttpClient

[mehmetf]

Goal

For Google's first party applications, using cleartext to communicate with a server is a launch blocker. To help Flutter applications detect such problems, we would like to explicitly ban cleartext http and only allow TLS traffic in HttpClient. Since there are legitimate use cases for cleartext transmission, it should also be possible to override this behavior via a security review.

Proposal

Many packages such as Flutter access dart:io directly for its network calls. So, if the app is loading network assets (such as an image), it could accidentally use cleartext without a problem. We can ensure that does not happen by banning HTTP in the lowest level possible in Dart. We could do this in the platform libraries by throwing an exception in dart:io#HttpClient if scheme is set to HTTP. There are several attributes of this requirement that shapes the proposed implementation.

This is for client apps only. From dart:io#HttpClient perspective, this just means iOS and Android (not server). We should ban cleartext if Platform.isIOS or Platform.isAndroid. Note that Web is out of scope and should be handled separately.

We need to allow overrides. I propose to create a new zone variable for "allowClearText" in HttpOverrides. This would be easy to use and readable. It also fits existing usage pattern if we expand the purpose of HttpOverrides beyond testing (it is already being used beyond testing by some clients).

Combining these proposals, we get:

• Modify HttpOverrides to support allowClearText as a zone variable.

• Create an _EmbedderConfig class to contain a static configuration as a default. This will be overridden in embedders for iOS and Android.

• Modify _HttpClient to ban HTTP scheme only if:

   (Zone.current[allowClearText] ?? _EmbedderConfig.allowClearText) == false

See this internal design doc go/disable-http-flutter-dd for a more detailed discussion of alternatives.

[mehmetf]

@natebosch @vsmenon PTAL

[zanderso]

Is it only cleartext http communication that is a launch blocker? If this also applies to other communication, then it may make sense to do roughly the same thing, but pushed down to the dart:io Socket API. It might be a bit simpler there too, since you can just make the non-Secure socket connect and bind methods throw rather than hooking into the middle of the HttpClient connection setup.

The override flag could go in IOOverrides: https://api.dart.dev/stable/2.7.1/dart-io/IOOverrides-class.html.

[mehmetf]

The internal documentation states HTTP only even though I am sure they would like to encrypt any traffic to/from an external service (FTP?). However, I am not sure if a similar ban exists for internal communication (IPCs?). It might be simpler to implement but I suspect it would be a bigger change to roll out.

PS: Would this break observatory? I know we currently use ws:// to connect but not sure if these are secure sockets or not.

[vsmenon]

@a-siva @lrhn @mit-mit - thoughts on this?

[lrhn]

Having different behavior in public vs. internal builds is an accident waiting to happen. It just takes one small bug in the transform and then the protection is gone from the internal Google version of Dart. Also, we cannot ensure that all Google Dart code runs on that version when we ship it to client devices. Same for behavior which differs between platforms. If it works elsewhere, but not on iOS or Android, then it onlyr equires one mistake to enable it on iOS and Android again.

I'm not sure how I feel about using zone variables to override a default. Users are usually not very aware of zones, and it's easy to have code run in a different zone than you expect. If any zone enables the unsafe usage, the entire application should be thoroughly reviewed to ensure that that zone itself is contained.

Is it only the HttpClient class that we need to worry about? If so, we could give it a constructor parameter which forces it to use an encrypted connection on every request, and fail if it can't get one. Then we can add a lint ensuring that code passes that parameter.

Breaking existing code is bad, but if we haven't already, we could perhaps make the HttpClient attempt to upgrade (https://en.wikipedia.org/wiki/HTTP/1.1_Upgrade_header) non-encrypted connections where possible (HTTPS everywhere FTW!) If the upgrade succeeds, then nobody should have any issues. If not, the constructor parameter comes into play.

So, maybe make it:

HttpClient({
  SecurityContext context, 
  HttpsUpgrade httpsUpgrade = HttpsUpgrade.whenPossible
}) ...

/// When an [HttpClient] upgrades an HTTP connection to HTTPS.
enum HttpsUpgrade {
  /// An HTTP connection is always upgraded to HTTPS.
  ///
  /// If the upgrade isn't possible, the connection fails.
  always,
  /// An HTTP connection is upgraded to HTTPS when the server allows it.
  ///
  /// An upgrade is always attempted, but it isn't possible, the connection is performed over HTTP.
  whenPossible,
  /// An HTTP request is always honored and never upgraded to HTTPS by the client.
  never
}

Then we can make a lint which requires the always option to be passed. You can ignore that using // ignore, as for any other lint, but it can be caught in code reviews if it isn't intended.

It is technically a breaking change if the current behavior matches never and we make the default whenPossible.

[mehmetf]

The upgrade option is interesting and should be implemented if it isn't already. I would like to keep that out of scope since that's not an allowable option internally.

I don't foresee code transform accidents being a problem since we can mitigate against it by putting in a few simple tests that block Dart rolls.

I had considered a constructor parameter but it ends up being a more complicated ecosystem change. HttpClient is being used directly in 30+ places internally (including in 5 third_party libraries). Each package will either need to expose the same option (and have its own lint) or needs to convert to being HTTPS-only. It is definitely doable but I feel like this solution is more manageable. Moreover, it fits to the intent of HttpOverrides as I understand it.

[mehmetf]

I am also aware that zones are tougher to use for most clients but we expect the overrides to be the exception not the norm. So from developer UX perspective, I am OK with them.

Overrides necessitate a thorough review of the code no matter what methodology is used. There are many different ways that people can shoot themselves in the foot (such as accidentally leaking an unsafe HttpClient instance).

[natebosch]

Would we consider changing the default externally in Dart 3 so that internal and external configurations match?

[mehmetf]

If everyone is OK with this proposal, I will add comments and tests to the PR and send out for review.

[lrhn]

Let's ask @sortie as well.

This reminds me of the "unsafe" annotation: https://github.com/dart-lang/sdk/issues/40595 You are saying that using a clear-text client is unsafe in some situations (in Flutter clients mainly). However, you want to enforce this at run-time, not just show warnings at compile-time.

What if it was just a hint, but one that was enabled automatically for Flutter clients (they have a default analysis options file AFAIK)?

I really dislike hard-coding platform names in the code. What makes iOS and Android so special? Is there something else we can do for those platforms (like set an environment variable by default while compiling or something)? It's not like Windows is less of a client OS, it's just not a mobile OS (and not currently a Flutter compilation target, but there is no reason it can't be in the future). Or, in short, we do not want to special case one particular use-case in the platform libraries.

Maybe we can make it a Dart embedder/compiler choice. But why just this one use-case then (plain-text HTTP). Are there other features that need to be handled as well?

So, I think that I'm fairly strongly against this change. It's to specific, too ad-hoc, to belong in the platform libraries as long as it's defined in terms of Platform.isIOS or Platform.isAndroid. That's not where that configuration should be located.

I'm fine with adding a general feature which allows you to disallow plain-text HTTP connections. I'm also fine with allowing some way to enable it by default when compiling (perhaps use an environment variable, so compile with -Dhttp.allowCleartext=false). Then you can have zone-based overrides as well.

Something like:

class HttpClient  ... {
  /// Runs [action] in a [Zone] where plain-text HTTP connections are allowed.
  ///
  /// Some programs or environments may disallow un-encrypted HTTP connections.
  /// In such programs, running the code creating the connection using this function will 
  /// allow unencrypted connections. This ensures that an unencrypted connection isn't
  /// created by accident, and it is visible in the code where it is being allowed.
  static Future<T> allowPlaintext<T>(FutureOr<T> action()) {
    return await runZoned(action, zoneVariables: {#_allowPlaintext, true});
  }
  bool get _allowsPlaintext => 
      const bool.fromEnvironment("http.allowPlaintext") || Zone.current[#_allowPlaintext] == true;

  ... where the test needs to be ...
  if (userRequestedPlaintext & !_allowsPlaintext) {
     throw StateError("Plain-text HTTP connections are disallowed.");
  }

[mehmetf]

Thanks for the comments.

This is necessarily platform specific whether we have the code in core libs or expose it as a compile-time flag. Android and iOS ban HTTP connections for apps at the OS level. Linux and Windows do not. That's what makes iOS and Android special. This might change in the future, at which point we can enable it for other platforms as well.

I am on board with not having platform-specific code in core libraries. I understand it is jarring. However, your suggestion exposed a problem in my plan. Today, we bring in pre-built core snapshots to google3. Neither my source transform idea nor your compile-time environment variable idea work in this situation since we don't build the VM or the core libs in google3.

Can this be a runtime flag to VM instead? We can have this passed in by the embedder of a particular platform and we can configure it in google3 specifically. Can core libraries read a VM flag? I would appreciate an example.

What if it was just a hint, but one that was enabled automatically for Flutter clients (they have a default analysis options file AFAIK)?

There will be a hint which will check whether you are explicitly allowing cleartext by setting a zone variable. We can indeed use unsafe_api hint to do it because that particular zone variable will be marked as unsafe and will require a security audit to use.

From your response, it also sounded like you were not sure why we need the runtime check if we have the hint above. Nothing prevents someone from attempting to pass in a http URL to a secure HttpClient since we cannot check the URL content at compile time. It is just an arbitrary string that can be constructed and passed around in many ways. We need this to fail at runtime.

[zanderso]

Can this be a runtime flag to VM instead? We can have this passed in by the embedder of a particular platform and we can configure it in google3 specifically. Can core libraries read a VM flag? I would appreciate an example.

See the class _EmbedderConfig:

https://github.com/dart-lang/sdk/blob/a75ffc89566a1353fb1a0f0c30eb805cc2e8d34c/sdk/lib/io/embedder_config.dart#L17

For example, calls to dart:io exit() are disallowed in the Fuchsia flutter_runner:

https://github.com/flutter/engine/blob/bd8c9555027e45eea222f22d9dbe46606eb1b4f8/shell/platform/fuchsia/flutter/isolate_configurator.cc#L74

[mehmetf]

Thanks @zanderso. So that's how embedder flags can leak into Dart.

@lrhn let me know if you are OK with this approach.

[mehmetf]

@Hixie Is there a chance you would want to make this the default for all Flutter iOS/Android apps? This is the default behavior of iOS and Android platforms. On the other hand, this would to be a pretty big breaking change if apps are relying on it.

[Hixie]

I guess I can sell this so long as opting into http support is a one-liner.

Re upgrades, I don't see how to do that in a secure way. I would not recommend that. But maybe I'm misunderstanding the idea.

[mehmetf]

Sounds good.

FTR: Runtime flags would not work without extensive changes. We don't currently have a mechanism in Flutter engine for configuring VM flags for release mode. Android relies on Intent extras whereas iOS takes arguments from process execution. These are only viable for development workflow. If we want these flags in release workflow, we would have to create a way to get VM flags via static configuration files (manifest on Android and Info.plist on iOS). I am not sure if we want to do that.

@cbracken could you please let me know if there's an existing policy in place for allowing custom VM flags for release mode? I wonder if this was previously discussed.

[Hixie]

Personally I'd prefer some sort of code-level control than runtime flags. Runtime flags are much more buried and mysterious and therefore much more likely to be misused, misunderstood, or accidentally set incorrectly.

On Tue, Feb 18, 2020 at 7:40 PM Mehmet Fidanboylu notifications@github.com wrote:

Sounds good.

FTR: Runtime flags would not work without extensive changes. We don't currently have a mechanism in Flutter engine for configuring VM flags for release mode. Android relies on Intent extras whereas iOS takes arguments from process execution. These are only viable for development workflow. If we want these flags in release workflow, we would have to create a way to get VM flags via static configuration files (manifest on Android and Info.plist on iOS). I am not sure if we want to do that.

@cbracken https://github.com/cbracken could you please let me know if there's an existing policy in place for allowing custom VM flags for release mode? I wonder if this was previously discussed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/40548?email_source=notifications&email_token=AAEGSHFONMAKBU3I66LWME3RDSS4HA5CNFSM4KR5IKZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMGHRAI#issuecomment-588019841, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEGSHG7QXHZIX2MMEAEYCLRDSS4HANCNFSM4KR5IKZA .

-- Ian Hickson

[lrhn]

Having a platform level flag, no matter how it's integrated into the platform, is fine with me.

If it can be applied at individual application compile-time, that's the most fine grained approach. Then Google internal builds can set it to false in the build system. Using a -Dio.http.allowCleartext=false declaration might work, depending on how we actually handle these compile-time constants in the build process.

If it is applied to the run-time system when the embedder is built (that only works for native builds, not compilation to JavaScript, which also exposes HTTP requests), then we will have to figure out where to apply the flag (if the VM runtime snapshot is not built by the Google build system, then we can't put it into the snapshot).

As long as the code can check this flag in some way rather than hard-coding where the restriction applies into the libraries themselves, then I am fine with it. Then we can keep it a non-breaking change for everyone who doesn't opt in to the more restrictive behavior.

[lrhn]

One thing I would encourage thinking about is whether this is a one-off situations, or we are expecting other similar restrictions to become issues in the future.

If it's one-off, then the generality of the solution is not important. If we are going to have a significant number of similar restrictions, then we probably want to have a more general framework around them, with a consistent way to access the "platform configuration" from code.

[sortie]

I'll take a closer look at the issue at hand and think about the best solution. My gut feeling right now is that this is probably best enforced by the embedder and could be controlled by some configuration file in a Flutter project, much like the existing app permissions and such already are.

[mehmetf]

I am wondering whether these defaults should be configurable at the project level. My gut feeling right now is no. That's why I asked @Hixie above whether it would be fine making this proposal the default behavior for all Flutter apps.

If defaults are not configurable, then I would love to have platform-specific configuration to core libs without involving the embedder. Cases like this are not specific to Flutter. They are specific to platform behavior. iOS and Android ban HTTP communication regardless of how Dart VM is being embedded. I also believe Fuchsia ban on dart:io#exit is not specific to Flutter. Therefore, I feel like making these configurations pass through embedder is unnecessary indirection.

I agree with @lrhn that peppering Platform.isXXX all over the place is too adhoc. However, I would love to have a central _PlatformConfig class as part of core libs that return a particular set of defaults specific to a platform. This would give Dart the power to affect platform configuration that is entirely contained within the Dart language and does not require messing around with C code or Tonic.

This decision would also be easy to reverse. If the use case arises that would require configurable defaults, we can get rid of _PlatformConfig and switch to a configuration file setup since it is entirely private and not mutable/usable by the user project.

[mehmetf]

If what we want is to have configurable defaults, then I am happy to collaborate on this @sortie but my hopes are not high.

There's already a mechanism to pass VM flags in each embedder. However, as noted above, it is not designed to support passing configuration in release mode (and it would be undesirable to affect the behavior of VM via flags in release mode). That would mean creating an entirely different mechanism. For instance, the canonical way to represent static configuration on Android is via the manifest file. You would need to do the following to send this all the way to Dart:

• Read Manifest and extract values in Java using canonical Android APIs.
• Convert these values to flutter engine switches.
• Add the flag definitions to switches.h
• In Android C++ code, handle these switches and delegate them to Dart via Tonic

This is just Android. You would need the equivalent of this for every platform. For covering a use case that does not yet exist, this is a tall order.

[zanderso]

The embedder (e.g. Flutter engine targeting iOS/Android) can set the value of a static getter in HttpClient that controls whether cleartext is allowed, via _EmbedderConfig. Then the value of that static getter can be overriden with a one liner at the top of main() by setting HttpOverrides.global, or more locally, with HttpOverrides.runWithHttpOverrides().

[mehmetf]

@zanderso What do you think about having Platform specific defaults in core libs itself? It feels unnecessary to me that OS-mandated behavior is configured statically by the embedder.

For instance, why would we not do this:

class _PlatformConfig {
  final bool isolateCanExit;
  final bool ...;

  const _PlatformConfig(...);
}

const _PlatformConfig _fuchsiaConfig = _PlatformConfig(
  isolateCanExit: false,
  httpCleartextEnabled: true,
  ...
);

const _PlatformConfig _iOSConfig = _PlatformConfig(
  isolateCanExit: true,
  httpCleartextEnabled: false,
  ...
);

_PlatformConfig get platformConfig {
  if (Platform.isIOS) return _iOSConfig;
  ...
}

as opposed to using Tonic to reach in and change static values in various classes from the embedder?

This only works if the defaults are not configurable and really are a property of the underlying platform (not the embedder). That's the only use case I have seen so far.

[zanderso]

@mehmetf I was mostly under the impression that @lrhn wanted to keep the Platform.isXXX logic to a minimum in the core libraries, but it's possible I misunderstood.

[lrhn]

I did.

I think trying to create a "configuration object" and a set of known configurations is the wrong direction to take.

Things that you might want to configure is an open-ended enum. It's not something that is best handled by code with a fixed API. The set of configurations is also open-ended. Trying to enumerate them in code is a bad idea. That means that when someone ports Dart to the SugarOS carbohydrate based computers in 2053, we need to update the platform libraries with a const _PlatformConfig _sugarOSConfig = ... and add another case to get platformConfig.

A better design for the "configuration" would be a set of capabilities (not a Set of anything, just basically just a way to query for each configurable thing that you know about).

The best way to introduce such a configuration is also not code. It should come from the environment in some way, whether that's the compilation environment or the run-time environment or the platform build system or hardcoded into each run-time binary ... whatever makes it work without having to fix any particular configuration into code.

The general rule I'm leaning on here is that open ended enumeration should not be named in code because that makes them hard to grow from the outside. (And yes, the isIOS, isAndroid, isWindows, etc. getters is a particular good example of such a bad design. Whenever someone new wants to use Dart on a new platform, we'll have to add their is getter to not make them feel left out. I argued long against adding isFuchsia :smile:).

[Hixie]

I was imagining something much simpler, where you just have HttpClient not support http out of the box anymore, and to use http you'd have to explicitly say:

HttpClient client = new HttpClient(allowInsecureHttp: true);

...and without that argument, you can't use unencrypted HTTP.

If google3 needs something stricter then we could add a lint that disallows using that flag, or something.

[mehmetf]

@lrhn Sounds good. I would like to adhere to the principles that the SDK follows. I will add a capability similar to _EmbedderConfig to _http lib. We can use this pattern for future capabilities in other core libs.

[mehmetf]

@Hixie while that's tempting, I would like to reduce the scope of this change to the intended platforms. We have many tools written in Dart that run on desktop. While it would be great if https was banned from all of them, I feel it is too big of a change.

We can decide this later and just flip the default found in _EmbedderConfig to disallow (That will give embedders the chance to opt out).

[Hixie]

It's a very small change, just requires one new argument on each HttpClient constructor call site. Given the scope of other breaking changes we are considering on a regular basis (removing implicit casting, default non-nullability, lots of APIs changing to not return null anymore, etc), this seems like a more or less trivial change, and one that is easily handled by lints (e.g. we could add a default-on lint that warns you about the new argument if you don't specify it).

[mehmetf]

My response to you was regarding platform defaults. I would like to enable this primarily for iOS and Android.

I don't want to add a new argument to the HttpClient because many applications do not use dart:io directly. Instead they use package:http or Image.network() from package:flutter or something else. These packages would then either need to default http support to something or expose their own named parameter (and their own lint) to configure this capability. It would require ongoing maintenance. It is preferable to centrally control this via HttpOverrides.

[mehmetf]

I edited the proposal above to reflect the discussions.

[Hixie]

(Zone.current[allowClearText] ?? _EmbedderConfig.allowClearText) == false

Invoking Zones on a security feature seems like a footgun, given the complexity of Zones. I'd really much prefer it just be a constructor argument. Ambient authority (which is what global zone variables, or indeed any global variant, essentially boil down to) generally leads to security bugs.

I'm also concerned that setting _EmbedderConfig.allowClearText to false does not disallow cleartext. This also seems like a footgun.

Having this differ from platform to platform also seems like a security problem waiting to happen.

In general I am increasingly agitated the more I think about this. :-)

I really would much rather, if we do anything here at all, that we just always default to http being disallowed, and have an explicit flag on the constructor that enables it. Yes, this means that people who wrap this API will need to expose whether or not to allow HTTP, but if we think HTTP is a security problem, then that seems like the right choice. This is no different, IMHO, to what you would have to do if you wanted to allow self-signed certificates, or enable the use of a private certificate on requests, or enable proxy authentication.

In fact, it seems almost identical to the issue of self-signed certificates, for which we already have a solution. Maybe the right thing to do here is for us to actually use that same badCertificateCallback for all non-encrypted connections, but passing it a special cert instance that represents "no cert"?

[mehmetf]

We definitely need to do something :-).

I need @mit-mit or another Dart authority to make a decision on the defaults. I am abiding by policy here which only covers iOS and Android apps.

Can you expand on the badCertificateCallback suggestion? Links to code?

(I am still not convinced the benefits outweigh the costs of introducing a ctr argument; if it comes to that let's discuss offline.)

[mehmetf]

I see https://api.dart.dev/stable/2.7.1/dart-io/HttpClient/badCertificateCallback.html.

If you are directly using HttpClient, this is probably fine. It would mean it is not a breaking change for classes that implement HttpClient. However, same cost arguments apply to every library that wraps HttpClient. I will chat with you offline so we can reach a conclusion.

[zanderso]

@Hixie Without advocating for either approach, I'd like to clarify that extending and using the HttpOverrides API doesn't require direct manipulation or inspection of Zones. The API encapsulates Zones in way that I believe is not as error-prone as @mehmetf has presented it.

[lrhn]

I'm still very concerned that we might be addressing this at the wrong level.

There is nothing inherently wrong with HTTP connections, but in some situations there is a policy disallowing them. That's business logic, not platform logic. We should not be putting any business logic into the platform level code.

We do need to support business logic choices, which means that the platform needs some hook that can intercept the undesired usage and prevent it if necessary. I would prefer if that logic was general enough that it could be used for other things than just throwing an error. (For example, what if someone decided they want to intercept the connection and upgrade it to HTTPS instead, it would be nice if we didn't have to add one more hook to allow that as well).

So, if at all possible, I'd like what we do to be generalizable, not too specific.

I'm also worried about the incentives that the design puts onto users.

If we add the allowHttpConnection(body) call, then we are effectively documenting that general library code cannot trust an HTTP connection to work. Users might start wrapping their entire application in such a call, just to be sure, even if they are not sure they are affected (and even if they are indeed not affected). It's a design that works when used and understood correctly, but it encourages unnecessary overhead, CYA, and cargo cult code when it's not understood. It's not clearly delineated what is allowed when ("the environment you run in decides") so users are left to guess. They usually do that badly, or at least defensively.

And finally, after reading this thread, the CL, and the linked document, I'm still not sure exactly what the precise requirements are that we are trying to solve for.

Is the requirement:

Flutter applications must not be able to make an web request using an HTTP schema URI without explicitly opting in to allowing such.

?

If that is the case, then we can add one deep hook in dart:io that the flutter embedder can attach to, and add the override allowHttpConnections method to package:flutter (or some other Flutter specific library).

If not, what is the actual requirement?

[mehmetf]

Lasse and I synced offline and are in agreement. His primary concern was exposure of allowHttp override at dart:io level. Any client who sees that would not be able to discern whether setting it is necessary or not.

Since the requirement is Flutter only (and the defaults are exposed only to embedders), the allow overrides functionality should only be exposed in embedders as well; not in core Dart. I will move that function to dart:ui or package:flutter.