[Feature Request] Encrypt token cache in shared preferences on Xamarin.Android when the OS enables it

[jmprieur]

Is your feature request related to a problem? Please describe. Enable encryption at rest for the token cache to make more difficult for an adversary to use the data stored in the cache for malicious purposes.

Note: MSAL.Android native encrypts the token cache when possible on Android devices.

Notes on the security aspect:

• Adding encryption at rest is categorized as Defense in depth.
• Shared preferences on Android are meant to be only visible to the application (or application group) that creates them, however in practice on rooted devices, these values are easy to read.

[bgavrilMS]

There are 2 aspects to consider here:

1. What encyption logic to use - MSAL.Android's (about 1000 lines of Java code) or Xamarin.Essentials (about 300 lines of C# code) ? The advantage of Xamarin.Essentials logic is that it's already written in C# and there are tests for it. I assume both have passed crypto board scrutiny as they are pretty low level.

2. Migration from the old location to the new location. I think the simplest will be to do smth like:

• when PCA is created (not on every Acquire* call), look in the old storage and if data is found, migrate it to the new storage and delete the old storage.

[henrik-me]

+1 to be inspired from Xamarin.Essentials and for the migration proposal.

[WilliamWatterson86]

@bgavrilMS We recently had an external company perform a pen test on our apps and this came up. So hopefully the changes planned for v4.9 will help with this one.

Here was the result of the test:

Description: The device stores sensitive information without making use of encryption in the shared preferences. In this instance, this sensitive information included the refresh token used to generate a new authorisation bearer header on the onmicrosoft.com domain.

An attacker who had access to the shared preferences file, could potentially create a new authorisation bearer header and use it to authenticate and interact with the back-end API as the user of the mobile application.

Solution: Storage of sensitive information should be avoided or reduced to the minimum required by the application functionality. Shared area of the filesystem cannot be used to store this information and encryption must be used even when information are stored in private area of the filesytem.

[jmprieur]

Discussing the possibility of having an opt-in flag and leave the migration aspect in 4.? Or let developer specify their own persistence (if they have super stick security requirements).

[stevehansen]

Any updates on this, we also got this from our pentest that the tokens were stored in plain text, the documentation mentions that it should have been using "secure storage", we tried to use the TokenCache hook but that just crashed our application:

System.PlatformNotSupportedException: 'You should not use these TokenCache methods on mobile platforms. They are meant to allow applications to define their own storage strategy on .net desktop and non-mobile platforms such as .net core. On mobile platforms, MSAL.NET implements a secure and performant storage mechanism. For more details about custom token cache serialization, visit https://aka.ms/msal-net-serialization'

Running on MAUI for .NET 7

[bgavrilMS]

@localden @moumighosh - thoughts about this? MSAL Android does a better job here, but changing the default in MSAL .NET is complicated, as we'd need to migrate the cache.

@stevehansen - shared preferences are safe as long as the phone isn't hacked / rootkitted.

[localden]

I am trying to understand the attack vector here - if the phone is locked and the device is encrypted, key exfiltration should not be possible, no?

Assuming that the vector here is more along the lines of "malicious app installed on the device tries to exfiltrate tokens from the filesystem cache," I like the suggested approach of "feature flagging" this change, enabling customers to opt-in for a more secure storage of tokens (that will auto-migrate the cache to the new format). Could we potentially add something like .WithEncryptedTokenCache(KeyToEncrypt) to the PCA builder for this?

[stevehansen]

@localden that was mostly the idea that I tried, following the MSDN documentation about the ITokenCache and had code like this:

internal static class TokenCacheHelper 
{ 
    private const string CacheKey = nameof(CacheKey); 

    public static void EnableSerialization(ITokenCache tokenCache) 
    { 
        tokenCache.SetBeforeAccess(BeforeAccessNotification); 
        tokenCache.SetAfterAccess(AfterAccessNotification); 
    } 

    private static readonly string CacheFilePath = Path.Combine(FileSystem.CacheDirectory, "msalcache.bin3"); 
    private static readonly object FileLock = new(); 

    private static void BeforeAccessNotification(TokenCacheNotificationArgs args) 
    { 
        lock (FileLock) 
        { 
            args.TokenCache.DeserializeMsalV3(File.Exists(CacheFilePath) 
                ? GetCrypto().Decrypt(File.ReadAllText(CacheFilePath)) 
                : null); 
        } 
    } 

    private static void AfterAccessNotification(TokenCacheNotificationArgs args) 
    { 
        // if the access operation resulted in a cache update 
        if (args.HasStateChanged) 
        { 
            lock (FileLock) 
            { 
                // reflect changes in the persistent store 
                File.WriteAllText(CacheFilePath, GetCrypto().Encrypt(args.TokenCache.SerializeMsalV3())); 
            } 
        } 
    } 

    private static Crypto GetCrypto() 
    { 
        byte[] password; 

        var storeKey = SecureStorage.GetAsync(CacheKey).GetAwaiter().GetResult(); 
        if (string.IsNullOrEmpty(storeKey)) 
        { 
            password = RandomNumberGenerator.GetBytes(32); 
            SecureStorage.SetAsync(CacheKey, Convert.ToBase64String(password)).Wait(); 
        } 
        else 
            password = Convert.FromBase64String(storeKey); 

        return new(password); 
    } 
} 

Where our Crypto class is what we used in the rest of the app to encrypt data. But you can't use the hook at the moment on mobile platforms: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/src/client/Microsoft.Identity.Client/TokenCache.Notifications.cs#L183

[bgavrilMS]

This is correct, we actively prevent folks from using token cache extensibility on mobile, in the idea that MSAL just does it for you.

I agree that MSAL could do a better job, the problem is that moving to a different cache mechanism involves some sort of cache migration to read from old unencrypted location.

[munkii]

Has there been any progress on this request? An external penetration testing company highlighted this issue for us too. I can email the specific findings to a team member if needed.

[bgavrilMS]

Duplicate of https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/1361