From 01b728719b5f29c88fcf8c422285592a0a89b34d Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Tue, 7 Dec 2021 11:30:50 +0000 Subject: [PATCH] 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 | 6 ++ .../http/nsHttpChannelAuthProvider.cpp | 57 ++++++++++++++++++- netwerk/test/unit/test_auth_multiple.js | 49 ++++++++++++---- 3 files changed, 99 insertions(+), 13 deletions(-) diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index e4b0c19b82d6..361c9032e81b 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/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 diff --git a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp b/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp index 8033c2fb2b7a..8875104ffca4 100644 --- a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp +++ b/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; diff --git a/netwerk/test/unit/test_auth_multiple.js b/netwerk/test/unit/test_auth_multiple.js index d177be33ed2d..8ae1e1f32279 100644 --- a/netwerk/test/unit/test_auth_multiple.js +++ b/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"); +});