Skip to content

Fix for SDL violation in device pop scenarios #2744

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Fix for SDL violation in device pop scenarios #2744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
66a3fc9
initial fix for sdl violation in device pop scenarios
somalaya Aug 25, 2025
16c21b1
added telemetry to track mint signed access token success
somalaya Aug 25, 2025
2ffa9e8
skip encryption padding when no encryption purpose passed
somalaya Aug 28, 2025
895331e
addressed comments
somalaya Sep 12, 2025
8da2828
added param
somalaya Sep 15, 2025
d64073d
refactored API 23+ and 28+ methods
somalaya Oct 30, 2025
76549a8
UT added
somalaya Oct 30, 2025
9691151
Merge branch 'dev' into somalaya/devicePopSdlFix
somalaya Oct 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions ...m/microsoft/identity/common/internal/platform/AndroidDevicePoPManagerEncryptionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,27 @@

import android.os.Build;

import androidx.annotation.NonNull;
import androidx.test.core.app.ApplicationProvider;

import com.microsoft.identity.common.java.crypto.IDevicePopManager;
import com.microsoft.identity.common.java.exception.ClientException;
import com.microsoft.identity.common.java.flighting.CommonFlight;
import com.microsoft.identity.common.java.flighting.CommonFlightsManager;
import com.microsoft.identity.common.java.flighting.IFlightsManager;
import com.microsoft.identity.common.java.flighting.IFlightsProvider;

import org.jetbrains.annotations.NotNull;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.mockito.Mockito;

import java.io.IOException;
import java.security.InvalidKeyException;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.CertificateException;
Expand Down Expand Up @@ -92,6 +100,7 @@ public void setUp() throws ClientException {
@After
public void tearDown() {
devicePopManager.clearAsymmetricKey();
CommonFlightsManager.INSTANCE.resetFlightsManager();
}

@Test
Expand All @@ -102,4 +111,46 @@ public void testEncryption() throws ClientException {
Assert.assertEquals(DATA_TO_ENCRYPT, decryptedValue);
Assert.assertNotEquals(DATA_TO_ENCRYPT, cipherText);
}

@Test
public void testEncryption_Disabled() throws ClientException, CertificateException, KeyStoreException, NoSuchAlgorithmException, IOException {
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
Mockito.when(mockFlightsProvider.isFlightEnabled(CommonFlight.DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER))
.thenReturn(true);
// Create anonymous IFlightsManager
IFlightsManager anonymousFlightsManager = new IFlightsManager() {
@Override
public @NotNull IFlightsProvider getFlightsProvider(long waitForConfigsWithTimeoutInMs) {
return mockFlightsProvider;
}
@Override
public @NotNull IFlightsProvider getFlightsProviderForTenant(@NotNull String tenantId, long waitForConfigsWithTimeoutInMs) {
return mockFlightsProvider;
}
@Override
public @NotNull IFlightsProvider getFlightsProviderForTenant(@NotNull String tenantId) {
return mockFlightsProvider;
}
@NonNull
@Override
public IFlightsProvider getFlightsProvider() {
return mockFlightsProvider;
}
};

// Initialize CommonFlightsManager with the anonymous implementation
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(anonymousFlightsManager);
IDevicePopManager devicePopManager = new AndroidDevicePopManager(ApplicationProvider.getApplicationContext());
devicePopManager.generateAsymmetricKey();
try {
final String cipherText = devicePopManager.encrypt(cipher, DATA_TO_ENCRYPT);
devicePopManager.decrypt(cipher, cipherText);
} catch (Exception exception) {
Assert.assertTrue(exception instanceof ClientException);
Assert.assertTrue(exception.getCause().getCause().getMessage().contains("Incompatible purpose"));
return;
}
Assert.fail();

}
}
124 changes: 76 additions & 48 deletions ...rc/main/java/com/microsoft/identity/common/internal/platform/AndroidDevicePopManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

