Skip to content
This repository has been archived by the owner on Jul 5, 2020. It is now read-only.

[2.11.0-beta1] guard against malicious headers in quickpulse #1191

Merged
merged 12 commits into from
Jun 19, 2019

Conversation

TimothyMothra
Copy link
Member

@TimothyMothra TimothyMothra commented May 15, 2019

Part 1: Cijo please review

In a previous SDL we introduced InjectionGuardConstants to protect from malicious values being injected into the SDK and causing memory overruns.

I found a place in QuickPulse not doing this check.

(For reference see, mseng bug 651018)

Part 2: Alex please review

QuickPulseServiceClient was retrieving an array of request headers but only using the First() value.
I didn't want to do a length check on an entire array if only the first value would be used.
I changed the implementation to only return the first element in an array.

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. blocking to not merge now to avoid a conflict. will sync on this in person.

Src/Common/InjectionGuardConstants.cs Outdated Show resolved Hide resolved
@TimothyMothra TimothyMothra added this to the 2.11 milestone Jun 18, 2019
@TimothyMothra TimothyMothra changed the title guard against malicious headers in quickpulse [2.11-beta1] guard against malicious headers in quickpulse Jun 18, 2019
@TimothyMothra TimothyMothra changed the title [2.11-beta1] guard against malicious headers in quickpulse [2.11.0-beta1] guard against malicious headers in quickpulse Jun 18, 2019
@TimothyMothra
Copy link
Member Author

@cijothomas is it safe to merge this now? :)

@TimothyMothra TimothyMothra merged commit ec904d0 into develop Jun 19, 2019
@TimothyMothra TimothyMothra deleted the tilee/quickpulse_protect_headers branch June 19, 2019 20:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants