Skip to content

Device identity: CSR signed with non exportable TEE/StrongBox Key - #376

Merged
pandey019 merged 7 commits into
wootzapp:chromiumfrom
1311-hack1:DeviceIdentity
Sep 23, 2025
Merged

pandey019 merged 7 commits into
wootzapp:chromiumfrom
1311-hack1:DeviceIdentity

Conversation

@1311-hack1

@1311-hack1 1311-hack1 commented Sep 22, 2025 •

Copy link
Copy Markdown
Contributor

User description

Reference to issue #358 and continuation of PR #373


PR Type

Enhancement


Description

  • Implement CSR generation with hardware-backed key signing

  • Replace AsyncTask with Chromium's network thread architecture

  • Add OpenSSL-based PKCS#10 CSR creation functionality

  • Update enrollment API to use CSR-based authentication


Diagram Walkthrough

flowchart LR
  A["Device Enrollment"] --> B["Request Nonce"]
  B --> C["Generate Hardware Key"]
  C --> D["Create CSR with OpenSSL"]
  D --> E["Sign CSR with Hardware Key"]
  E --> F["Submit CSR + Attestation Chain"]
  F --> G["Receive DIC Certificate"]
Loading

File Walkthrough

Relevant files
Enhancement
wootz_keystore.cc
Add OpenSSL-based CSR generation with hardware signing     

src/net/android/wootz_keystore.cc

  • Add OpenSSL includes for CSR generation functionality
  • Implement GenerateCSR function with PKCS#10 compliance
  • Add helper functions for X509_NAME creation and key validation
  • Implement hardware key signing for CSR using existing
    SignWithHardwareKey
  • Add JNI method JNI_WootzHardwareKeyStore_GenerateCSR for Java
    integration
+343/-0 
WootzDeviceEnrollment.java
Migrate to thread-based networking with CSR enrollment     

src/net/android/java/src/org/chromium/net/WootzDeviceEnrollment.java

  • Replace AsyncTask with dedicated Thread and Handler pattern
  • Switch from challenge-based to nonce-based enrollment flow
  • Add Bearer token authentication for API requests
  • Integrate ChromiumNetworkAdapter for network traffic annotation
  • Update enrollment submission to use CSR instead of certificate chain
+162/-132
WootzEnrollmentUtils.java
Add CSR enrollment JSON utilities                                               

src/net/android/java/src/org/chromium/net/WootzEnrollmentUtils.java

  • Add createCSREnrollmentRequestJson method for new API format
  • Implement convertPemChainToArray for certificate chain parsing
  • Add escapeJsonString utility for proper JSON encoding
  • Support CSR-based enrollment payload structure
+98/-0   
WootzHardwareKeyStore.java
Add CSR generation with native OpenSSL integration             

src/net/android/java/src/org/chromium/net/WootzHardwareKeyStore.java

  • Add @NativeMethods interface for JNI CSR generation
  • Implement generateCSR method calling native OpenSSL implementation
  • Add generateDeviceIdentifier for unique device identification
  • Update signWithHardwareKey documentation for CSR signing context
+83/-4   
wootz_keystore.h
Add CSR generation function declaration                                   

src/net/android/wootz_keystore.h

  • Add GenerateCSR function declaration with OpenSSL implementation
  • Document CSR generation parameters and return format
  • Extend header with PKCS#10 compliance documentation
+16/-0   

@qodo-code-review

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

358 - Partially compliant

Compliant requirements:

  • Generate a PKCS#10 CSR using the device key.
  • Submit CSR with attestation chain and nonce to enrollment endpoint.
  • Generate hardware-backed key with attestation challenge support.
  • Receive and store DIC certificate after enrollment.
  • Provide security info/logging hooks for diagnostics (mTLS security info utility present).

Non-compliant requirements:

  • Auto-presenting DIC for mTLS to admin dashboard (not visible/verified in this diff).
  • Full server-side attestation verification and issuance policy (out of scope of client PR).
  • Short-lived AGC issuance and IdP proxy flow (not implemented in this PR).

