From 39cf02c89d97a06c893be78ca1e24fe019630597 Mon Sep 17 00:00:00 2001 From: Iulian Moraru Date: Thu, 15 Jun 2023 17:48:35 +0300 Subject: [PATCH] Backed out changeset 3fc04d8eb32b (bug 1659763) for frame checker crashes. a=backout --- dom/security/FramingChecker.cpp | 208 +++++++++--------- dom/security/FramingChecker.h | 10 +- netwerk/protocol/http/nsHttpAtomList.h | 1 - netwerk/protocol/http/nsHttpHeaderArray.cpp | 9 +- netwerk/protocol/http/nsHttpHeaderArray.h | 9 +- .../meta/x-frame-options/multiple.html.ini | 84 +++++++ 6 files changed, 204 insertions(+), 117 deletions(-) diff --git a/dom/security/FramingChecker.cpp b/dom/security/FramingChecker.cpp index caeac5798629..39fcea268ab5 100644 --- a/dom/security/FramingChecker.cpp +++ b/dom/security/FramingChecker.cpp @@ -5,26 +5,26 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "FramingChecker.h" - -#include // uint32_t - -#include "nsCOMPtr.h" +#include "nsCharSeparatedTokenizer.h" #include "nsContentUtils.h" -#include "nsDebug.h" -#include "nsError.h" +#include "nsCSPUtils.h" +#include "nsDocShell.h" #include "nsHttpChannel.h" #include "nsContentSecurityUtils.h" -#include "nsIContentPolicy.h" +#include "nsGlobalWindowOuter.h" +#include "nsIChannel.h" +#include "nsIConsoleReportCollector.h" +#include "nsIContentSecurityPolicy.h" #include "nsIScriptError.h" -#include "nsLiteralString.h" +#include "nsNetUtil.h" +#include "nsQueryObject.h" #include "nsTArray.h" -#include "nsStringFwd.h" -#include "mozilla/Assertions.h" +#include "mozilla/BasePrincipal.h" +#include "mozilla/dom/nsCSPUtils.h" +#include "mozilla/dom/LoadURIOptionsBinding.h" #include "mozilla/dom/WindowGlobalParent.h" +#include "mozilla/NullPrincipal.h" #include "mozilla/net/HttpBaseChannel.h" -#include "mozilla/RefPtr.h" -#include "mozilla/Services.h" -#include "mozilla/Unused.h" #include "nsIObserverService.h" @@ -57,13 +57,72 @@ void FramingChecker::ReportError(const char* aMessageTag, httpChannel->AddConsoleReport(nsIScriptError::errorFlag, "X-Frame-Options"_ns, nsContentUtils::eSECURITY_PROPERTIES, spec, 0, 0, nsDependentCString(aMessageTag), params); +} - // we are notifying observers for testing purposes because there is no event - // to gather that an iframe load was blocked or not. - nsCOMPtr observerService = - mozilla::services::GetObserverService(); - nsAutoString policy(aPolicy); - observerService->NotifyObservers(aURI, "xfo-on-violate-policy", policy.get()); +/* static */ +bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel, + const nsAString& aPolicy) { + nsCOMPtr uri; + aHttpChannel->GetURI(getter_AddRefs(uri)); + + // return early if header does not have one of the values with meaning + if (!aPolicy.LowerCaseEqualsLiteral("deny") && + !aPolicy.LowerCaseEqualsLiteral("sameorigin")) { + ReportError("XFrameOptionsInvalid", aHttpChannel, uri, aPolicy); + return true; + } + + // If the X-Frame-Options value is SAMEORIGIN, then the top frame in the + // parent chain must be from the same origin as this document. + bool checkSameOrigin = aPolicy.LowerCaseEqualsLiteral("sameorigin"); + + nsCOMPtr loadInfo = aHttpChannel->LoadInfo(); + RefPtr ctx; + loadInfo->GetBrowsingContext(getter_AddRefs(ctx)); + + while (ctx) { + nsCOMPtr principal; + // Generally CheckOneFrameOptionsPolicy is consulted from within the + // DocumentLoadListener in the parent process. For loads of type object + // and embed it's called from the Document in the content process. + // After Bug 1646899 we should be able to remove that branching code for + // querying the principal. + if (XRE_IsParentProcess()) { + WindowGlobalParent* window = ctx->Canonical()->GetCurrentWindowGlobal(); + if (window) { + // Using the URI of the Principal and not the document because e.g. + // window.open inherits the principal and hence the URI of the + // opening context needed for same origin checks. + principal = window->DocumentPrincipal(); + } + } else if (nsPIDOMWindowOuter* windowOuter = ctx->GetDOMWindow()) { + principal = nsGlobalWindowOuter::Cast(windowOuter)->GetPrincipal(); + } + + if (principal && principal->IsSystemPrincipal()) { + return true; + } + + if (checkSameOrigin) { + bool isSameOrigin = principal && principal->IsSameOrigin(uri); + // one of the ancestors is not same origin as this document + if (!isSameOrigin) { + ReportError("XFrameOptionsDeny", aHttpChannel, uri, aPolicy); + return false; + } + } + ctx = ctx->GetParent(); + } + + // If the value of the header is DENY, and the previous condition is + // not met (current docshell is not the top docshell), prohibit the + // load. + if (aPolicy.LowerCaseEqualsLiteral("deny")) { + ReportError("XFrameOptionsDeny", aHttpChannel, uri, aPolicy); + return false; + } + + return true; } // Ignore x-frame-options if CSP with frame-ancestors exists @@ -89,34 +148,33 @@ static bool ShouldIgnoreFrameOptions(nsIChannel* aChannel, // subdocument. This will iterate through and check any number of // X-Frame-Options policies in the request (comma-separated in a header, // multiple headers, etc). -// This is based on: -// https://html.spec.whatwg.org/multipage/document-lifecycle.html#the-x-frame-options-header /* static */ bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel, nsIContentSecurityPolicy* aCsp, bool& outIsFrameCheckingSkipped) { - // Step 1. If navigable is not a child navigable return true if (!aChannel) { return true; } - // xfo check only makes sense for subdocument and object loads, if this is - // not a load of such type, there is nothing to do here. nsCOMPtr loadInfo = aChannel->LoadInfo(); ExtContentPolicyType contentType = loadInfo->GetExternalContentPolicyType(); + + // xfo check only makes sense for subdocument and object loads, if this is + // not a load of such type, there is nothing to do here. if (contentType != ExtContentPolicy::TYPE_SUBDOCUMENT && contentType != ExtContentPolicy::TYPE_OBJECT) { return true; } - // xfo can only hang off an httpchannel, if this is not an httpChannel - // then there is nothing to do here. nsCOMPtr httpChannel; nsresult rv = nsContentSecurityUtils::GetHttpChannelFromPotentialMultiPart( aChannel, getter_AddRefs(httpChannel)); if (NS_WARN_IF(NS_FAILED(rv))) { return true; } + + // xfo can only hang off an httpchannel, if this is not an httpChannel + // then there is nothing to do here. if (!httpChannel) { return true; } @@ -124,100 +182,50 @@ bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel, // ignore XFO checks on channels that will be redirected uint32_t responseStatus; rv = httpChannel->GetResponseStatus(&responseStatus); + // GetResponseStatus returning failure is expected in several situations, so + // do not warn if it fails. if (NS_FAILED(rv)) { - // GetResponseStatus returning failure is expected in several situations, so - // do not warn if it fails. return true; } if (mozilla::net::nsHttpChannel::IsRedirectStatus(responseStatus)) { return true; } - nsAutoCString xfoHeaderValue; + nsAutoCString xfoHeaderCValue; Unused << httpChannel->GetResponseHeader("X-Frame-Options"_ns, - xfoHeaderValue); + xfoHeaderCValue); + NS_ConvertUTF8toUTF16 xfoHeaderValue(xfoHeaderCValue); - // Step 10. (paritally) if the only header we received was empty, then we - // process it as if it wasn't sent at all. + // if no header value, there's nothing to do. if (xfoHeaderValue.IsEmpty()) { return true; } - // Step 2. xfo checks are ignored in the case where CSP frame-ancestors is - // present, if so, there is nothing to do here. + // xfo checks are ignored in case CSP frame-ancestors is present, + // if so, there is nothing to do here. if (ShouldIgnoreFrameOptions(aChannel, aCsp)) { outIsFrameCheckingSkipped = true; return true; } - // Step 3-4. reduce the header options to a unique set and count how many - // unique values (that we track) are encountered. this avoids using a set to - // stop attackers from inheriting arbitrary values in memory and reduce the - // complexity of the code. - XFOHeader xfoOptions; - for (const nsACString& next : xfoHeaderValue.Split(',')) { - nsAutoCString option(next); - option.StripWhitespace(); - - if (option.LowerCaseEqualsLiteral("allowall")) { - xfoOptions.ALLOWALL = true; - } else if (option.LowerCaseEqualsLiteral("sameorigin")) { - xfoOptions.SAMEORIGIN = true; - } else if (option.LowerCaseEqualsLiteral("deny")) { - xfoOptions.DENY = true; - } else { - xfoOptions.INVALID = true; - } - } - - nsCOMPtr uri; - httpChannel->GetURI(getter_AddRefs(uri)); - - // Step 6. if header has multiple contradicting directives return early and - // prohibit the load. ALLOWALL is considered here for legacy reasons. - uint32_t xfoUniqueOptions = xfoOptions.DENY + xfoOptions.ALLOWALL + - xfoOptions.SAMEORIGIN + xfoOptions.INVALID; - if (xfoUniqueOptions > 1 && - (xfoOptions.DENY || xfoOptions.ALLOWALL || xfoOptions.SAMEORIGIN)) { - ReportError("XFrameOptionsInvalid", httpChannel, uri, u"invalid"_ns); - return false; - } - - // Step 7 (multiple INVALID values) and partially Step 10 (single INVALID - // value). if header has any invalid options, but no valid directives (DENY, - // ALLOWALL, SAMEORIGIN) then allow the load. - if (xfoOptions.INVALID) { - ReportError("XFrameOptionsInvalid", httpChannel, uri, u"invalid"_ns); - return true; - } - - // Step 8. if the value of the header is DENY prohibit the load. - if (xfoOptions.DENY) { - ReportError("XFrameOptionsDeny", httpChannel, uri, u"deny"_ns); - return false; - } - - // Step 9. If the X-Frame-Options value is SAMEORIGIN, then the top frame in - // the parent chain must be from the same origin as this document. - RefPtr ctx; - loadInfo->GetBrowsingContext(getter_AddRefs(ctx)); - - while (ctx && xfoOptions.SAMEORIGIN) { - nsCOMPtr principal = - ctx->Canonical()->GetCurrentWindowGlobal()->DocumentPrincipal(); - - if (principal && principal->IsSystemPrincipal()) { - return true; - } - - // one of the ancestors is not same origin as this document - if (!principal || !principal->IsSameOrigin(uri)) { - ReportError("XFrameOptionsDeny", httpChannel, uri, u"sameorigin"_ns); + // iterate through all the header values (usually there's only one, but can + // be many. If any want to deny the load, deny the load. + nsCharSeparatedTokenizer tokenizer(xfoHeaderValue, ','); + while (tokenizer.hasMoreTokens()) { + const nsAString& tok = tokenizer.nextToken(); + if (!CheckOneFrameOptionsPolicy(httpChannel, tok)) { + // if xfo blocks the load we are notifying observers for + // testing purposes because there is no event to gather + // what an iframe load was blocked or not. + nsCOMPtr uri; + httpChannel->GetURI(getter_AddRefs(uri)); + nsCOMPtr observerService = + mozilla::services::GetObserverService(); + nsAutoString policy(tok); + observerService->NotifyObservers(uri, "xfo-on-violate-policy", + policy.get()); return false; } - ctx = ctx->GetParent(); } - - // Step 10. return true; } diff --git a/dom/security/FramingChecker.h b/dom/security/FramingChecker.h index a91c35307285..45c43031e0ce 100644 --- a/dom/security/FramingChecker.h +++ b/dom/security/FramingChecker.h @@ -29,12 +29,7 @@ class FramingChecker { bool& outIsFrameCheckingSkipped); protected: - struct XFOHeader { - bool ALLOWALL = false; - bool SAMEORIGIN = false; - bool DENY = false; - bool INVALID = false; - }; + enum XFOHeader { eDENY, eSAMEORIGIN }; /** * Logs to the window about a X-Frame-Options error. @@ -46,6 +41,9 @@ class FramingChecker { */ static void ReportError(const char* aMessageTag, nsIHttpChannel* aChannel, nsIURI* aURI, const nsAString& aPolicy); + + static bool CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel, + const nsAString& aPolicy); }; #endif /* mozilla_dom_FramingChecker_h */ diff --git a/netwerk/protocol/http/nsHttpAtomList.h b/netwerk/protocol/http/nsHttpAtomList.h index 5b497af43e95..af8882af0b40 100644 --- a/netwerk/protocol/http/nsHttpAtomList.h +++ b/netwerk/protocol/http/nsHttpAtomList.h @@ -103,6 +103,5 @@ HTTP_ATOM(X_Firefox_Spdy, "X-Firefox-Spdy") HTTP_ATOM(X_Firefox_Spdy_Proxy, "X-Firefox-Spdy-Proxy") HTTP_ATOM(X_Firefox_Early_Data, "X-Firefox-Early-Data") HTTP_ATOM(X_Firefox_Http3, "X-Firefox-Http3") -HTTP_ATOM(X_Frame_Options, "X-Frame-Options") // methods are case sensitive and do not use atom table diff --git a/netwerk/protocol/http/nsHttpHeaderArray.cpp b/netwerk/protocol/http/nsHttpHeaderArray.cpp index 2c57fcc67621..944cf30f00b3 100644 --- a/netwerk/protocol/http/nsHttpHeaderArray.cpp +++ b/netwerk/protocol/http/nsHttpHeaderArray.cpp @@ -46,10 +46,13 @@ nsresult nsHttpHeaderArray::SetHeader( "Net original headers can only be set using SetHeader_internal()."); nsEntry* entry = nullptr; - int32_t index = LookupEntry(header, &entry); + int32_t index; - // If an empty value is received and we aren't merging headers discard it - if (value.IsEmpty() && header != nsHttp::X_Frame_Options) { + index = LookupEntry(header, &entry); + + // If an empty value is passed in, then delete the header entry... + // unless we are merging, in which case this function becomes a NOP. + if (value.IsEmpty()) { if (!merge && entry) { if (entry->variety == eVarietyResponseNetOriginalAndResponse) { MOZ_ASSERT(variety == eVarietyResponse); diff --git a/netwerk/protocol/http/nsHttpHeaderArray.h b/netwerk/protocol/http/nsHttpHeaderArray.h index 2cca578a1b76..622c97ace7ee 100644 --- a/netwerk/protocol/http/nsHttpHeaderArray.h +++ b/netwerk/protocol/http/nsHttpHeaderArray.h @@ -246,15 +246,10 @@ inline bool nsHttpHeaderArray::IsIgnoreMultipleHeader( [[nodiscard]] inline nsresult nsHttpHeaderArray::MergeHeader( const nsHttpAtom& header, nsEntry* entry, const nsACString& value, nsHttpHeaderArray::HeaderVariety variety) { - // merge of empty header = no-op - if (value.IsEmpty() && header != nsHttp::X_Frame_Options) { - return NS_OK; - } + if (value.IsEmpty()) return NS_OK; // merge of empty header = no-op - // x-frame-options having an empty header value still has an effect so we make - // sure that we retain encountering it nsCString newValue = entry->value; - if (!newValue.IsEmpty() || header == nsHttp::X_Frame_Options) { + if (!newValue.IsEmpty()) { // Append the new value to the existing value if (header == nsHttp::Set_Cookie || header == nsHttp::WWW_Authenticate || header == nsHttp::Proxy_Authenticate) { diff --git a/testing/web-platform/meta/x-frame-options/multiple.html.ini b/testing/web-platform/meta/x-frame-options/multiple.html.ini index b98b0b1d2bd5..6cb866eb49c9 100644 --- a/testing/web-platform/meta/x-frame-options/multiple.html.ini +++ b/testing/web-platform/meta/x-frame-options/multiple.html.ini @@ -5,6 +5,90 @@ if debug and (os == "linux") and fission and swgl: [OK, TIMEOUT] if debug and (os == "linux") and not fission: [OK, TIMEOUT] if debug and (os == "android") and fission: [OK, TIMEOUT] + [`ALLOWALL;INVALID` blocks same-origin framing] + expected: FAIL + + [`(the empty string),SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN,INVALID` blocks same-origin framing] + expected: FAIL + + [`INVALID;allowAll` blocks same-origin framing] + expected: FAIL + + [`INVALID,SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`ALLOWALL,INVALID` blocks same-origin framing] + expected: FAIL + + [`INVALID,allowAll` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN;` blocks same-origin framing] + expected: FAIL + + [`ALLOWALL,(the empty string)` blocks same-origin framing] + expected: FAIL + + [`INVALID,ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`INVALID;SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`(the empty string),ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`ALLOWALL,SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`ALLOWALL;` blocks same-origin framing] + expected: FAIL + + [`(the empty string);SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`allowAll,INVALID` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN,ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`ALLOWALL;SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`"DENY",SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`"DENY";SAMEORIGIN` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN,(the empty string)` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN;INVALID` blocks same-origin framing] + expected: FAIL + + [`(the empty string);ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN;ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`INVALID;ALLOWALL` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN;"DENY"` blocks same-origin framing] + expected: FAIL + + [`allowAll;INVALID` blocks same-origin framing] + expected: FAIL + + [`SAMEORIGIN,"DENY"` blocks same-origin framing] + expected: FAIL + [`INVALID;` allows same-origin framing] expected: if (os == "linux") and debug and fission and swgl: [PASS, TIMEOUT]