import com.microsoft.identity.common.java.crypto.IKeyStoreKeyManager;
import com.microsoft.identity.common.java.crypto.SecureHardwareState;
import com.microsoft.identity.common.java.flighting.CommonFlight;
import com.microsoft.identity.common.java.flighting.CommonFlightsManager;
import com.microsoft.identity.common.java.platform.AbstractDevicePopManager;
import com.microsoft.identity.common.logging.Logger;
import com.nimbusds.jose.crypto.impl.RSAKeyUtils;
Expand Down Expand Up @@ -348,18 +350,30 @@ private KeyPairGenerator getInitializedRsaKeyPairGenerator(@androidx.annotation.
* @param trySetAttestationChallenge True if we should attempt to generate an attestation challenge cert.
* @throws InvalidAlgorithmParameterException
*/
private void initialize(@androidx.annotation.NonNull final Context context,
@androidx.annotation.NonNull final KeyPairGenerator keyPairGenerator,
private void initialize(@NonNull final Context context,
@NonNull final KeyPairGenerator keyPairGenerator,
final int keySize,
final boolean useStrongbox,
final boolean enableImport,
final boolean trySetAttestationChallenge) throws InvalidAlgorithmParameterException {
final boolean unnecessaryCryptoPurposesDisabled =
CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER);
Copy link
Contributor

@p3dr0rv p3dr0rv Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ask copilot for a better name :p


int purposes = KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_VERIFY;
Copy link
Contributor

@p3dr0rv p3dr0rv Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Consider moving the definition of purposes inside each initializeX method — I believe it makes things a little easier to read.

if (!unnecessaryCryptoPurposesDisabled) {
// If the flight is not enabled, we can use the key for encryption and decryption.
purposes |= KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT;
}

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
initializePre23(context, keyPairGenerator, keySize);
} else if (Build.VERSION.SDK_INT < Build.VERSION_CODES.P) {
initialize23(keyPairGenerator, keySize, useStrongbox, trySetAttestationChallenge);
initialize23(keyPairGenerator, keySize, useStrongbox, trySetAttestationChallenge, purposes, unnecessaryCryptoPurposesDisabled);
} else {
initialize28(keyPairGenerator, keySize, useStrongbox, enableImport, trySetAttestationChallenge);
if (!unnecessaryCryptoPurposesDisabled && enableImport) {
purposes |= KeyProperties.PURPOSE_WRAP_KEY;
}
initialize28(keyPairGenerator, keySize, useStrongbox, trySetAttestationChallenge, purposes, unnecessaryCryptoPurposesDisabled);
}
}

Expand All @@ -368,28 +382,12 @@ private void initialize(@androidx.annotation.NonNull final Context context,
private void initialize23(@androidx.annotation.NonNull final KeyPairGenerator keyPairGenerator,
final int keySize,
final boolean useStrongbox,
final boolean trySetAttestationChallenge) throws InvalidAlgorithmParameterException {
KeyGenParameterSpec.Builder builder;
builder = new KeyGenParameterSpec.Builder(
mKeyManager.getKeyAlias(),
KeyProperties.PURPOSE_SIGN
| KeyProperties.PURPOSE_VERIFY
| KeyProperties.PURPOSE_ENCRYPT
| KeyProperties.PURPOSE_DECRYPT
)
.setKeySize(keySize)
.setSignaturePaddings(
KeyProperties.SIGNATURE_PADDING_RSA_PKCS1
)
.setDigests(
KeyProperties.DIGEST_NONE,
KeyProperties.DIGEST_SHA1,
KeyProperties.DIGEST_SHA256
).setEncryptionPaddings(
KeyProperties.ENCRYPTION_PADDING_RSA_OAEP,
KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1
);
final boolean trySetAttestationChallenge,
final int purposes,
final boolean unnecessaryCryptoPurposesDisabled) throws InvalidAlgorithmParameterException {
KeyGenParameterSpec.Builder builder = buildCommonKeyGenSpec(keySize, purposes, unnecessaryCryptoPurposesDisabled);

// API 23-specific conditions: attestation requires API 24+, StrongBox requires API 28+
if (trySetAttestationChallenge && Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
builder = setAttestationChallenge(builder);
}
Expand Down Expand Up @@ -444,29 +442,10 @@ private static KeyGenParameterSpec.Builder applyHardwareIsolation(
private void initialize28(@androidx.annotation.NonNull final KeyPairGenerator keyPairGenerator,
final int keySize,
final boolean useStrongbox,
final boolean enableImport,
final boolean trySetAttestationChallenge) throws InvalidAlgorithmParameterException {
int purposes = KeyProperties.PURPOSE_SIGN
| KeyProperties.PURPOSE_VERIFY
| KeyProperties.PURPOSE_ENCRYPT
| KeyProperties.PURPOSE_DECRYPT;
if (enableImport) {
purposes |= KeyProperties.PURPOSE_WRAP_KEY;
}
KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder(
mKeyManager.getKeyAlias(), purposes)
.setKeySize(keySize)
.setSignaturePaddings(
KeyProperties.SIGNATURE_PADDING_RSA_PKCS1
)
.setDigests(
KeyProperties.DIGEST_NONE,
KeyProperties.DIGEST_SHA1,
KeyProperties.DIGEST_SHA256
).setEncryptionPaddings(
KeyProperties.ENCRYPTION_PADDING_RSA_OAEP,
KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1
);
final boolean trySetAttestationChallenge,
final int purposes,
final boolean unnecessaryCryptoPurposesDisabled) throws InvalidAlgorithmParameterException {
KeyGenParameterSpec.Builder builder = buildCommonKeyGenSpec(keySize, purposes, unnecessaryCryptoPurposesDisabled);

if (trySetAttestationChallenge) {
builder = setAttestationChallenge(builder);
Expand All @@ -484,6 +463,55 @@ private void initialize28(@androidx.annotation.NonNull final KeyPairGenerator ke
keyPairGenerator.initialize(spec);
}

/**
* Sets encryption paddings on the KeyGenParameterSpec.Builder if encryption purposes are enabled.
*
* @param builder The KeyGenParameterSpec.Builder to configure
* @param unnecessaryCryptoPurposesDisabled When true, encryption paddings are not set to address
* SDL security requirements by limiting key usage to signing operations only
*/
private void setEncryptionPaddingsIfNeeded(@NonNull final KeyGenParameterSpec.Builder builder,
final boolean unnecessaryCryptoPurposesDisabled) {
final String methodTag = TAG + ":setEncryptionPaddingsIfNeeded";
if (!unnecessaryCryptoPurposesDisabled) {
Logger.verbose(methodTag, "Adding encryption paddings (RSA_OAEP, RSA_PKCS1) to key specification");
builder.setEncryptionPaddings(
KeyProperties.ENCRYPTION_PADDING_RSA_OAEP,
KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1
);
} else {
Logger.verbose(methodTag, "Skipping encryption paddings due to SDL security restrictions");
}
}

/**
* Builds the common KeyGenParameterSpec configuration shared between API 23+ and 28+ initialization methods.
* Setting core configuration : Key alias, purposes, size, signature paddings, digests, and encryption paddings.
*
* @param keySize The RSA key size
* @param purposes The key purposes (signing, encryption, etc.)
* @param unnecessaryCryptoPurposesDisabled Whether to skip encryption paddings for SDL compliance
* @return Configured KeyGenParameterSpec.Builder ready for API-specific configuration
*/
private KeyGenParameterSpec.Builder buildCommonKeyGenSpec(final int keySize,
final int purposes,
final boolean unnecessaryCryptoPurposesDisabled) {
KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder(
mKeyManager.getKeyAlias(), purposes)
.setKeySize(keySize)
.setSignaturePaddings(
KeyProperties.SIGNATURE_PADDING_RSA_PKCS1
)
.setDigests(
KeyProperties.DIGEST_NONE,
KeyProperties.DIGEST_SHA1,
KeyProperties.DIGEST_SHA256
);

setEncryptionPaddingsIfNeeded(builder, unnecessaryCryptoPurposesDisabled);

return builder;
}

@SuppressLint(NewApi)
@SuppressWarnings("deprecation")
Expand Down
4 changes: 4 additions & 0 deletions common4j/src/main/com/microsoft/identity/common/java/crypto/IDevicePopManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ public AlgorithmParameterSpec getParameters() {
* @return The encrypted plaintext.
* @throws ClientException If encryption fails.
*/
@Deprecated
String encrypt(Cipher cipher, String plaintext) throws ClientException;

/**
Expand All @@ -275,6 +276,7 @@ public AlgorithmParameterSpec getParameters() {
* @return The encrypted plaintext.
* @throws ClientException If encryption fails.
*/
@Deprecated
byte[] encrypt(@NonNull final Cipher cipher, @NonNull final byte[] plaintext) throws ClientException;

/**
Expand All @@ -285,6 +287,7 @@ public AlgorithmParameterSpec getParameters() {
* @return The decrypted text.
* @throws ClientException If decryption fails.
*/
@Deprecated
String decrypt(Cipher cipher, String ciphertext) throws ClientException;

/**
Expand All @@ -295,6 +298,7 @@ public AlgorithmParameterSpec getParameters() {
* @return The decrypted text.
* @throws ClientException If decryption fails.
*/
@Deprecated
byte[] decrypt(@NonNull Cipher cipher, byte[] ciphertext) throws ClientException;

/**
Expand Down
7 changes: 6 additions & 1 deletion common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,12 @@ public enum CommonFlight implements IFlightConfig {
* Flight to enable OpenID issuer validation code which validates issuer against the open id well known
* config endpoint and only reports the failure result.
*/
ENABLE_OPENID_ISSUER_VALIDATION_REPORTING("EnableOpenIdIssuerValidationReporting", true);
ENABLE_OPENID_ISSUER_VALIDATION_REPORTING("EnableOpenIdIssuerValidationReporting", true),

/**
* Flight to disable the unnecessary crypto operation purposes in device pop manager like encrypt, decrypt and wrap.
*/
DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER ("DisableUnnecessaryCryptoPurposes", false);

private String key;
private Object defaultValue;
Expand Down
3 changes: 2 additions & 1 deletion common4j/src/main/com/microsoft/identity/common/java/opentelemetry/SpanName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,6 @@ public enum SpanName {
ProcessWebCpEnrollmentRedirect,
ProcessWebCpAuthorizeUrlRedirect,
PersistToStorageAsync,
InstallCertOnWpj
InstallCertOnWpj,
DevicePopMintSignedAccessToken
}
33 changes: 25 additions & 8 deletions common4j/src/main/com/microsoft/identity/common/java/platform/AbstractDevicePopManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@
import com.microsoft.identity.common.java.exception.ClientException;
import com.microsoft.identity.common.java.logging.Logger;
import com.microsoft.identity.common.java.marker.CodeMarkerManager;
import com.microsoft.identity.common.java.opentelemetry.OTelUtility;
import com.microsoft.identity.common.java.opentelemetry.SpanExtension;
import com.microsoft.identity.common.java.opentelemetry.SpanName;
import com.microsoft.identity.common.java.util.StringUtil;
import com.microsoft.identity.common.java.util.TaskCompletedCallbackWithError;
import com.nimbusds.jose.JOSEException;
Expand Down Expand Up @@ -103,6 +106,9 @@
import javax.crypto.NoSuchPaddingException;

import edu.umd.cs.findbugs.annotations.Nullable;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.context.Scope;
import lombok.NonNull;

public abstract class AbstractDevicePopManager implements IDevicePopManager {
Expand Down Expand Up @@ -877,14 +883,25 @@ public String mintSignedAccessToken(@Nullable String httpMethod,
@NonNull final String accessToken,
@Nullable final String nonce,
@Nullable final String clientClaims) throws ClientException {
return mintSignedHttpRequestInternal(
httpMethod,
timestamp,
requestUrl,
accessToken,
nonce,
clientClaims
);
final Span span = OTelUtility.createSpan(SpanName.DevicePopMintSignedAccessToken.name());
try (final Scope scope = SpanExtension.makeCurrentSpan(span)) {
final String signedAccessToken = mintSignedHttpRequestInternal(
httpMethod,
timestamp,
requestUrl,
accessToken,
nonce,
clientClaims
);
span.setStatus(StatusCode.OK);
return signedAccessToken;
} catch (final Exception exception) {
span.recordException(exception);
span.setStatus(StatusCode.ERROR);
throw exception;
} finally {
span.end();
}
}

@Override
Expand Down
Loading