Skip to content
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.

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

GetAccountsAsync() can crash on UWP #1064

Closed
1 of 7 tasks
tipa opened this issue Apr 11, 2019 · 26 comments
Closed
1 of 7 tasks

GetAccountsAsync() can crash on UWP #1064

tipa opened this issue Apr 11, 2019 · 26 comments
Assignees
Labels
Milestone

Comments

@tipa
Copy link

tipa commented Apr 11, 2019

Which Version of MSAL are you using ?
MSAL 3.0.3-preview

Platform
UWP

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Auth
    • Username Password
    • Device code flow (browserless)
  • Web App
    • Authorization code
    • OBO
  • Web API
    • OBO

Other? - please describe;

Is this a new or existing app?
a. The app is in production, and I have upgraded to a new version of MSAL

Repro

var clientApp = PublicClientApplicationBuilder.Create("xyz").Build();
var user = (await clientApp.GetAccountsAsync()).FirstOrDefault();

Expected behavior
No crash

Actual behavior
App crashes (see stacktrace below) - but very rarely. I only got one report about this crash from my users so far.

Additional context/ Logs / Screenshots
System.Exception: ASN1 bad tag value met. (Exception from HRESULT: 0x8009310B) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x5c at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x44 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x1c at Microsoft.Identity.Client.Platforms.uap.UapCryptographyManager.Decrypt(Byte[]) + 0x86 at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccessAsync(TokenCacheNotificationArgs) + 0x13d at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccess(TokenCacheNotificationArgs) + 0x8 at Microsoft.Identity.Client.TokenCache.OnBeforeAccess(TokenCacheNotificationArgs) + 0x36 at Microsoft.Identity.Client.TokenCache.FetchAllAccountItemsFromCache(IEnumerable1
`

@jmprieur jmprieur added this to the 3.0.5 milestone Apr 11, 2019
@MarkZuber
Copy link
Contributor

This looks like a possible corrupted cache at desynchronization. The actual decrypt is failing. Recommendation would be to delete/clear the cache for this user.

@tipa
Copy link
Author

tipa commented Apr 11, 2019

I recommended reinstalling as my method to clear the credentials with the method below would crash as well.

public static async Task ClearCachedAuthTokenAsync()
{
        var clientApp = GetClientApplication();
        var user = (await clientApp.GetAccountsAsync()).FirstOrDefault();
        if (user != null) { try { await clientApp.RemoveAsync(user); } catch (MsalException) { } }
}

I think this should be handled by the library

@bgavrilMS
Copy link
Member

On UAP we store the data in a file that we encrypt with DPApi on behalf of the user. Could it be that the app is used by other users? The file location is ApplicationData.Current.LocalFolder and the name is "msalcache.dat".

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/dev3x/src/Microsoft.Identity.Client/Platforms/uap/UapTokenCacheBlobStorage.cs#L72

@tipa
Copy link
Author

tipa commented Apr 12, 2019

I believe the ApplicationData folder is not shared among Windows users as it is a subfolder of %localappdata%

@tipa
Copy link
Author

tipa commented Apr 16, 2019

I now got some more reports from other users experiencing the same issue. This issue hasn't been present a few updates ago, I would highly appreciate if it could be resolved.

@MarkZuber MarkZuber modified the milestones: 3.0.5, 3.0.6 Apr 16, 2019
@MarkZuber
Copy link
Contributor

@tipa Can you comment on your users' network environment? Are they domain joined? Any custom certificate / PKI infrastructure possibly? A search for "ASN1 bad tag value met dataprotectionprovider" where this type of error occurs all points to something wrong with certificate management / export / requests and I want to ensure this isn't an infrastructure issue with your environment where something is wrong with the certificates in general. If there is, then deleting the data file won't necessarily fix this issue since further encrypt/decrypt operations may also fail.

@tipa
Copy link
Author

tipa commented Apr 16, 2019

Unfortunately I cannot comment on my user's network environment. I would assume they are in a "regular" private wifi - no special infrastructure.
Actually, the error log I received today contained a different exception, but in the same method:

System.Exception: Die angegebenen Daten konnten nicht entschlüsselt werden. (Exception from HRESULT: 0x8009002C) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x5c at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x44 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x1c at Microsoft.Identity.Client.Platforms.uap.UapCryptographyManager.Decrypt(Byte[]) + 0x86 at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccessAsync(TokenCacheNotificationArgs) + 0x13d at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccess(TokenCacheNotificationArgs) + 0x8 at Microsoft.Identity.Client.TokenCache.OnBeforeAccess(TokenCacheNotificationArgs) + 0x36 at Microsoft.Identity.Client.TokenCache.FetchAllAccountItemsFromCache(IEnumerable1`

@MarkZuber
Copy link
Contributor

Do you know what version of Windows 10 they are running on? Is that consistent across the users with your error?

@MarkZuber
Copy link
Contributor

Also, are your users setup for roaming of data, onedrive sync or other technologies that might cause this file to move to another device? DPAPI encryption is only for a single device and if these files roam to any other machines this error will occur.

@tipa
Copy link
Author

tipa commented Apr 16, 2019

HRESULT: 0x8009310B happened on 10.0.17763.404, HRESULT: 0x8009002C on 10.0.17763.437.

How can they check if they have set up roaming of data?

Also, I had this error reported 2 weeks ago (on 10.0.18362.1):
System.IO.FileNotFoundException: 系统找不到指定的文件。 (Exception from HRESULT: 0x80070002) at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x5c at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x44 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x1c at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccessAsync(TokenCacheNotificationArgs) + 0x109 at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccess(TokenCacheNotificationArgs) + 0x8 at Microsoft.Identity.Client.TokenCache.OnBeforeAccess(TokenCacheNotificationArgs) + 0x36 at Microsoft.Identity.Client.TokenCache.Microsoft.Identity.Client.ITokenCacheInternal.GetAccounts(String) + 0x157 at Microsoft.Identity.Client.ClientApplicationBase.GetAccountsAsync()

@tipa
Copy link
Author

tipa commented Apr 17, 2019

Wouldn't it be possible to save the tokens similar to Xamarin.Essentials SecureStorage? I haven't had any issues with that so far...
https://docs.microsoft.com/en-us/xamarin/essentials/secure-storage?tabs=uwp

@henrik-me henrik-me modified the milestones: 3.0.6, 3.0.7 Apr 17, 2019
@MarkZuber MarkZuber modified the milestones: 3.0.8, 3.0.9 Apr 29, 2019
@MarkZuber
Copy link
Contributor

No, Xamarin SecureStorage is not appropriate for the cache. From the documentation:

Limitations
This API is intended to store small amounts of text. Performance may be slow if you try to use it to store large amounts of text.

The token cache contents can be larger and don't fit into this type of simple key/value model which is more appropriate for storing something like a client key.

Given that the actual decryption of the contents is failing, this really sounds like an environmental issue that the files are moving between devices.

The library does not and won't expose an "obliterate cache file from disk" method because in nearly all other cases this is a very dangerous operation that expands beyond the scope of any particular app because the cache file could be storing tokens for multiple apps in certain scenarios (for single-sign-on).

Can you provide additional environmental information (e.g. win10 version, domain joined or other network scenarios, if domain joined is there any group policy or roaming or backup/restore services that are installed on the devices)? Is this happening for all users or only a subset of users? If it's a subset can you identify any patterns in common between the affected users/devices (common software, etc)?

We don't control the encryption on the box in our library, it's a dependency. And if the keys for DPAPI encryption are failing that's beyond the MSAL library control and it's an environmental issue. We haven't seen any reports of this from any other UWP environments or our test environments. So any details you can provide to help pinpoint this are key.

@jmprieur jmprieur modified the milestones: 3.1, 4.0 May 29, 2019
@tipa
Copy link
Author

tipa commented Sep 2, 2019

This keeps happening on my users devices and I will try to get more information about their network situation. In the last case, the Windows 10 version was 10.0.17134.950

It would already help if the SDK could simply clear the token cache so that the app can ask the user again to login. Currently I need to ask my users to reinstall the app - certainly not the best solution

@bgavrilMS bgavrilMS reopened this Sep 2, 2019
@bgavrilMS
Copy link
Member

@tipa - is this with more recent version of MSAL? And the exception trace is the same?

@jmprieur @henrik-me - in UWP, MSAL stores the token cache in a file, encrypted with DPAPI. I propose that we add a small retry mechanism when accessing the cache on UWP. And if it fails, clear the cache as @tipa suggests. It's better to ask the user to login again rather than have them re-install the app.

@tipa
Copy link
Author

tipa commented Sep 2, 2019

Actually the exception (trace) is slightly different now (with MSAL 4.3.0):

System.Exception: The specified data could not be decrypted. (Exception from HRESULT: 0x8009002C)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x70
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x38
at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x17
at Microsoft.Identity.Client.Platforms.uap.UapCryptographyManager.Decrypt(Byte[]) + 0x86
at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccessAsync(TokenCacheNotificationArgs) + 0x13d
at Microsoft.Identity.Client.Platforms.uap.UapTokenCacheBlobStorage.OnBeforeAccess(TokenCacheNotificationArgs) + 0x8
at Microsoft.Identity.Client.TokenCache.d__93.MoveNext() + 0x6b
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x70
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x38
at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x17
at System.Runtime.CompilerServices.TaskAwaiter.GetResult() + 0xb
at Microsoft.Identity.Client.Cache.CacheSessionManager.d__16.MoveNext()

@jmprieur
Copy link
Contributor

jmprieur commented Sep 2, 2019

@bgavrilMS : maybe with an option. But can we try to understand why this happens first?

@bgavrilMS
Copy link
Member

@tipa - do you have a repro for the above? Or do you see a pattern for when the issue occurs? And how frequently this occurs?

We store the data in a file called msalcache.dat in LocalFolder

We use DPAPI to encrypt / decrypt the data. DPAPI performs these operations on behalf of the user. I've seen reports where windows updates have broken DPAPI, but generally there aren't many details.

I can start a conversation with the Windows ppl if we can narrow down the issue a bit.

@tipa
Copy link
Author

tipa commented Sep 2, 2019

Unfortunately I don't see a pattern or have an idea what causes this issue, but I pointed the user who reported this issue today to this thread, maybe he can give additional details about his network setup.
You can reproduce the crash manually by copying the msalcache.dat file from one PC to another. I haven't found another repro yet.

@bgavrilMS
Copy link
Member

Copying the file from one machine to another will most likely not work (for MSAL it is not a supported scenario anyway). This SO question touches on it, but generally documentation is sparse.

Note that for UWP we do allow developers to provide logic for serialization / deserialization (example here), so you can always define your own logic that adds retry mechanisms or simply deletes the cache file. That said, I'd really like to get this logic into MSAL, so that everyone benefits.

@tipa
Copy link
Author

tipa commented Sep 2, 2019

Copying the file from one machine to another will most likely not work (for MSAL it is not a supported scenario anyway). This SO question touches on it, but generally documentation is sparse.

I understand, I just mentioned it in case you had difficulties reproducing the exception

Note that for UWP we do allow developers to provide logic for serialization / deserialization (example here), so you can always define your own logic that adds retry mechanisms or simply deletes the cache file.

Right now I am catching the exception, checking for the HResult and deleting the file manually

That said, I'd really like to get this logic into MSAL, so that everyone benefits.

Thanks!

@tipa
Copy link
Author

tipa commented Sep 5, 2019

Can you provide additional environmental information (e.g. win10 version, domain joined or other network scenarios, if domain joined is there any group policy or roaming or backup/restore services that are installed on the devices)? Is this happening for all users or only a subset of users? If it's a subset can you identify any patterns in common between the affected users/devices (common software, etc)?

From one user I was able to gather these information:
OS version: 10.0.18362.295
PC joined to a domain
No group policies that are backing up / restoring user data
Only one user account on PC and that user account is only used on that single PC

@tipa
Copy link
Author

tipa commented Sep 15, 2019

Another user of my app just received the error System.Exception: ASN1 bad tag value met. (Exception from HRESULT: 0x8009310B)
OS version: 10.0.18362.356
Personal PC, not joined to a domain

@bgavrilMS
Copy link
Member

Is the error transient? Does re-opening the app help ?

The only reason I could think this is happening is that the token cache file is being copied over - but I think this is not the case? I'm trying to find someone with more knowledge of this API internally.

@tipa
Copy link
Author

tipa commented Sep 16, 2019

I can ask that the next user encountering this problem.
I highly doubt that these users manually changed the contents of the app folder or the msalcache file itself.

@jennyf19
Copy link
Collaborator

@tipa This is included in the 4.5.0 release.

@nbevans
Copy link

nbevans commented Feb 2, 2022

The root cause of the issue (not the hack which was merged) seems to be the way in which the DataProtectionProvider object is constructed. If you read the MSDN for it - it says to use the parameter-less ctor when decrypting. But use the 1 parameter ctor (where you provide the descriptor string that specifies ether LOCAL=user or LOCAL=machine) when you are encrypting.

image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants