search

muun / apollo

Muun Android wallet
https://muun.com
MIT License
256 stars 46 forks source link

Please fix build reproducibility #54

Open Giszmo opened 2 years ago

Giszmo commented 2 years ago

Your verification script claims

# Remove the signature since OSS users won't have Muuns private signing key

but then goes to remove all the content of META-INF and resources.arsc.

While the former is probably benign as META-INF content won't be executed or displayed in the app, removing resources.arsc from the diff is not ok.

As my review shows, the diff boils down to just one line in the strings.xml:

$ apktool d -o apkGoogle Muun\ 46.10\ \(io.muun.apollo\).apk 
$ apktool d -o apkBuild apolloui-prod-release-unsigned.apk
$ diff --brief --recursive apkBuild apkGoogle
Files apkBuild/apktool.yml and apkGoogle/apktool.yml differ
Only in apkGoogle/original/META-INF: APOLLORE.RSA
Only in apkGoogle/original/META-INF: APOLLORE.SF
Files apkBuild/original/META-INF/MANIFEST.MF and apkGoogle/original/META-INF/MANIFEST.MF differ
Files apkBuild/res/values/strings.xml and apkGoogle/res/values/strings.xml differ
$ diff apkBuild/res/values/strings.xml apkGoogle/res/values/strings.xml
77c77
<     <string name="com.crashlytics.android.build_id">e0c37a103082460fbf95f3c097222e61</string>
---
>     <string name="com.crashlytics.android.build_id">95a3152a98594e8ca1324bdefd26a5b9</string>

Crashlytics is a convenient library but also an increased attack surface and if their tools are not compatible with reproducible builds, then that's another reason for not using them.

Please fix reproducibility. I would really love to add Muun to the "reproducible" section at https://walletscrutiny.com/

Giszmo commented 2 years ago

Is there any update on this issue? I tried to investigate how to make the build_id deterministic but couldn't find anything neither. Crashlytics itself is also problematic anyway, as it's phoning home and could be abused to leak keys, either by the wallet provider or the crashlytics provider, so I would prefer to see it removed altogether.

champo commented 2 years ago

Hi Leo, we don't plan on fixing this in the near future. We sadly don't have the bandwidth right now to deal with the work this issue entails.

Replacing crashlytics with a nicer alternative is not an insane amount of work but it's not trivial either: we'd need to consider the alternatives and choose one that both gives us high-quality crash reporting, fixes the reproducibility issue, and improves on security and privacy concerns. I spent some time researching alternatives and didn't find any obvious answers here.

So while I agree this issue makes automatic verification harder, and we do want to do better here, it is humanly verifiable right now. Weighing all the factors I've mentioned means we'll take a while to get back to this and fix it.

Thanks for the work you put in!

Giszmo commented 2 years ago

Understood. I would personally offer $200 to fix this issue by replacing the random build_id with a build_id derived from the git revision for example.

Giszmo commented 2 years ago

I poked crashlytics.

Giszmo commented 2 years ago

Sadly for the last two versions - 49.2 and 49.3 - the diff is more than just crashlytics.

champo commented 2 years ago

Thanks for the heads up. We'll look into it ASAP.

champo commented 2 years ago

Well, it looks like the issue is one you've found before: sqldelight is generating non-reproducible code. We recently updated to the latest version and started hitting the issue you reported. I believe the only way around this is to use a forked version of SQLDelight. The issue is fixed for the new 2.0 version, but doesn't seem like there's a backport to 1.5. If you're aware of any workarounds please let me know!

devrandom commented 2 years ago

There are about 10 reproducible Bitcoin wallets on the Play Store. Not having a reproducible build is a competitive disadvantage, especially when people use https://walletscrutiny.com/?verdict=reproducible&platform=android for their starting point.

Without a reproducible build, one might consider the wallet to be effectively closed-source, since it's difficult or impossible to ascertain what sources were used for any particular APK.

emanuelb commented 2 years ago

latest version v49.9 has more diff, tested with instructions at: https://github.com/muun/apollo/blob/master/BUILD.md#verify-an-existing-apk

used podman instead with command: (after removing last 3 lines in Dockerfile, to be able to access the build container and copy apk from it) https://github.com/muun/apollo/blob/cc0e92c11053ecc28b541f793355ab28f05663f2/android/Dockerfile#L68-L71