Requires further human verification:

  • Confirm CSR/nonce formats exactly match backend API contract.
  • Validate that server verifies Android Key Attestation (package/signing cert, boot state) and binds DIC correctly.
  • End-to-end mTLS to admin with issued DIC on real devices (TEE/StrongBox).
  • Privacy/security review of device identifier generation and logging.
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
Hardcoded bearer token placeholder and verbose logging

  • Hardcoded BEARER_TOKEN and URL constants in WootzDeviceEnrollment can lead to accidental credential leakage. These should be provisioned securely (e.g., via policy/config, not stored in code).
  • Logging full JSON payloads including CSR and attestation chain may expose sensitive device metadata. Reduce log verbosity or redact sensitive fields.
  • Verify CSR extension handling does not inadvertently mark EKU as critical if backend/tools assume non-critical; mismatches can cause interoperability issues.
  • Ensure the CSR signing pathway cannot be abused to sign arbitrary data; restrict usage to CSR TBS only and validate input sizes.
⚡ Recommended focus areas for review

Possible Issue

CSR signing uses raw TBS with SHA256withECDSA on Java side, but the DER signature is attached directly without ensuring the algorithm and signature format alignment; also uses OpenSSL APIs that may not exist in BoringSSL (e.g., X509_REQ_set1_signature_algo/value). Verify availability and that the signature bytes are in the expected DER format for X509_REQ.

X509_ALGOR* sig_alg = X509_ALGOR_new();
if (!sig_alg) {
  LOG(ERROR) << "Failed to create signature algorithm";
  return false;
}

// Set the algorithm OID for ecdsa-with-SHA256
if (!X509_ALGOR_set0(sig_alg, OBJ_nid2obj(NID_ecdsa_with_SHA256), V_ASN1_NULL, nullptr)) {
  LOG(ERROR) << "Failed to set signature algorithm OID";
  X509_ALGOR_free(sig_alg);
  return false;
}

// Set signature algorithm on CSR using BoringSSL function
if (!X509_REQ_set1_signature_algo(req, sig_alg)) {
  LOG(ERROR) << "Failed to set signature algorithm on CSR";
  X509_ALGOR_free(sig_alg);
  return false;
}

X509_ALGOR_free(sig_alg);  // CSR takes a copy, so we can free our reference

// Set signature value on CSR using BoringSSL function
if (!X509_REQ_set1_signature_value(req, signature.data(), signature.size())) {
  LOG(ERROR) << "Failed to set signature value on CSR";
  return false;
}
Hardcoded Config

Placeholder constants for NONCE_URL, ENROLLMENT_URL, and BEARER_TOKEN are hardcoded. This risks leaking secrets and lacks environment separation. Replace with secure config/injection and avoid embedding bearer tokens.

private static final String NONCE_URL = "PROD_NONCE_URL";
private static final String ENROLLMENT_URL = "PROD_ENROLLMENT_URL";
private static final String BEARER_TOKEN = "PROD_BEARER_TOKEN";
Privacy Concern

Device identifier uses hardware info plus currentTimeMillis and radio version; this is unstable and may leak sensitive info. It also changes over time, defeating stable identity. Consider using attested key’s public key hash or a server-assigned device_id.

