[System.Security] Fix CryptographicException from ProtectedData.Protect with multiple threads

The code in ManagedProtection.GetKey() instantiated a new RSACryptoServiceProvider singleton without proper
locking to avoid multiple threads running the initialization concurrently.
Since RSACryptoServiceProvider (or rather KeyPairPersistence underneath) uses an XML file with a name derived
from the parameters to store the data there could be a case where one thread writes while another tries to read
and gets garbage. This results in a CryptographicException later on.

Added locking to prevent this.

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=38933

2 files changed, 41 insertions, 7 deletions

diff --git a/mcs/class/System.Security/Mono.Security.Cryptography/ManagedProtection.cs b/mcs/class/System.Security/Mono.Security.Cryptography/ManagedProtection.cs
index 4c0b7cb93f9..a32d88a3a59 100644
--- a/mcs/class/System.Security/Mono.Security.Cryptography/ManagedProtection.cs
+++ b/mcs/class/System.Security/Mono.Security.Cryptography/ManagedProtection.cs
@@ -237,23 +237,29 @@ namespace Mono.Security.Cryptography {
 
 		private static RSA user;
 		private static RSA machine;
+		private readonly static object user_lock = new object ();
+		private readonly static object machine_lock = new object ();
 
 		private static RSA GetKey (DataProtectionScope scope)
 		{
 			switch (scope) {
 			case DataProtectionScope.CurrentUser:
 				if (user == null) {
-					CspParameters csp = new CspParameters ();
-					csp.KeyContainerName = "DAPI";
-					user = new RSACryptoServiceProvider (1536, csp);
+					lock (user_lock) {
+						CspParameters csp = new CspParameters ();
+						csp.KeyContainerName = "DAPI";
+						user = new RSACryptoServiceProvider (1536, csp);
+					}
 				}
 				return user;
 			case DataProtectionScope.LocalMachine:
 				if (machine == null) {
-					CspParameters csp = new CspParameters ();
-					csp.KeyContainerName = "DAPI";
-					csp.Flags = CspProviderFlags.UseMachineKeyStore;
-					machine = new RSACryptoServiceProvider (1536, csp);
+					lock (machine_lock) {
+						CspParameters csp = new CspParameters ();
+						csp.KeyContainerName = "DAPI";
+						csp.Flags = CspProviderFlags.UseMachineKeyStore;
+						machine = new RSACryptoServiceProvider (1536, csp);
+					}
 				}
 				return machine;
 			default:
diff --git a/mcs/class/System.Security/Test/System.Security.Cryptography/ProtectedDataTest.cs b/mcs/class/System.Security/Test/System.Security.Cryptography/ProtectedDataTest.cs
index f18ca734af9..4848f686216 100644
--- a/mcs/class/System.Security/Test/System.Security.Cryptography/ProtectedDataTest.cs
+++ b/mcs/class/System.Security/Test/System.Security.Cryptography/ProtectedDataTest.cs
@@ -12,6 +12,9 @@
 using NUnit.Framework;
 
 using System;
+using System.Text;
+using System.Collections.Generic;
+using System.Threading.Tasks;
 using System.Security.Cryptography;
 
 namespace MonoTests.System.Security.Cryptography {
@@ -64,6 +67,31 @@ namespace MonoTests.System.Security.Cryptography {
 			ProtectUnprotect (notMuchEntropy, DataProtectionScope.LocalMachine);
 		}
 
+		[Test] // https://bugzilla.xamarin.com/show_bug.cgi?id=38933
+		public void ProtectCurrentUserMultiThread ()
+		{
+			string data = "Hello World";
+			string entropy = "This is a long string with no meaningful content.";
+			var entropyBytes = Encoding.UTF8.GetBytes (entropy);
+			var dataBytes = Encoding.UTF8.GetBytes (data);
+			var tasks = new List<Task> ();
+
+			for (int i = 0; i < 20; i++)
+			{
+				tasks.Add (new Task (() => {
+					byte[] encryptedBytes = ProtectedData.Protect (dataBytes, entropyBytes, DataProtectionScope.CurrentUser);
+					Assert.IsFalse (IsEmpty (encryptedBytes), "#1");
+
+					byte[] decryptedBytes = ProtectedData.Unprotect (encryptedBytes, entropyBytes, DataProtectionScope.CurrentUser);
+					string decryptedString = Encoding.UTF8.GetString(decryptedBytes);
+					Assert.AreEqual (data, decryptedString, "#2");
+				}, TaskCreationOptions.LongRunning));
+			}
+
+			foreach (var t in tasks) t.Start ();
+			Task.WaitAll (tasks.ToArray ());
+		}
+
 		[Test]
 		public void DataProtectionScope_All ()
 		{