podman build --rm -t muun_build_apk -f android/Dockerfile .
podman run --rm -uroot --name muun_build_apk --entrypoint /bin/bash -ti muun_build_apk
podman cp localhost/muun_build_apk:/src/android/apolloui/build/outputs/apk/prod/release/apolloui-prod-release-unsigned.apk ~/muun/LocalBuild/l.apk

diff result 

Files ./LocalBuild/assets/dexopt/baseline.prof and ./FromGplay/assets/dexopt/baseline.prof differ
Files ./LocalBuild/classes2.dex and ./FromGplay/classes2.dex differ
Files ./LocalBuild/classes.dex and ./FromGplay/classes.dex differ
Files ./LocalBuild/lib/arm64-v8a/libgojni.so and ./FromGplay/lib/arm64-v8a/libgojni.so differ
Files ./LocalBuild/lib/armeabi-v7a/libgojni.so and ./FromGplay/lib/armeabi-v7a/libgojni.so differ
Files ./LocalBuild/lib/x86/libgojni.so and ./FromGplay/lib/x86/libgojni.so differ
Files ./LocalBuild/lib/x86_64/libgojni.so and ./FromGplay/lib/x86_64/libgojni.so differ
Only in ./FromGplay/META-INF: APOLLORE.RSA
Only in ./FromGplay/META-INF: APOLLORE.SF
Only in ./FromGplay/META-INF: MANIFEST.MF
Files ./LocalBuild/resources.arsc and ./FromGplay/resources.arsc differ
acrespo commented 2 years ago

Hi there! We're finally rolling out v49.10. We've fixed the issue with the SQLDelight Gradle Plugin generating non-reproducible code, as well as the issue @emanuelb reported, in which go 1.18 started embedding version control information in binaries, which made Go Mobile generate non-reproducible code depending on whether the local git repository where you build the apk had modified files or not.

Thank you for your reports and your patience!

Giszmo commented 2 years ago

Great to hear! WalletScrutiny is currently on hold for lack of financing but will probably be back at reviewing binaries starting in October. I did mark the muun review as "outdated". As soon as funding is secured, all those "outaded" apps are top priority.

mohammadrafigh commented 1 year ago

I tested the latest release v50.13 and there is still a diff in resources.arsc:

$ DOCKER_BUILDKIT=1 docker build -f android/Dockerfile -o apk .
$ diff --brief --recursive /tmp/fromPlay_io.muun.apollo_1013 /tmp/fromBuild_io.muun.apollo_1013
...
Diff:
Only in /tmp/fromPlay_io.muun.apollo_1013/META-INF: APOLLORE.RSA
Only in /tmp/fromPlay_io.muun.apollo_1013/META-INF: APOLLORE.SF
Only in /tmp/fromPlay_io.muun.apollo_1013/META-INF: MANIFEST.MF
Files /tmp/fromPlay_io.muun.apollo_1013/resources.arsc and /tmp/fromBuild_io.muun.apollo_1013/resources.arsc differ

Since resources.arsc is a binary digging into it is hard but there are many lines of diff when it's converted to hex so posting many lines of hex diffs here won't help. But please let me know if it could help.

emanuelb commented 1 year ago

resources.arsc can be unpacked and compared manually as explained in: https://salsa.debian.org/reproducible-builds/diffoscope/-/issues/115 which is more useful then hex diff.

it's worth to recompile with using disorderfs as the diff might be caused by ordering issue that disorderfs fix (like happened for other wallet in: https://gitlab.com/walletscrutiny/walletScrutinyCom/-/issues/238#note_591641562 )

mohammadrafigh commented 1 year ago

@emanuelb thank you. After unpacking resources.arsc the only diff was related to Crashlytics build_id again:

$ aapt2 dump resources apollo.apk > fromPlay.txt
$ aapt2 dump resources /tmp/test_io.muun.apollo/app/apk/apolloui-prod-release-unsigned.apk > fromBuild.txt
$ diff fromPlay.txt fromBuild.txt 
11708c11708
<       () "e2e27f098f0f4dd5bd472ec4d8b0ba2d"
---
>       () "10abb903957f4fe18afcf69d8156997d"
devrandom commented 1 year ago

@mohammadrafigh why is this considered non-reproducible? the build_id doesn't affect functionality, right?

mohammadrafigh commented 1 year ago

As discussed above generally it might not affect the functionality. But in WalletScrutiny accepting benign looking diffs for a reproducible label is a slippery slope. This build id can and should be fixed. Eventhough we mentioned that this diff is benign in this review.