google / google/crumbles

Multiple log-loss, key-lifecycle, and decryption defects

Open
#5 8 comments 0 reactions 1 assignee Claimed by @iammyr View on GitHub
Dominant language
Java
Stars
10
Forks
3
PR merge metrics
No merged PRs in 30d

Description

## Summary

Reviewing current `main` at commit `ebb6b381af36f862b04b5a8d89073a492eea637e` found several independent defects that can silently lose collected logs, create files that cannot be decrypted, or crash the decrypted-log viewer.

This issue intentionally does **not** duplicate the protobuf runtime crash reported in #3. The clean-checkout build problem is already tracked in #4 and is only noted under validation.

## 1. External-key logging silently drops retrieved batches

`CrumblesDeviceAdminReceiver.encryptLogs()` creates a new `CrumblesLogsEncryptor` for every callback:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesDeviceAdminReceiver.java#L333-L354

The configured external key is held in the separate singleton encryptor used by the UI. Selecting an external key also deletes the primary internal key:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesLogsEncryptor.java#L140-L145

The fresh receiver-side encryptor therefore has neither an external public key nor an internal key, and `encryptLogs()` returns `null`. This happens only after `retrieveSecurityLogs()` or `retrieveNetworkLogs()` returned the batch, so the device-policy batch has already been consumed.

### Reproduction

1. Import or generate an external encryption key.
2. Enable security/network logging.
3. Force or wait for a log callback.
4. Observe `Failed to encrypt logs: logBatch is null` and no output file.

### Suggested fix

Resolve the persisted active key before retrieving logs, inject the same configured key source into the receiver, and do not retrieve a batch unless encryption/storage is ready.

## 2. Files are marked sent and deleted without upload confirmation

The sending worker renames files to `_processing.bin` and considers posting a notification successful:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesSendAndMarkProcessingWorker.java#L121-L138

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesSendAndMarkProcessingWorker.java#L197-L243

An independent periodic worker starts with a ten-minute offset and unconditionally renames every processing file to `_sent.bin`:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesWorkScheduler.java#L76-L101

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesMarkProcessingAsSentWorker.java#L53-L93

No signal shows that the user opened the notification, selected a destination, or completed an upload. Renaming also invalidates the `FileProvider` URI retained by the notification. The daily deletion worker later removes the file.

This is particularly risky for zero-touch provisioning on Android 13+, because notification permission is requested only when `CrumblesMain` is opened. A provisioned device can schedule the workers without ever showing that activity.

### Suggested fix

Do not infer delivery from elapsed time. Keep files pending until an explicit, verifiable completion signal exists. If the platform share sheet cannot provide such confirmation, retain the original file and let the user explicitly mark/delete it. Check notification permission before changing file state.

## 3. Cancelling private-key export leaves an unrecoverable active key

The exportable public key is persisted and activated before the private-key export UI completes:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesManageExternalKeysActivity.java#L198-L217

Cancelling the format-choice dialog zeroes the only private-key byte array but does not roll back the active public key:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesManageExternalKeysActivity.java#L224-L246

Future logs are then encrypted to a key for which no private key exists. Activity destruction during the export flow creates a similar risk.

### Suggested fix

Treat key replacement as a transaction: generate a candidate pair, let the user export and explicitly confirm custody, then persist/activate its public key. Preserve the previous key until confirmation and roll back on cancellation or lifecycle interruption.

## 4. Collected logs are serialized in one format and displayed as another

Collection serializes `List` using Java object serialization:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesDeviceAdminReceiver.java#L327-L330

After decryption, the UI decodes those bytes directly as UTF-8:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesMain.java#L538-L545

The displayed result therefore includes Java serialization headers/binary data rather than the original readable log entries. Current tests mock decryption with ordinary UTF-8 bytes and do not exercise the production collection-to-viewer path.

### Suggested fix

Use a defined format end to end, preferably a protobuf message or UTF-8 JSON/text. Add an integration test that serializes a real collected list, encrypts it, decrypts it, and verifies the rendered entries.

## 5. Internal re-encryption keys produce files the app cannot decrypt

The UI generates and offers keys under `re_encrypt_*` aliases:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesReEncryptKeysActivity.java#L138-L172

However, `decryptLogs()` always loads the primary `KEY_ALIAS`:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesLogsEncryptor.java#L272-L304

The `LogBatch` also contains no usable key alias/fingerprint. A file re-encrypted to an internal `re_encrypt_*` key therefore cannot be opened through the app even though the corresponding private key remains in the Keystore.

### Suggested fix

Store a cryptographic key identifier in authenticated metadata and resolve the correct Keystore alias during decryption, or remove internal re-encryption keys from the UI until their full lifecycle is implemented.

## 6. Decrypted-log handoff can exceed the Binder transaction limit

Each decrypted batch is retained twice—as a `String` and as raw bytes—inside a serializable list passed through an activity Intent:

https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesMain.java#L512-L561

Multiple file selection is supported. Android's Binder transaction buffer is approximately 1 MiB and shared across process transactions, so realistic batches can make `startActivity()` throw `TransactionTooLargeException`.

### Suggested fix

Store decrypted results in app-private temporary files and pass small identifiers or `FileProvider` URIs. Avoid placing log bodies in Intent extras or saved instance state.

## Additional security and correctness concerns

- `LogMetadata.device.device_id` is hard-coded to `"123456789"`, so logs from different devices cannot be reliably attributed:
https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesLogsEncryptor.java#L419-L429
- Private-key bytes are stored in fragment arguments and displayed without `FLAG_SECURE`, allowing screenshots/recent-task previews and retaining extra key copies across fragment state:
https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/src/main/java/com/android/securelogging/CrumblesPrivateKeyViewerDialogFragment.java#L52-L84
- AES-GCM authenticates the encrypted payload but no batch metadata is supplied as AAD, so timestamp, device ID, blob size, encryption type, and key-encryption type can be modified without detection.
- The macOS setup script uses a release URL missing the `/download/` path component:
https://github.com/google/crumbles/blob/ebb6b381af36f862b04b5a8d89073a492eea637e/setup_device.command#L33-L34

## Validation limitations

The documented clean-checkout Gradle command cannot currently run because the build files and wrapper are absent; that separate problem is tracked in #4. The defects above were confirmed by source-flow analysis and comparison with the `v1.0` tag. The core paths for findings 1–6 are also present in the published v1.0 source.

## Recommended regression coverage

1. Full device-admin callback → encryption → file → decryption → rendering round trip.
2. External-key collection test using a fresh receiver instance.
3. Upload notification cancellation/no-action test proving files remain pending.
4. Exportable-key cancellation and activity-recreation tests.
5. Decryption test for every advertised re-encryption key type.
6. Multi-file payload test exceeding 1 MiB without using Intent extras.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.