private static String generateDeviceIdentifier() {
    // Create device ID based on hardware characteristics
    String manufacturer = android.os.Build.MANUFACTURER;
    String model = android.os.Build.MODEL;
    String serial = android.os.Build.getRadioVersion(); // More stable than SERIAL

    // Create a hash-based identifier to ensure uniqueness and privacy
    String deviceInfo = manufacturer + "-" + model + "-" + serial + "-" + System.currentTimeMillis();
    return "wootz-device-" + Math.abs(deviceInfo.hashCode());

@qodo-code-review

qodo-code-review Bot commented Sep 22, 2025 •

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Hardcoded secrets and URLs exist

The code contains hardcoded secrets and API endpoints, posing a security risk
and reducing flexibility. These values should be externalized and loaded from a
secure configuration source instead of being committed to the repository.

Examples:

src/net/android/java/src/org/chromium/net/WootzDeviceEnrollment.java [32-34]
    private static final String NONCE_URL = "PROD_NONCE_URL";
    private static final String ENROLLMENT_URL = "PROD_ENROLLMENT_URL";
    private static final String BEARER_TOKEN = "PROD_BEARER_TOKEN";

Solution Walkthrough:

Before:

// file: src/net/android/java/src/org/chromium/net/WootzDeviceEnrollment.java
public class WootzDeviceEnrollment {
    private static final String NONCE_URL = "PROD_NONCE_URL";
    private static final String ENROLLMENT_URL = "PROD_ENROLLMENT_URL";
    private static final String BEARER_TOKEN = "PROD_BEARER_TOKEN";

    private static String requestNonce() throws IOException {
        URL url = new URL(NONCE_URL);
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
        connection.setRequestProperty("Authorization", "Bearer " + BEARER_TOKEN);
        // ...
    }
    // ...
}

After:

// file: src/net/android/java/src/org/chromium/net/WootzDeviceEnrollment.java
public class WootzDeviceEnrollment {
    // Values are loaded from a secure configuration provider, not hardcoded.
    private static final String NONCE_URL = AppConfig.getEnrollmentNonceUrl();
    private static final String ENROLLMENT_URL = AppConfig.getEnrollmentUrl();
    private static final String BEARER_TOKEN = AppConfig.getEnrollmentApiToken();

    private static String requestNonce() throws IOException {
        URL url = new URL(NONCE_URL);
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
        connection.setRequestProperty("Authorization", "Bearer " + BEARER_TOKEN);
        // ...
    }
    // ...
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical security risk and poor design practice by pointing out the hardcoded BEARER_TOKEN and endpoint URLs, which should be externalized from the source code.

High
Possible issue
Make device identifier generation deterministic

Remove System.currentTimeMillis() from the deviceInfo string in
generateDeviceIdentifier to ensure the generated identifier is deterministic and
stable across application restarts.

src/net/android/java/src/org/chromium/net/WootzHardwareKeyStore.java [271-280]

 private static String generateDeviceIdentifier() {
     // Create device ID based on hardware characteristics
     String manufacturer = android.os.Build.MANUFACTURER;
     String model = android.os.Build.MODEL;
     String serial = android.os.Build.getRadioVersion(); // More stable than SERIAL
     
     // Create a hash-based identifier to ensure uniqueness and privacy
-    String deviceInfo = manufacturer + "-" + model + "-" + serial + "-" + System.currentTimeMillis();
+    String deviceInfo = manufacturer + "-" + model + "-" + serial;
     return "wootz-device-" + Math.abs(deviceInfo.hashCode());
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical logic flaw in the new generateDeviceIdentifier method. Using System.currentTimeMillis() makes the device ID non-deterministic, which would likely break the device enrollment and identification process.

High
Improve PEM chain parsing logic

Replace the fragile String.split logic in convertPemChainToArray with a more
robust regular expression to correctly parse concatenated PEM certificate
chains.

src/net/android/java/src/org/chromium/net/WootzEnrollmentUtils.java [257-282]

 public static String[] convertPemChainToArray(String pemChain) {
     if (pemChain == null || pemChain.trim().isEmpty()) {
         return new String[0];
     }
     
-    // Split by certificate boundaries
-    String[] certificates = pemChain.split("-----END CERTIFICATE-----");
     java.util.List<String> certList = new java.util.ArrayList<>();
+    java.util.regex.Pattern pattern = java.util.regex.Pattern.compile(
+            "-----BEGIN CERTIFICATE-----[^-]*-----END CERTIFICATE-----", 
+            java.util.regex.Pattern.DOTALL);
+    java.util.regex.Matcher matcher = pattern.matcher(pemChain);
     
-    for (String cert : certificates) {
-        String trimmedCert = cert.trim();
-        if (!trimmedCert.isEmpty()) {
-            // Add back the END boundary and ensure proper formatting
-            if (!trimmedCert.startsWith("-----BEGIN CERTIFICATE-----")) {
-                // Find and preserve the BEGIN boundary if it exists
-                int beginIndex = trimmedCert.indexOf("-----BEGIN CERTIFICATE-----");
-                if (beginIndex >= 0) {
-                    trimmedCert = trimmedCert.substring(beginIndex);
-                }
-            }
-            certList.add(trimmedCert + "-----END CERTIFICATE-----");
-        }
+    while (matcher.find()) {
+        certList.add(matcher.group());
     }
     
     return certList.toArray(new String[0]);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the string splitting logic is fragile and proposes a more robust regex-based solution, which improves the reliability of parsing PEM certificate chains.

Low
  • Update

@pandey019
pandey019 merged commit 4880e22 into wootzapp:chromium Sep 23, 2025
1 check passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants