Adding Check Against Injection Attacks

[FuPingFranco]

Description

Add a check in ID.Web to check for the presence of '&' for assertion and sub-assertion in ExtraQueryParameters dictionary.

[MZOLN]

This looks like a DID security fix am I correct?
If it is, we should be using whitelisting vs black listing.
Your comment in the function suggests that you perform (paraphrasing) "checks to ensure that assertions do not contain unsupported characters" but the implementation only checks for "&". There could be other characters or representation of characters that cause issue.

If we are discussing a security feature, the best approach is to whitelist vs blacklist, thus validate if each character in the assertion is in the expected range (for example A-z0-9) vs check if it contains unexpected characters.

[jmprieur]

I still don't understand the logic in CheckAssertionsForInjectionAttempt

[FuPingFranco]

This looks like a DID security fix am I correct? If it is, we should be using whitelisting vs black listing. Your comment in the function suggests that you perform (paraphrasing) "checks to ensure that assertions do not contain unsupported characters" but the implementation only checks for "&". There could be other characters or representation of characters that cause issue.

If we are discussing a security feature, the best approach is to whitelist vs blacklist, thus validate if each character in the assertion is in the expected range (for example A-z0-9) vs check if it contains unexpected characters.

Good point, I just did this check based on this PR https://identitydivision.visualstudio.com/DevEx/_git/Microsoft-IdentityModel-S2S/pullrequest/9985?_a=files&path=/src/Microsoft.IdentityModel.S2S.Tokens/TokenManager.cs I'll get more details to ensure I have a more complete solution. Thanks Marcin!

[FuPingFranco]

This looks like a DID security fix am I correct? If it is, we should be using whitelisting vs black listing. Your comment in the function suggests that you perform (paraphrasing) "checks to ensure that assertions do not contain unsupported characters" but the implementation only checks for "&". There could be other characters or representation of characters that cause issue.

If we are discussing a security feature, the best approach is to whitelist vs blacklist, thus validate if each character in the assertion is in the expected range (for example A-z0-9) vs check if it contains unexpected characters.

@MZOLN Could you open a new issues with a bit more details? I would like to close this task with the current defined scope. Thanks Marcin!

[jennyf19]

[jennyf19]

@FuPingFranco please create an issue in Id Web for this for easier tracking and link to the follow-up PR I did as well. thanks. #2639