Skip to content

Commit

Permalink
[CFNetwork] Fix CVE 2018-8292 on Mac OS X. (#5272)
Browse files Browse the repository at this point in the history
The MessageHandler class suffers from the CVE 2018-8292. this commit
fixes the issue by ensuring that the we donot use autoredirect from
CFNetwork and perform the first request. If the response is a redirect,
follow it wihout the Autherization headers.
  • Loading branch information
monojenkins authored and mandel-macaque committed Dec 14, 2018
1 parent 2e9867a commit 0406829
Showing 1 changed file with 40 additions and 11 deletions.
51 changes: 40 additions & 11 deletions src/CFNetwork/MessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

namespace CFNetwork {

[Obsolete ("Use 'System.Net.Http.CFNetworkHandler' or the more recent 'Foundation.NSUrlSessionHandler' instead.")]
public class MessageHandler : HttpClientHandler {
public MessageHandler ()
{
Expand Down Expand Up @@ -86,19 +87,19 @@ protected override async Task<HttpResponseMessage> SendAsync (HttpRequestMessage
if (!request.RequestUri.IsAbsoluteUri)
throw new InvalidOperationException ();

using (var message = CreateRequest (request)) {
using (var message = CreateRequest (request, true)) {
var body = await CreateBody (request, message, cancellationToken);
return await ProcessRequest (request, message, body, true, cancellationToken);
return await ProcessRequest (request, message, body, true, cancellationToken, true);
}
}
#endregion

CFHTTPMessage CreateRequest (HttpRequestMessage request)
CFHTTPMessage CreateRequest (HttpRequestMessage request, bool isFirstRequest)
{
var message = CFHTTPMessage.CreateRequest (
request.RequestUri, request.Method.Method, request.Version);

SetupRequest (request, message);
SetupRequest (request, message, isFirstRequest);

if ((auth == null) || (Credentials == null) || !PreAuthenticate)
return message;
Expand All @@ -111,11 +112,12 @@ CFHTTPMessage CreateRequest (HttpRequestMessage request)
if (credential == null)
return message;

message.ApplyCredentials (auth, credential);
if (isFirstRequest)
message.ApplyCredentials (auth, credential);
return message;
}

void SetupRequest (HttpRequestMessage request, CFHTTPMessage message)
void SetupRequest (HttpRequestMessage request, CFHTTPMessage message, bool isFirstRequest)
{
string accept_encoding = null;
if ((AutomaticDecompression & DecompressionMethods.GZip) != 0)
Expand All @@ -127,6 +129,8 @@ void SetupRequest (HttpRequestMessage request, CFHTTPMessage message)

if (request.Content != null) {
foreach (var header in request.Content.Headers) {
if (!isFirstRequest && header.Key == "Authorization")
continue;
var value = string.Join (",", header.Value);
message.SetHeaderFieldValue (header.Key, value);
}
Expand Down Expand Up @@ -190,12 +194,23 @@ bool GetKeepAlive (HttpRequestMessage request)

return request.Headers.Contains ("Keep-Alive");
}

// Decide if we redirect or not, similar to what is done in the managed handler
// https://github.com/mono/mono/blob/eca15996c7163f331c9f2cd0a17b63e8f92b1d55/mcs/class/referencesource/System/net/System/Net/HttpWebRequest.cs#L5681
static bool IsRedirect (HttpStatusCode status)
{
return status == HttpStatusCode.Ambiguous || // 300
status == HttpStatusCode.Moved || // 301
status == HttpStatusCode.Redirect || // 302
status == HttpStatusCode.RedirectMethod || // 303
status == HttpStatusCode.RedirectKeepVerb; // 307
}

async Task<HttpResponseMessage> ProcessRequest (HttpRequestMessage request,
CFHTTPMessage message,
WebRequestStream body,
bool retryWithCredentials,
CancellationToken cancellationToken)
CancellationToken cancellationToken, bool isFirstRequest)
{
cancellationToken.ThrowIfCancellationRequested ();

Expand All @@ -210,21 +225,35 @@ async Task<HttpResponseMessage> ProcessRequest (HttpRequestMessage request,
request.RequestUri)
);

stream.Stream.ShouldAutoredirect = AllowAutoRedirect;
if (!isFirstRequest)
stream.Stream.ShouldAutoredirect = AllowAutoRedirect;
stream.Stream.AttemptPersistentConnection = GetKeepAlive (request);

var response = await stream.Open (
WorkerThread, cancellationToken).ConfigureAwait (false);
WorkerThread, cancellationToken).ConfigureAwait (true); // with false, we will have a deadlock.

var status = (HttpStatusCode)response.ResponseStatusCode;

if ( IsRedirect (status)) {
request.Headers.Authorization = null;
stream.Dispose ();
// we cannot reuse the message, will deadlock and also the message.ApplyCredentials (auth, credential);
// was called the first time
using (var retryMsg = CreateRequest (request, false)) {
return await ProcessRequest (
request, retryMsg, null, false, cancellationToken, false);
}

}
if (retryWithCredentials && (body == null) &&
(status == HttpStatusCode.Unauthorized) ||
(status == HttpStatusCode.ProxyAuthenticationRequired)) {
if (HandleAuthentication (request.RequestUri, message, response)) {
stream.Dispose ();
return await ProcessRequest (
request, message, null, false, cancellationToken);
using (var retryMsg = CreateRequest (request, true)) { // behave as if it was the first attempt
return await ProcessRequest (
request, message, null, false, cancellationToken, true); // behave as if it was the first attempt
}
}
}

Expand Down

3 comments on commit 0406829

@xamarin-release-manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥 Jenkins job (on internal Jenkins) failed in stage(s) 'Test run' 🔥

Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 283 tests passed.

Failed tests

  • MSBuild tests/iOS: Failed (Execution failed with exit code 2)

@rolfbjarne
Copy link
Member

Choose a reason for hiding this comment

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

Test failures is unrelated:

@rolfbjarne
Copy link
Member

Choose a reason for hiding this comment

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

Setting state to success where context is continuous-integration/jenkins/branch.

No blocking issues found

Please sign in to comment.