Conversation
| private void GenerateCorrelationIdAndAddToDictionary(string ikey, string appId) | ||
| { | ||
| // Arbitrary maximum length to guard against injections. | ||
| appId = StringUtilities.EnforceMaxLength(appId, InjectionGuardConstants.AppIdMaxLengeth); |
There was a problem hiding this comment.
Should we just drop the AppId if it does not satisfy the expected length and/or format? However, let's see if it's actually consuming less resources than to pass it around after it's been cut to MaxLength and write once into the item.
There was a problem hiding this comment.
I don't understand your "less resources" comment.
If we drop the AppId, the sdk will keep making appId requests (after brief timeout). I think either approach could be valid, but lets discuss what's best for this use case.
There was a problem hiding this comment.
There are two ways we can go about appId if we think it's wrong (e.g. does not satisfy length or format):
- Drop it and let it query for it again later;
- Accept it (e.g. cut to length or ignore the wrong format) and let SDK process the wrong appID entry everywhere down the line.
Both cases have their own pros, so I was interested in which one is "cheaper" from CPU/Memory perspective - trying out for new appId or keep re-using the wrong one on related code paths.
There was a problem hiding this comment.
Discussed offline with Dmitry.
We are deciding to truncate the value and accept it as is.
Assuming that if an attacker injected a fraudulent value, that attacker would continue to inject fraudulent values on retry attempts. Best to quit and wait until app pool recycles to try again.
| /// <summary> | ||
| /// Max length of incoming Request Header value allowed. | ||
| /// </summary> | ||
| public const int RequestHeaderMaxLength = 100; |
There was a problem hiding this comment.
Will 100 limit be enough for correlation properties in Request_Context header? I remember there was some property bag propagated though one of the correlation headers... @SergeyKanzhelev would know the right length to cut it as per spec.
There was a problem hiding this comment.
Today it only has app-id=v1_guid or something like this. So 100 may be enough. I believe @iusafaro removed role name reading/setting logic already so no role name in this header any longer.
We will definitely be in trouble when we will want to send more than a single property. So maybe allow up to 1Kb to be safe, but not break potential scenarios.
Note, this header will be gone with the new W3C headers.
| Debug.Assert(input != null, $"{nameof(input)} must not be null"); | ||
| Debug.Assert(maxLength > 0, $"{nameof(maxLength)} must be greater than 0"); | ||
|
|
||
| if (input != null && input.Length > maxLength) |
There was a problem hiding this comment.
We seem to have assert on maxLength > 0 but still execute code with that value - is it intentional?
There was a problem hiding this comment.
I saw this design pattern used in other places.
Not sure about best practices in this case.
There was a problem hiding this comment.
Discussed with Dmitry offline.
We're going to leave this for now, both conditions are already addressed in the if statement so no risk to the application.
At a later date, we'll add a Test Project around our Common library and remove these asserts.
3 participants
Fix for issue discovered in SDL review. Will guard against arbitrary length strings received from outside the application.