Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(149)

Unified Diff: Source/core/frame/SubresourceIntegrity.cpp

Issue 596043003: Basic console error messages for subresource integrity. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Patch Set: Addressed comments from mkwst Created 6 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « Source/core/dom/ScriptLoader.cpp ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: Source/core/frame/SubresourceIntegrity.cpp
diff --git a/Source/core/frame/SubresourceIntegrity.cpp b/Source/core/frame/SubresourceIntegrity.cpp
index 6563810284be7a97f344cbc7c534ed517df70f76..a449653d826765f44153558af6d28bdf2f06af1e 100644
--- a/Source/core/frame/SubresourceIntegrity.cpp
+++ b/Source/core/frame/SubresourceIntegrity.cpp
@@ -8,7 +8,9 @@
#include "core/HTMLNames.h"
#include "core/dom/Document.h"
#include "core/dom/Element.h"
+#include "core/frame/ConsoleTypes.h"
#include "core/frame/UseCounter.h"
+#include "core/inspector/ConsoleMessage.h"
#include "platform/Crypto.h"
#include "platform/ParsingUtilities.h"
#include "platform/RuntimeEnabledFeatures.h"
@@ -30,6 +32,11 @@ static bool isIntegrityCharacter(UChar c)
return isASCIIAlphanumeric(c) || c == '+' || c == '/' || c == '=';
}
+static void logErrorToConsole(const String& message, Document& document)
+{
+ document.addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, ErrorMessageLevel, message));
+}
+
static bool DigestsEqual(const DigestValue& digest1, const DigestValue& digest2)
{
if (digest1.size() != digest2.size())
@@ -43,8 +50,35 @@ static bool DigestsEqual(const DigestValue& digest1, const DigestValue& digest2)
return true;
}
-// FIXME: If CheckSubresourceIntegrity fails, Blink should create a console
-// message to alert the developer of the failure.
+static String algorithmToString(HashAlgorithm algorithm)
+{
+ static const struct {
+ HashAlgorithm algorithm;
+ const char* name;
+ } kAlgorithmToString[] = {
+ { HashAlgorithmSha256, "SHA-256" },
+ { HashAlgorithmSha384, "SHA-384" },
+ { HashAlgorithmSha512, "SHA-512" }
+ };
+
+ // See comment in parseIntegrityAttribute about why sizeof() is used
+ // instead of WTF_ARRAY_LENGTH.
+ size_t i = 0;
+ size_t kSupportedAlgorithmsLength = sizeof(kAlgorithmToString) / sizeof(kAlgorithmToString[0]);
+ for (; i < kSupportedAlgorithmsLength; i++) {
+ if (kAlgorithmToString[i].algorithm == algorithm)
+ return kAlgorithmToString[i].name;
+ }
+
+ ASSERT_NOT_REACHED();
+ return String();
+}
+
+static String digestToString(const DigestValue& digest)
+{
+ return base64Encode(reinterpret_cast<const char*>(digest.data()), digest.size(), Base64DoNotInsertLFs);
+}
+
bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, const String& source, const KURL& resourceUrl)
{
if (!RuntimeEnabledFeatures::subresourceIntegrityEnabled())
@@ -53,9 +87,8 @@ bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, con
if (!element.fastHasAttribute(HTMLNames::integrityAttr))
return true;
- // FIXME: If insecureOriginMsg is not empty after the check, Blink
- // should send a console message.
- //
+ Document& document = element.document();
+
// Instead of just checking SecurityOrigin::isSecure on resourceUrl, this
// checks canAccessFeatureRequiringSecureOrigin so that file:// protocols
// and localhost resources can be allowed. These may be useful for testing
@@ -63,19 +96,25 @@ bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, con
// allows them here.
String insecureOriginMsg = "";
RefPtr<SecurityOrigin> resourceSecurityOrigin = SecurityOrigin::create(resourceUrl);
- if (!element.document().securityOrigin()->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)) {
- UseCounter::count(element.document(), UseCounter::SRIElementWithIntegrityAttributeAndInsecureOrigin);
+ if (!document.securityOrigin()->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)) {
+ UseCounter::count(document, UseCounter::SRIElementWithIntegrityAttributeAndInsecureOrigin);
+ // FIXME: This console message should probably utilize
+ // inesecureOriginMsg to give a more helpful message to the user.
+ logErrorToConsole("The 'integrity' attribute may only be used in documents in secure origins.", document);
return false;
}
if (!resourceSecurityOrigin->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)) {
- UseCounter::count(element.document(), UseCounter::SRIElementWithIntegrityAttributeAndInsecureResource);
+ UseCounter::count(document, UseCounter::SRIElementWithIntegrityAttributeAndInsecureResource);
+ logErrorToConsole("The 'integrity' attribute may only be used with resources on secure origins.", document);
return false;
}
String integrity;
HashAlgorithm algorithm;
- if (!parseIntegrityAttribute(element.fastGetAttribute(HTMLNames::integrityAttr), integrity, algorithm)) {
- UseCounter::count(element.document(), UseCounter::SRIElementWithUnparsableIntegrityAttribute);
+ String attribute = element.fastGetAttribute(HTMLNames::integrityAttr);
+ if (!parseIntegrityAttribute(attribute, integrity, algorithm)) {
+ UseCounter::count(document, UseCounter::SRIElementWithUnparsableIntegrityAttribute);
+ logErrorToConsole("The 'integrity' attribute's value '" + attribute + "' is not valid integrity metadat.", document);
Mike West 2014/09/30 09:13:28 s/metadat/metadata/.
jww 2014/09/30 17:59:33 Done.
return false;
}
@@ -91,12 +130,21 @@ bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, con
DigestValue convertedHashVector;
convertedHashVector.append(reinterpret_cast<uint8_t*>(hashVector.data()), hashVector.size());
if (DigestsEqual(digest, convertedHashVector)) {
- UseCounter::count(element.document(), UseCounter::SRIElementWithMatchingIntegrityAttribute);
+ UseCounter::count(document, UseCounter::SRIElementWithMatchingIntegrityAttribute);
return true;
+ } else {
+ // This message exposes the digest of the resource to the console.
+ // Because this is only to the console, that's okay for now, but we
+ // need to be very carefully not to expose this in exceptions or
Mike West 2014/09/30 09:13:28 s/carefully/careful/
jww 2014/09/30 17:59:33 Done.
+ // JavaScript, otherwise it risks exposing information about the
+ // resource cross-origin.
+ logErrorToConsole("The computed " + algorithmToString(algorithm) + " integrity '" + digestToString(digest) + "' does not match the 'integrity' attribute '" + integrity + "' for resource '" + resourceUrl.elidedString() + "'.", document);
}
+ } else {
+ logErrorToConsole("There was an error computing an 'integrity' value for resource '" + resourceUrl.elidedString() + "'.", document);
}
- UseCounter::count(element.document(), UseCounter::SRIElementWithNonMatchingIntegrityAttribute);
+ UseCounter::count(document, UseCounter::SRIElementWithNonMatchingIntegrityAttribute);
return false;
}
@@ -104,7 +152,8 @@ bool SubresourceIntegrity::parseIntegrityAttribute(const String& attribute, Stri
{
DEFINE_STATIC_LOCAL(const String, integrityPrefix, ("ni://"));
// Any additions or subtractions from this struct should also modify the
- // respective entries in the kAlgorithmMap array in checkDigest().
+ // respective entries in the kAlgorithmMap array in checkDigest() as well
+ // as the array in algorithmToString().
static const struct {
const char* prefix;
HashAlgorithm algorithm;
« no previous file with comments | « Source/core/dom/ScriptLoader.cpp ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698