Bug 650091 - Sort authentication challenges by safety rating r=necko-reviewers,kershaw

Differential Revision: https://phabricator.services.mozilla.com/D132499

modules/libpref/init/StaticPrefList.yaml

@@ -9321,6 +9321,12 @@
   value: true
   mirror: always
 
+# Whether to use challenges in the most secure order. See bug 650091.
+- name: network.auth.choose_most_secure_challenge
+  type: RelaxedAtomicBool
+  value: true
+  mirror: always
+
 # See the full list of values in nsICookieService.idl.
 - name: network.cookie.cookieBehavior
   type: RelaxedAtomicInt32

netwerk/protocol/http/nsHttpChannelAuthProvider.cpp

@@ -547,6 +547,38 @@ class MOZ_STACK_CLASS ChallengeParser final : Tokenizer {
   }
 };
 
+enum ChallengeRank {
+  Unknown = 0,
+  Basic = 1,
+  Digest = 2,
+  NTLM = 3,
+  Negotiate = 4,
+};
+
+ChallengeRank Rank(const nsACString& aChallenge) {
+  if (StringBeginsWith(aChallenge, "Negotiate"_ns,
+                       nsCaseInsensitiveCStringComparator)) {
+    return ChallengeRank::Negotiate;
+  }
+
+  if (StringBeginsWith(aChallenge, "NTLM"_ns,
+                       nsCaseInsensitiveCStringComparator)) {
+    return ChallengeRank::NTLM;
+  }
+
+  if (StringBeginsWith(aChallenge, "Digest"_ns,
+                       nsCaseInsensitiveCStringComparator)) {
+    return ChallengeRank::Digest;
+  }
+
+  if (StringBeginsWith(aChallenge, "Basic"_ns,
+                       nsCaseInsensitiveCStringComparator)) {
+    return ChallengeRank::Basic;
+  }
+
+  return ChallengeRank::Unknown;
+}
+
 nsresult nsHttpChannelAuthProvider::GetCredentials(
     const nsACString& aChallenges, bool proxyAuth, nsCString& creds) {
   LOG(("nsHttpChannelAuthProvider::GetCredentials"));
@@ -555,10 +587,12 @@ nsresult nsHttpChannelAuthProvider::GetCredentials(
   using AuthChallenge = struct AuthChallenge {
     nsDependentCSubstring challenge;
     uint16_t algorithm = 0;
+    ChallengeRank rank = ChallengeRank::Unknown;
 
     void operator=(const AuthChallenge& aOther) {
       challenge.Rebind(aOther.challenge, 0);
       algorithm = aOther.algorithm;
+      rank = aOther.rank;
     }
   };
 
@@ -574,6 +608,7 @@ nsresult nsHttpChannelAuthProvider::GetCredentials(
     nsAutoCString realm, domain, nonce, opaque;
     bool stale = false;
     uint16_t qop = 0;
+    ac.rank = Rank(ac.challenge);
     if (StringBeginsWith(ac.challenge, "Digest"_ns,
                          nsCaseInsensitiveCStringComparator)) {
       Unused << nsHttpDigestAuth::ParseChallenge(ac.challenge, realm, domain,
@@ -584,8 +619,26 @@ nsresult nsHttpChannelAuthProvider::GetCredentials(
   }
 
   cc.StableSort([](const AuthChallenge& lhs, const AuthChallenge& rhs) {
-    // Non-digest challenges should not be reordered.
-    if (!lhs.algorithm || !rhs.algorithm || lhs.algorithm == rhs.algorithm) {
+    if (StaticPrefs::network_auth_choose_most_secure_challenge()) {
+      // Different auth types
+      if (lhs.rank != rhs.rank) {
+        return lhs.rank < rhs.rank ? 1 : -1;
+      }
+
+      // If they're the same auth type, and not a Digest, then we treat them
+      // as equal (don't reorder them).
+      if (lhs.rank != ChallengeRank::Digest) {
+        return 0;
+      }
+    } else {
+      // Non-digest challenges should not be reordered when the pref is off.
+      if (lhs.algorithm == 0 || rhs.algorithm == 0) {
+        return 0;
+      }
+    }
+
+    // Same algorithm.
+    if (lhs.algorithm == rhs.algorithm) {
       return 0;
     }
     return lhs.algorithm < rhs.algorithm ? 1 : -1;

netwerk/test/unit/test_auth_multiple.js

@@ -23,14 +23,11 @@ XPCOMUtils.defineLazyGetter(this, "PORT", function() {
 const FLAG_RETURN_FALSE = 1 << 0;
 const FLAG_WRONG_PASSWORD = 1 << 1;
 const FLAG_BOGUS_USER = 1 << 2;
-const FLAG_PREVIOUS_FAILED = 1 << 3;
+// const FLAG_PREVIOUS_FAILED = 1 << 3;
 const CROSS_ORIGIN = 1 << 4;
-const FLAG_NO_REALM = 1 << 5;
+// const FLAG_NO_REALM = 1 << 5;
 const FLAG_NON_ASCII_USER_PASSWORD = 1 << 6;
 
-const nsIAuthPrompt2 = Ci.nsIAuthPrompt2;
-const nsIAuthInformation = Ci.nsIAuthInformation;
-
 function AuthPrompt1(flags) {
   this.flags = flags;
 }
@@ -261,12 +258,8 @@ function basic_auth(metadata, response) {
 // Digest functions
 //
 function bytesFromString(str) {
-  var converter = Cc[
-    "@mozilla.org/intl/scriptableunicodeconverter"
-  ].createInstance(Ci.nsIScriptableUnicodeConverter);
-  converter.charset = "UTF-8";
-  var data = converter.convertToByteArray(str);
-  return data;
+  const encoder = new TextEncoder("utf-8");
+  return encoder.encode(str);
 }
 
 // return the two-digit hexadecimal code for a byte
@@ -391,6 +384,10 @@ function setup() {
 setup();
 
 add_task(async function test_ntlm_first() {
+  Services.prefs.setBoolPref(
+    "network.auth.choose_most_secure_challenge",
+    false
+  );
   challenges = ["NTLM", `Basic realm="secret"`, digestChallenge];
   httpserv.identity.add("http", "ntlm.com", httpserv.identity.primaryPort);
   let chan = makeChan(URL("ntlm.com", "/path"));
@@ -406,6 +403,10 @@ add_task(async function test_ntlm_first() {
 });
 
 add_task(async function test_basic_first() {
+  Services.prefs.setBoolPref(
+    "network.auth.choose_most_secure_challenge",
+    false
+  );
   challenges = [`Basic realm="secret"`, "NTLM", digestChallenge];
   httpserv.identity.add("http", "basic.com", httpserv.identity.primaryPort);
   let chan = makeChan(URL("basic.com", "/path"));
@@ -421,6 +422,10 @@ add_task(async function test_basic_first() {
 });
 
 add_task(async function test_digest_first() {
+  Services.prefs.setBoolPref(
+    "network.auth.choose_most_secure_challenge",
+    false
+  );
   challenges = [digestChallenge, `Basic realm="secret"`, "NTLM"];
   httpserv.identity.add("http", "digest.com", httpserv.identity.primaryPort);
   let chan = makeChan(URL("digest.com", "/path"));
@@ -434,3 +439,25 @@ add_task(async function test_digest_first() {
   Assert.equal(req.QueryInterface(Ci.nsIHttpChannel).responseStatus, 200);
   Assert.equal(buf, "digest");
 });
+
+add_task(async function test_choose_most_secure() {
+  // When the pref is true, we rank the challenges by how secure they are.
+  // In this case, NTLM should be the most secure.
+  Services.prefs.setBoolPref("network.auth.choose_most_secure_challenge", true);
+  challenges = [digestChallenge, `Basic realm="secret"`, "NTLM"];
+  httpserv.identity.add(
+    "http",
+    "ntlmstrong.com",
+    httpserv.identity.primaryPort
+  );
+  let chan = makeChan(URL("ntlmstrong.com", "/path"));
+
+  chan.notificationCallbacks = new Requestor(FLAG_RETURN_FALSE, 2);
+  let [req, buf] = await new Promise(resolve => {
+    chan.asyncOpen(
+      new ChannelListener((req, buf) => resolve([req, buf]), null)
+    );
+  });
+  Assert.equal(req.QueryInterface(Ci.nsIHttpChannel).responseStatus, 200);
+  Assert.equal(buf, "OK");
+});