From 1bb8dfe2ce701fdf4bc1e22ae3a209053ec0534e Mon Sep 17 00:00:00 2001 From: Tanvi Vyas Date: Mon, 15 Dec 2014 17:40:43 -0800 Subject: [PATCH] =?UTF-8?q?Bug=201082837=20-=20Track=20insecure=20redirect?= =?UTF-8?q?s=20on=20imgRequest.=20r=3D=3F?= --- image/src/imgLoader.cpp | 38 +++++++++++++++++++++++++++++++------- image/src/imgLoader.h | 2 ++ image/src/imgRequest.cpp | 40 ++++++++++++++++++++++++++++++++++++++-- image/src/imgRequest.h | 6 ++++++ 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/image/src/imgLoader.cpp b/image/src/imgLoader.cpp index 96971c0846ce..56a96567446e 100644 --- a/image/src/imgLoader.cpp +++ b/image/src/imgLoader.cpp @@ -2038,7 +2038,8 @@ nsresult imgLoader::LoadImage(nsIURI *aURI, nsCOMPtr channelLoadGroup; newChannel->GetLoadGroup(getter_AddRefs(channelLoadGroup)); - request->Init(aURI, aURI, channelLoadGroup, newChannel, entry, aCX, + request->Init(aURI, aURI, /* aHadInsecureRedirect = */ false, + channelLoadGroup, newChannel, entry, aCX, aLoadingPrincipal, corsmode, aReferrerPolicy); // Add the initiator type for this image load @@ -2266,8 +2267,16 @@ nsresult imgLoader::LoadImageWithChannel(nsIChannel *channel, imgINotificationOb channel->GetOriginalURI(getter_AddRefs(originalURI)); // No principal specified here, because we're not passed one. - request->Init(originalURI, uri, channel, channel, entry, - aCX, nullptr, imgIRequest::CORS_NONE, RP_Default); + // In LoadImageWithChannel, the redirects that may have been + // assoicated with this load would have gone through necko. + // We only have the final URI in ImageLib and hence don't know + // if the request went through insecure redirects. But if it did, + // the necko cache should have handled that (since all necko cache hits + // including the redirects will go through content policy). Hence, we + // can set aHadInsecureRedirect to false here. + request->Init(originalURI, uri, /* aHadInsecureRedirect = */ false, + channel, channel, entry, aCX, nullptr, + imgIRequest::CORS_NONE, RP_Default); nsRefPtr pl = new ProxyListener(static_cast(request.get())); @@ -2515,7 +2524,8 @@ imgCacheValidator::imgCacheValidator(nsProgressNotificationProxy* progress, : mProgressProxy(progress), mRequest(request), mContext(aContext), - mImgLoader(loader) + mImgLoader(loader), + mHadInsecureRedirect(false) { NewRequestAndEntry(forcePrincipalCheckForCacheEntry, loader, getter_AddRefs(mNewRequest), getter_AddRefs(mNewEntry)); @@ -2622,9 +2632,8 @@ NS_IMETHODIMP imgCacheValidator::OnStartRequest(nsIRequest *aRequest, nsISupport // We use originalURI here to fulfil the imgIRequest contract on GetURI. nsCOMPtr originalURI; channel->GetOriginalURI(getter_AddRefs(originalURI)); - mNewRequest->Init(originalURI, uri, aRequest, channel, mNewEntry, - mContext, loadingPrincipal, - corsmode, refpol); + mNewRequest->Init(originalURI, uri, mHadInsecureRedirect, aRequest, channel, + mNewEntry, mContext, loadingPrincipal, corsmode, refpol); mDestListener = new ProxyListener(mNewRequest); @@ -2718,6 +2727,21 @@ NS_IMETHODIMP imgCacheValidator::AsyncOnChannelRedirect(nsIChannel *oldChannel, // Note all cache information we get from the old channel. mNewRequest->SetCacheValidation(mNewEntry, oldChannel); + // If the previous URI is a non-HTTPS URI, record that fact for later use by + // security code, which needs to know whether there is an insecure load at any + // point in the redirect chain. + nsCOMPtr oldURI; + bool isHttps = false; + bool isChrome = false; + bool schemeLocal = false; + if (NS_FAILED(oldChannel->GetURI(getter_AddRefs(oldURI))) || + NS_FAILED(oldURI->SchemeIs("https", &isHttps)) || + NS_FAILED(oldURI->SchemeIs("chrome", &isChrome)) || + NS_FAILED(NS_URIChainHasFlags(oldURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal)) || + (!isHttps && !isChrome && !schemeLocal)) { + mHadInsecureRedirect = true; + } + // Prepare for callback mRedirectCallback = callback; mRedirectChannel = newChannel; diff --git a/image/src/imgLoader.h b/image/src/imgLoader.h index fdbc6ee21962..66d1d8b90dd5 100644 --- a/image/src/imgLoader.h +++ b/image/src/imgLoader.h @@ -527,6 +527,8 @@ private: nsCOMPtr mContext; imgLoader* mImgLoader; + + bool mHadInsecureRedirect; }; #endif // imgLoader_h__ diff --git a/image/src/imgRequest.cpp b/image/src/imgRequest.cpp index a8881df3d4d5..5c453cadf735 100644 --- a/image/src/imgRequest.cpp +++ b/image/src/imgRequest.cpp @@ -77,6 +77,7 @@ imgRequest::imgRequest(imgLoader* aLoader) , mIsInCache(false) , mDecodeRequested(false) , mNewPartPending(false) + , mHadInsecureRedirect(false) { } imgRequest::~imgRequest() @@ -94,6 +95,7 @@ imgRequest::~imgRequest() nsresult imgRequest::Init(nsIURI *aURI, nsIURI *aCurrentURI, + bool aHadInsecureRedirect, nsIRequest *aRequest, nsIChannel *aChannel, imgCacheEntry *aCacheEntry, @@ -125,6 +127,26 @@ nsresult imgRequest::Init(nsIURI *aURI, mCORSMode = aCORSMode; mReferrerPolicy = aReferrerPolicy; + // If the original URI and the current URI are different, check whether the + // original URI is secure. We deliberately don't take the current URI into + // account, as it needs to be handled using more complicated rules than + // earlier elements of the redirect chain. + if (aURI != aCurrentURI) { + bool isHttps = false; + bool isChrome = false; + bool schemeLocal = false; + if (NS_FAILED(aURI->SchemeIs("https", &isHttps)) || + NS_FAILED(aURI->SchemeIs("chrome", &isChrome)) || + NS_FAILED(NS_URIChainHasFlags(aURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal)) || + (!isHttps && !isChrome && !schemeLocal)) { + mHadInsecureRedirect = true; + } + } + + // imgCacheValidator may have handled redirects before we were created, so we + // allow the caller to let us know if any redirects were insecure. + mHadInsecureRedirect = mHadInsecureRedirect || aHadInsecureRedirect; + mChannel->GetNotificationCallbacks(getter_AddRefs(mPrevChannelSink)); NS_ASSERTION(mPrevChannelSink != this, @@ -1153,8 +1175,20 @@ imgRequest::OnRedirectVerifyCallback(nsresult result) LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnChannelRedirect", "old", spec.get()); } - // make sure we have a protocol that returns data rather than opens - // an external application, e.g. mailto: + // If the previous URI is a non-HTTPS URI, record that fact for later use by + // security code, which needs to know whether there is an insecure load at any + // point in the redirect chain. + bool isHttps = false; + bool isChrome = false; + bool schemeLocal = false; + if (NS_FAILED(mCurrentURI->SchemeIs("https", &isHttps)) || + NS_FAILED(mCurrentURI->SchemeIs("chrome", &isChrome)) || + NS_FAILED(NS_URIChainHasFlags(mCurrentURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal)) || + (!isHttps && !isChrome && !schemeLocal)) { + mHadInsecureRedirect = true; + } + + // Update the current URI. mChannel->GetURI(getter_AddRefs(mCurrentURI)); if (LOG_TEST(PR_LOG_DEBUG)) { @@ -1164,6 +1198,8 @@ imgRequest::OnRedirectVerifyCallback(nsresult result) LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnChannelRedirect", "new", spec.get()); } + // Make sure we have a protocol that returns data rather than opens an + // external application, e.g. 'mailto:'. bool doesNotReturnData = false; nsresult rv = NS_URIChainHasFlags(mCurrentURI, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, diff --git a/image/src/imgRequest.h b/image/src/imgRequest.h index 650b462e86a3..c3bf3a155b80 100644 --- a/image/src/imgRequest.h +++ b/image/src/imgRequest.h @@ -66,6 +66,7 @@ public: nsresult Init(nsIURI *aURI, nsIURI *aCurrentURI, + bool aHadInsecureRedirect, nsIRequest *aRequest, nsIChannel *aChannel, imgCacheEntry *aCacheEntry, @@ -113,6 +114,10 @@ public: bool GetMultipart() const; + // Returns whether we went through an insecure (non-HTTPS) redirect at some + // point during loading. This does not consider the current URI. + bool HadInsecureRedirect() const { return mHadInsecureRedirect; } + // The CORS mode for which we loaded this image. int32_t GetCORSMode() const { return mCORSMode; } @@ -268,6 +273,7 @@ private: bool mIsInCache : 1; bool mDecodeRequested : 1; bool mNewPartPending : 1; + bool mHadInsecureRedirect : 1; }; #endif