Misuse of cryptographic primitives

[Tyratox]

Hello there :)

During a security review we noticed the following issues in this repository and would like them to confirm them so that they can the hopefully be fixed:

Missing integrity protection

api-management-policy-snippets/examples/oauth-proxy/oauth-proxy-slide-session-fragment.xml

Lines 6 to 25 in 87225c2

	<set-variable name="cookie-expiry" value="@(DateTimeOffset.UtcNow.AddSeconds({{SessionCookieExpirationInSeconds}}).ToUnixTimeMilliseconds())" />
	<set-variable name="cookie-prefix" value="@($"{(string)context.Variables["cacheKey"]}.{(string)context.Variables["ivTokens"]}.{(long)context.Variables["cookie-expiry"]}")" />
	<!-- encrypt this using the cookie-iv-->
	<set-variable name="encryptedCookie" value="@{
	var cookie = Encoding.UTF8.GetBytes((string)context.Variables["cookie-prefix"]);
	var ivString = (string)context.Variables["ivCookie"];
	var iv = Guid.Parse(ivString).ToByteArray();
	var key1 = Convert.FromBase64String("{{CookieEncryptionKey1}}");
	var key2 = Convert.FromBase64String("{{CookieEncryptionKey2}}");
	var key = {{CookieEncryptionKey}};
	var encryptionKey = key == 1 ? key1 : key2;
	var encryptedCookie = cookie.Encrypt("Aes", encryptionKey, iv);
	return $"{Convert.ToBase64String(encryptedCookie)}.{ivString}.{key}";
	}" />
	<!-- add the new session cookie to the response -->
	<set-header name="Set-Cookie" exists-action="append">
	<value>@($"{{CookiePrefix}}={(string)context.Variables["encryptedCookie"]}; SameSite=Lax; secure; path=/; expires={DateTimeOffset.FromUnixTimeMilliseconds((long)context.Variables["cookie-expiry"]).ToString("R")}; Secure; HttpOnly" )</value>
	</set-header>

Information is stored in an encrypted cookie that is not integrity protected. Therefore an attacker may tamper with the cookie to modify the expiration date.

The documentation correctly points out the steps that would be necessary to protect the integrity, i.e. a signature / HMAC or a cipher mode with integrity protection like AES-GCM

api-management-policy-snippets/examples/oauth-proxy/readme.md

Lines 142 to 159 in 87225c2

	## oauth-proxy-callback
	> Implemented by [oauth-proxy-callback.xml](./oauth-proxy-callback.xml)
	### Purpose
	This policy handles a callback from an IdP to complete an OIDC flow.
	### Steps
	- Get the ```code``` and ```state``` parameter from the incoming querystring
	- Check for an incoming ```oidc``` cookie suffixed with the ```state``` parameter
	- Lookup the state and nonce properties from cache which were previously stored in the ```signin``` policy
	- Return 401 if we cannot find them
	- If the state parameter in the querystring from the IdP matches the cookie, and was stored in our cache, then switch the code for a token using a PKCE code-
	- Check the nonce in the returned token matches the nonce stored in session
	- Return 401 if we cannot match the nonce
	- Creates an IV which is round-tripped in the session cookie (not stored server-side)
	- Encrypts the tokens using the above IV, and the TokenEncryptionKey(1 or 2)
	- Store the encrypted tokens in Redis
	- Set a session-cookie which comprises of our cache-key, the IV, the cookies expiry timestamp. Signs it using a HMAC-SHA-512 signature creating using the SessionCookieKey(1 or 2) named value.

According to the Microsoft documentation at https://learn.microsoft.com/en-us/azure/api-management/api-management-policy-expressions#ref-context-request, there is no parameter for the cipher mode and according to the .NET documentation, the default cipher mode is CBC (https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.symmetricalgorithm.mode?view=net-10.0#property-value) which does not offer integrity protection.

Let me know if there is a part that is missing.

Using an IV as a secret

According to the comments in the code, the idea is to use store an IV with the cookie so that the cached contents cannot be decrypted without the cookie. The problem is that the IV is not a replacement for the secret and effectively only protects the first block in CBC and otherwise just makes sure the encryption is randomized.

api-management-policy-snippets/examples/oauth-proxy/oauth-proxy-callback.xml

Lines 78 to 106 in 87225c2

	<!-- The plan -->
	<!-- Encrypt all the tokens using the current TokenEncryptionKey value (as indicated by the named-value, TokenEncryptionKey). IV is a new Guid -->
	<!-- Store all of these in the APIM cache suffixed with '.1' or '.2' to indicate which TokenEncryptionKey was used -->
	<!-- The plain-text of the cookie is [cacheKey].[tokenIv].[cookieExpiry] -->
	<!-- Encrypt the cookie using the Cookie Encryption key, and another new Guid as the IV-->
	<!-- Return the cookie as [encrypted-plain-text].[cookieIv].[1|2] -->
	<!-- The IV for the tokens is only stored inside the cookie meaning no-one can decrypt the tokens without the cookie -->
	<!-- The tokens are not accessible to the client-apps meaning they cannot leak through a front channel -->
	<!-- The tokens are stored with information about which of the 2 possible keys encrypted them, allowing key refresh -->
	<!-- The cookie sent to the client also has information about which of the 2 possible keys encrypted it, allowing key refresh -->
	<!-- create the IV used in encrypting the cookie. This is stored in our cache and looked up using the cache key -->
	<set-variable name="ivCookie" value="@(Guid.NewGuid().ToString())" />
	<!-- create the IV used in encrypting the tokens. We don't store this server side. It flows encrypted in cookies -->
	<set-variable name="ivTokens" value="@(Guid.NewGuid().ToString())" />
	<!-- A new GUID used to lookup the IV in the cache. Keeping it different to the other cache key -->
	<set-variable name="ivCookieCacheKey" value="@(Guid.NewGuid().ToString())" />
	<!-- Encrypt the tokenResponse variable, and store that in cache. We'll handle the refresh token separately as it lasts longer -->
	<set-variable name="encryptedAccessToken" value="@{
	var token = Encoding.UTF8.GetBytes((string)context.Variables["accessToken"]);
	var iv = Guid.Parse((string)context.Variables["ivTokens"]).ToByteArray();
	var key1 = Convert.FromBase64String("{{TokenEncryptionKey1}}");
	var key2 = Convert.FromBase64String("{{TokenEncryptionKey2}}");
	var key = {{TokenEncryptionKey}};
	var encryptionKey = key == 1 ? key1 : key2;
	var encryptedToken = token.Encrypt("Aes", encryptionKey, iv);
	return $"{Convert.ToBase64String(encryptedToken)}.{key}";
	}" />

If you look at the following graphic showing CBC, you can see that even without knowing the IV, all steps can be performed in reverse except for the last one giving you all plaintext blocks except for the first one.

If you wanted to accomplish what is outlined in the plan, you would instead need to generate an ephemeral secret, store that in the cookie which is encrypted with the server-secrets (as done with the IV right now) and then store the IV together with the encrypted tokens in the cache. This way the server does know the IV but cannot decrypt the contents without knowing the ephemeral secret that is stored in the user cookie.

This issue is not as bad as the first one as it would require an attacker to compromise the server in order to decrypt the contents. Note that this secret is of no use to the client unless they also get access to the encrypted tokens which are never shared with them. To decrypt the token, a collaboration of client and server is therefore necessary which seems to be what the original plan was (according to my interpretation of the comments).

As a side note: The variable ivCookieCacheKey seems to be redundant.

Thanks you for the great policy fragements and let me know if you disagree on some of the points or would like to discuss them.