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

android: different ClientHello between debug and release mode #1444

Closed
bassosimone opened this issue Apr 9, 2021 · 3 comments · Fixed by ooni/probe-cli#315
Closed

android: different ClientHello between debug and release mode #1444

bassosimone opened this issue Apr 9, 2021 · 3 comments · Fixed by ooni/probe-cli#315
Assignees
Labels
bug Something isn't working priority/high

Comments

@bassosimone
Copy link
Contributor

bassosimone commented Apr 9, 2021

Now that I understand the problem, it's actually quite simple. (It's always like that.)

  1. The TLS codebase chooses whether to put AES at top or not depending on hardware flags.

  2. Hardware flags are initialized from the system auxv.

  3. Reading the system auxv on Android depends on /proc/self/auxv being readable.

  4. If /proc/self/auxv is not readable, the code will not initialize the hardware capabilities and it will just attempt to determine the page size.

  5. It's documented that Go will not be able to open /proc/self/auxv in release mode (see x/mobile/app: cannot open /proc/self/auxv: Permission denied golang/go#9229) in many (all?) cases.

  6. It's documented that in some networks not having the AES cipher at top causes a TLS protocol mismatch that prevents any TLS communication (see Deep packet inspection to classify V2Ray traffic in Dec, 2020 v2fly/v2ray-core#557).

The way in which v2ray developers fixed the issue seem to be using uTLS. We are already investigating this possibility. We are also considering modifying Go and using the modified Go for now, while we continue working on uTLS.

@bassosimone
Copy link
Contributor Author

bassosimone commented Apr 20, 2021

One option that I am considering is to patch the src/runtime of the Go used to build for Android as follows:

diff --git a/src/runtime/os_linux.go b/src/runtime/os_linux.go
index 058c7daf9c..9afc6ca2a3 100644
--- a/src/runtime/os_linux.go
+++ b/src/runtime/os_linux.go
@@ -234,6 +234,7 @@ func sysargs(argc int32, argv **byte) {
 			physPageSize = size
 		}
 		munmap(p, size)
+		sethwcap()
 		return
 	}
 	var buf [128]uintptr
diff --git a/src/runtime/sethwcap_android_arm64.go b/src/runtime/sethwcap_android_arm64.go
new file mode 100644
index 0000000000..ab15bcd429
--- /dev/null
+++ b/src/runtime/sethwcap_android_arm64.go
@@ -0,0 +1,38 @@
+// +build android
+// +build arm64
+
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+//
+// See https://go-review.googlesource.com/c/sys/+/197540/
+
+package runtime
+
+import "internal/cpu"
+
+/*
+#include <sys/auxv.h>
+
+// getauxval is not available on Android until API level 20. Link it as a weak
+// symbol and use other methods as fallback.
+unsigned long getauxval(unsigned long type) __attribute__((weak));
+*/
+import "C"
+
+// getauxval returns the value of the getauxval auxiliary vector getter, or
+// zero where the functionality is unavailable.
+func getauxval(t uint) uint {
+	if C.getauxval == C.NULL {
+		return 0
+	}
+	return uint(C.getauxval(C.ulong(t)))
+}
+
+// sethwcap uses getauxval to set cpu.HWCap on
+// linux_arm64 and does nothing otherwise.
+func sethwcap() {
+	// getauxval returns zero on failure and, if we end up here, we
+	// already have a zero cpu.HWCap anyway.
+	cpu.HWCap = getauxval(_AT_HWCAP)
+}
diff --git a/src/runtime/sethwcap_otherwise.go b/src/runtime/sethwcap_otherwise.go
new file mode 100644
index 0000000000..a6bffbef97
--- /dev/null
+++ b/src/runtime/sethwcap_otherwise.go
@@ -0,0 +1,7 @@
+// +build !android !arm64
+
+package runtime
+
+// sethwcap uses getauxval to set cpu.HWCap on
+// linux_arm64 and does nothing otherwise.
+func sethwcap() {}

This diff is annoying because it makes runtime depend on C. But, if it's working as intended, we have now a clean way to correctly initialize the hardware capabilities of the processor on Android arm64 devices. Tomorrow I'll test.

(I also explored forcing AES as the default in every case, but it makes me nervous knowing that the non-ASM implementation of AES in Golang is not constant time. I do not fully understand the implications but it does not seem right to unconditionally promote AES to be on top when there's no hardware AES support.)

bassosimone added a commit to ooni/go that referenced this issue Apr 21, 2021
There are Android devices where reading /proc/self/auxv fails
with permission denied. Android is increasingly preventing apps
and tools from accessing system functionality.

Go uses /proc/self/auxv to fill cpu.ARM64. In turn, it uses
the values in this data structure to choose whether to enable
an assembly implementation of AES as well as to choose the
order of the cipher suites in the TLS ClientHello.

Unfortunately, as documented in ooni/probe#1444,
reordering the ciphers in the ClientHello triggers censorship
in Iran. Because new(?) Android devices do not allow Go to access
/proc/self/auxv, we therefore see that OONI is not WAI.

The most obvious fix to this issue could seem to teach crypto/tls to
always put AES ciphers on top. Unfortunately, the pure Go AES
implementation in the standard library is not constant time (while
the one in assembly is constant time). Therefore, forcing AES to
always be at the top of the ciphers list does not seem right.

Thus, we can say that our real core problem is that Go is not able
to know whether we support AES and PMULL on android/arm64.

This diff attempts to solve this problem. The solution I have
chosen is to implement a cpuproxy package exposing predicate
functions describing the CPU's support for AES and PMULL.

This cpuproxy package will use the existing logic for every GOOS
and GOARCH except android/arm64. When we are in android/arm64,
we use `getauxval(3)` to obtain the hardware capabilities and we
use this information to implement the predicates.

My initial attempt at solving this problem was to call `getauxval(3)`
directly during the initialization of the `runtime` package. But it
seems that when you use CGO you depend on the `syscall` package, and
in turn `syscall` depends on `runtime`. So, when I was compiling OONI
for Android, I always got a circular-import hard failure.

Another approach could be that of reading `/proc/cpuinfo` inside of
the `runtime` initialization. I did not explore it.

This solution based on `libc` and `getauxval(3)` is not perfect but we
already build OONI for Android with CGO enabled, so we should be fine
in terms of the functionality that we require as OONI.

I also adjusted the tests to reflect the fact that now there are more
packages that depend on CGO (when using Android).
@bassosimone
Copy link
Contributor Author

It may be that the right fix for this issue is to patch golang/mobile rather than golang/go. The current solution is patching golang/go and it seems the effort to research how to patch golang/mobile would further postpone shipping a fix. We want to engineer the next release using the current patch for golang.go (implemented in ooni/probe-cli#311) and to continue investigating whether there are simpler fixes to this issue after that. We should also consider whether there is a way to upstream the fix.

@bassosimone
Copy link
Contributor Author

The PR at ooni/probe-cli#315 introduces a new build script that takes care of using our Go fork when we are compiling for Android. It might be that there are better ways to do that, perhaps patching golang/mobile instead. For now, it's fine to use this solution, which is based on a variation of the above patch (see github.com/ooni/go for more info). We are going to close this issue soon.

bassosimone added a commit to ooni/probe-cli that referenced this issue Apr 27, 2021
This PR introduces the `./make` script. For now, this script knows how to make Android releases. We have code in #311 that builds other artifacts in this repository.

Because our current concern is ooni/probe#1444, in this PR, I have chosen to keep the Android code necessary to fix ooni/probe#1444.

Additionally, this PR removes the script to publish to Bintray. This change part of the ooni/probe#1440 cleanup effort.
bassosimone added a commit to ooni/go that referenced this issue Nov 10, 2021
This diff pulls code from https://go-review.googlesource.com/c/sys/+/197540/
to add better support for detecting arm64 CPU features.

I extracted this diff from 62177de with
the intent of coming up with a more manageable patch set.

The idea here is to use this better cpuproxy functionality to feed
crypto libraries with better understanding of cpu capabilities.

The original issue describing this problem in its full extent was
ooni/probe#1444.

We're now working to forward-port this patch to go1.17.

AFAICT, as of go1.17, the src/runtime/os_linux.go still falls back to
reduce functionality in case /proc/self/auxv isn't readable:

https://github.com/golang/go/blob/go1.17.3/src/runtime/os_linux.go#L216

The current issue describing this problem as of go1.17 is
ooni/probe#1863.

Is there a better way?
bassosimone added a commit to ooni/go that referenced this issue Nov 10, 2021
I extracted this diff from 62177de with
the intent of coming up with a more manageable patch set.

The idea here is to take advantage of the cpuproxy package to use the
information it provides to better initialize crypto using arm64.

The original issue describing this problem in its full extent was
ooni/probe#1444.

We're now working to forward-port this patch to go1.17.

The current issue describing this problem as of go1.17 is
ooni/probe#1863.

Is there a better way?
bassosimone added a commit to ooni/go that referenced this issue Nov 10, 2021
I extracted and adapted this diff from 62177de
with the intent of coming up with a more manageable patch set.

The idea here is to take advantage of the cpuproxy package to use the
information it provides to better initialize crypto using arm64.

The original issue describing this problem in its full extent was
ooni/probe#1444.

We're now working to forward-port this patch to go1.17.

The current issue describing this problem as of go1.17 is
ooni/probe#1863.

Is there a better way?
bassosimone added a commit to ooni/go that referenced this issue Nov 10, 2021
I extracted and adapted this diff from 62177de
with the intent of coming up with a more manageable patch set.

The idea here is to fix the tests because we have changed the imports
expected by some packages inside of src/crypto to use i/cpuproxy.

The original issue describing this problem in its full extent was
ooni/probe#1444.

We're now working to forward-port this patch to go1.17.

The current issue describing this problem as of go1.17 is
ooni/probe#1863.

Is there a better way?
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
This PR introduces the `./make` script. For now, this script knows how to make Android releases. We have code in ooni#311 that builds other artifacts in this repository.

Because our current concern is ooni/probe#1444, in this PR, I have chosen to keep the Android code necessary to fix ooni/probe#1444.

Additionally, this PR removes the script to publish to Bintray. This change part of the ooni/probe#1440 cleanup effort.
bassosimone added a commit to ooni/oocrypto that referenced this issue May 22, 2022
This diff pulls code from https://go-review.googlesource.com/c/sys/+/197540/
to add better support for detecting arm64 CPU features.

The idea here is to use this better proxy functionality for arm64 to feed
crypto libraries with better understanding of cpu capabilities.

The original issue describing this problem in its full extent was
ooni/probe#1444.

The ooni/go@7807667
diff originally solved this problem for the github.com/ooni/go fork
and the branch tracking go1.17.x development.

AFAICT, as of go1.17, the src/runtime/os_linux.go still falls back to
reduced functionality in case /proc/self/auxv isn't readable:

https://github.com/golang/go/blob/go1.17.3/src/runtime/os_linux.go#L216

The current issue describing this problem as of go1.17 is
ooni/probe#1863.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority/high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant