WebCrypto HMAC verification uses a non-constant-time memcmp
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Feb 2014 23:31:54 +0000 (23:31 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Feb 2014 23:31:54 +0000 (23:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=128198
<rdar://problem/15976961>

Reviewed by Oliver Hunt.

Source/WebCore:

* crypto/mac/CryptoAlgorithmHMACMac.cpp: (WebCore::CryptoAlgorithmHMAC::platformVerify):
Use a constant time memcmp.

Source/WTF:

Added a "constant time" memcmp that doesn't depend on data. The name is somewhat
strange, as the function is of course O(length) and not constant time at all,
but apparently this is how everyone calls such functions.

* GNUmakefile.list.am:
* WTF.vcxproj/WTF.vcxproj:
* WTF.vcxproj/WTF.vcxproj.filters:
* WTF.xcodeproj/project.pbxproj:
* wtf/CMakeLists.txt:
* wtf/CryptographicUtilities.cpp: Added. (WTF::constantTimeMemcmp):
* wtf/CryptographicUtilities.h: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@163407 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/GNUmakefile.list.am
Source/WTF/WTF.vcxproj/WTF.vcxproj
Source/WTF/WTF.vcxproj/WTF.vcxproj.filters
Source/WTF/WTF.xcodeproj/project.pbxproj
Source/WTF/wtf/CMakeLists.txt
Source/WTF/wtf/CryptographicUtilities.cpp [new file with mode: 0644]
Source/WTF/wtf/CryptographicUtilities.h [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/crypto/mac/CryptoAlgorithmHMACMac.cpp

index 62f85d81f36a3918041e85b84dca4655088424f7..4ca6d43e80fc550b6caa78cee03ef261f61e11fa 100644 (file)
@@ -1,3 +1,23 @@
+2014-02-04  Alexey Proskuryakov  <ap@apple.com>
+
+        WebCrypto HMAC verification uses a non-constant-time memcmp
+        https://bugs.webkit.org/show_bug.cgi?id=128198
+        <rdar://problem/15976961>
+
+        Reviewed by Oliver Hunt.
+
+        Added a "constant time" memcmp that doesn't depend on data. The name is somewhat
+        strange, as the function is of course O(length) and not constant time at all,
+        but apparently this is how everyone calls such functions.
+
+        * GNUmakefile.list.am:
+        * WTF.vcxproj/WTF.vcxproj:
+        * WTF.vcxproj/WTF.vcxproj.filters:
+        * WTF.xcodeproj/project.pbxproj:
+        * wtf/CMakeLists.txt:
+        * wtf/CryptographicUtilities.cpp: Added. (WTF::constantTimeMemcmp):
+        * wtf/CryptographicUtilities.h: Added.
+
 2014-02-04  Anders Carlsson  <andersca@apple.com>
 
         Rename StringImpl::getCharacters to StringImpl::characters
index 237ab98e1623f2d615a8a2db13bad1e56c1d0cd2..b670fddb066bc08096621ad25e3679e1751886e8 100644 (file)
@@ -27,6 +27,8 @@ wtf_sources += \
     Source/WTF/wtf/CompilationThread.h \
     Source/WTF/wtf/Compression.h \
     Source/WTF/wtf/Compression.cpp \
+    Source/WTF/wtf/CryptographicUtilities.cpp \
+    Source/WTF/wtf/CryptographicUtilities.h \
     Source/WTF/wtf/CryptographicallyRandomNumber.cpp \
     Source/WTF/wtf/CryptographicallyRandomNumber.h \
     Source/WTF/wtf/CurrentTime.cpp \
index 440afe1d645f52b773f9e1d656aff450a778567b..fb9408c922b485baa643a362e173bc198490ace7 100644 (file)
@@ -55,6 +55,7 @@
     <ClCompile Include="..\wtf\BitVector.cpp" />
     <ClCompile Include="..\wtf\CompilationThread.cpp" />
     <ClCompile Include="..\wtf\Compression.cpp" />
+    <ClCompile Include="..\wtf\CryptographicUtilities.cpp" />
     <ClCompile Include="..\wtf\CryptographicallyRandomNumber.cpp" />
     <ClCompile Include="..\wtf\CurrentTime.cpp" />
     <ClCompile Include="..\wtf\DataLog.cpp" />
     <ClInclude Include="..\wtf\CheckedBoolean.h" />
     <ClInclude Include="..\wtf\Compiler.h" />
     <ClInclude Include="..\wtf\Compression.h" />
+    <ClInclude Include="..\wtf\CryptographicUtilities.h" />
     <ClInclude Include="..\wtf\CryptographicallyRandomNumber.h" />
     <ClInclude Include="..\wtf\CurrentTime.h" />
     <ClInclude Include="..\wtf\DataLog.h" />
index 2241a85b7d5be375c6377dae760e255e10d55110..1178a477e8b1a46f162642b13af248846121f9db 100644 (file)
     <ClCompile Include="..\wtf\BitVector.cpp">
       <Filter>wtf</Filter>
     </ClCompile>
+    <ClCompile Include="..\wtf\CryptographicUtilities.cpp">
+      <Filter>wtf</Filter>
+    </ClCompile>
     <ClCompile Include="..\wtf\CryptographicallyRandomNumber.cpp">
       <Filter>wtf</Filter>
     </ClCompile>
     <ClInclude Include="..\wtf\Compiler.h">
       <Filter>wtf</Filter>
     </ClInclude>
+    <ClInclude Include="..\wtf\CryptographicUtilities.h">
+      <Filter>wtf</Filter>
+    </ClInclude>
     <ClInclude Include="..\wtf\CryptographicallyRandomNumber.h">
       <Filter>wtf</Filter>
     </ClInclude>
index fe2755c717d126c70593c5317985c1bcb3e30e7f..1c49a010e264add72e3931d9ec7be24d5bfc1a02 100644 (file)
                B38FD7BD168953E80065C969 /* FeatureDefines.h in Headers */ = {isa = PBXBuildFile; fileRef = B38FD7BC168953E80065C969 /* FeatureDefines.h */; };
                CD5497AC15857D0300B5BC30 /* MediaTime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CD5497AA15857D0300B5BC30 /* MediaTime.cpp */; };
                CD5497AD15857D0300B5BC30 /* MediaTime.h in Headers */ = {isa = PBXBuildFile; fileRef = CD5497AB15857D0300B5BC30 /* MediaTime.h */; };
+               E15556F518A0CC18006F48FB /* CryptographicUtilities.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E15556F318A0CC18006F48FB /* CryptographicUtilities.cpp */; };
+               E15556F618A0CC18006F48FB /* CryptographicUtilities.h in Headers */ = {isa = PBXBuildFile; fileRef = E15556F418A0CC18006F48FB /* CryptographicUtilities.h */; };
                EB95E1F0161A72410089A2F5 /* ByteOrder.h in Headers */ = {isa = PBXBuildFile; fileRef = EB95E1EF161A72410089A2F5 /* ByteOrder.h */; };
                FEDACD3D1630F83F00C69634 /* StackStats.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FEDACD3B1630F83F00C69634 /* StackStats.cpp */; };
                FEDACD3E1630F83F00C69634 /* StackStats.h in Headers */ = {isa = PBXBuildFile; fileRef = FEDACD3C1630F83F00C69634 /* StackStats.h */; };
                B38FD7BC168953E80065C969 /* FeatureDefines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = FeatureDefines.h; sourceTree = "<group>"; };
                CD5497AA15857D0300B5BC30 /* MediaTime.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MediaTime.cpp; sourceTree = "<group>"; };
                CD5497AB15857D0300B5BC30 /* MediaTime.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MediaTime.h; sourceTree = "<group>"; };
+               E15556F318A0CC18006F48FB /* CryptographicUtilities.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CryptographicUtilities.cpp; sourceTree = "<group>"; };
+               E15556F418A0CC18006F48FB /* CryptographicUtilities.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CryptographicUtilities.h; sourceTree = "<group>"; };
                EB95E1EF161A72410089A2F5 /* ByteOrder.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ByteOrder.h; sourceTree = "<group>"; };
                FEDACD3B1630F83F00C69634 /* StackStats.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = StackStats.cpp; sourceTree = "<group>"; };
                FEDACD3C1630F83F00C69634 /* StackStats.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StackStats.h; sourceTree = "<group>"; };
                                A8A47270151A825A004123FF /* Compiler.h */,
                                A7E643C417C5423B003BB16B /* Compression.cpp */,
                                A7E643C517C5423B003BB16B /* Compression.h */,
+                               E15556F318A0CC18006F48FB /* CryptographicUtilities.cpp */,
+                               E15556F418A0CC18006F48FB /* CryptographicUtilities.h */,
                                A8A47273151A825A004123FF /* CryptographicallyRandomNumber.cpp */,
                                A8A47274151A825A004123FF /* CryptographicallyRandomNumber.h */,
                                A8A47275151A825A004123FF /* CurrentTime.cpp */,
                                A8A4742E151A825B004123FF /* TCPackedCache.h in Headers */,
                                A8A4742F151A825B004123FF /* TCPageMap.h in Headers */,
                                A8A47430151A825B004123FF /* TCSpinLock.h in Headers */,
+                               E15556F618A0CC18006F48FB /* CryptographicUtilities.h in Headers */,
                                A8A47432151A825B004123FF /* TCSystemAlloc.h in Headers */,
                                1A6EB1E0187D0BD30030126F /* StringView.h in Headers */,
                                A8A47433151A825B004123FF /* TemporaryChange.h in Headers */,
                                A8A473C3151A825B004123FF /* FastMalloc.cpp in Sources */,
                                A5BA15FC182435A600A82E69 /* StringImplCF.cpp in Sources */,
                                A5BA15FA182435A600A82E69 /* AtomicStringCF.cpp in Sources */,
+                               E15556F518A0CC18006F48FB /* CryptographicUtilities.cpp in Sources */,
                                0F9D3360165DBA73005AD387 /* FilePrintStream.cpp in Sources */,
                                A8A473B5151A825B004123FF /* fixed-dtoa.cc in Sources */,
                                1A1D8B9E1731879800141DA4 /* FunctionDispatcher.cpp in Sources */,
index 35ba4e46f1926dd0ce3088bb1377ccabbee5b118..695af6a8cbb2f16147ce3c2b4141abf18547719c 100644 (file)
@@ -13,6 +13,7 @@ set(WTF_HEADERS
     CompilationThread.h
     Compiler.h
     Compression.h
+    CryptographicUtilities.h
     CryptographicallyRandomNumber.h
     CurrentTime.h
     DataLog.h
@@ -150,6 +151,7 @@ set(WTF_SOURCES
     BitVector.cpp
     CompilationThread.cpp
     Compression.cpp
+    CryptographicUtilities.cpp
     CryptographicallyRandomNumber.cpp
     CurrentTime.cpp
     DataLog.cpp
diff --git a/Source/WTF/wtf/CryptographicUtilities.cpp b/Source/WTF/wtf/CryptographicUtilities.cpp
new file mode 100644 (file)
index 0000000..bae227e
--- /dev/null
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2008 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+
+#include "config.h"
+#include "CryptographicUtilities.h"
+
+namespace WTF {
+
+// FIXME: Use platform APIs where available. See <rdar://problem/12685603> for Mac/iOS.
+NEVER_INLINE int constantTimeMemcmp(const void* voidA, const void* voidB, size_t length)
+{
+    const uint8_t* a = static_cast<const uint8_t*>(voidA);
+    const uint8_t* b = static_cast<const uint8_t*>(voidB);
+
+    uint8_t result = 0;
+    for (size_t i = 0; i < length; ++i)
+        result |= a[i] ^ b[i];
+
+    return result;
+}
+
+}
diff --git a/Source/WTF/wtf/CryptographicUtilities.h b/Source/WTF/wtf/CryptographicUtilities.h
new file mode 100644 (file)
index 0000000..452a4a8
--- /dev/null
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2014 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+
+#ifndef WTF_CryptographicUtilities_h
+#define WTF_CryptographicUtilities_h
+
+namespace WTF {
+
+// Returns zero if arrays are equal, and non-zero otherwise. Execution time does not depend on array contents.
+WTF_EXPORT_PRIVATE int constantTimeMemcmp(const void*, const void*, size_t length);
+
+}
+
+using WTF::constantTimeMemcmp;
+
+#endif
index b9fe72fe9226787430405b2b62cded5a2ce24b90..4f90d8ff7ecb6f55db9abc90d9355773992e2c17 100644 (file)
@@ -1,3 +1,14 @@
+2014-02-04  Alexey Proskuryakov  <ap@apple.com>
+
+        WebCrypto HMAC verification uses a non-constant-time memcmp
+        https://bugs.webkit.org/show_bug.cgi?id=128198
+        <rdar://problem/15976961>
+
+        Reviewed by Oliver Hunt.
+
+        * crypto/mac/CryptoAlgorithmHMACMac.cpp: (WebCore::CryptoAlgorithmHMAC::platformVerify):
+        Use a constant time memcmp.
+
 2014-02-04  Simon Fraser  <simon.fraser@apple.com>
 
         Add WK2 event handling path for iOS, and make Mac and iOS code more similar
index 5cecf53e9a18d5fc56f444ddcf97df3157477aae..f51055216332b7b99173bd6409d5944da02be09e 100644 (file)
@@ -32,6 +32,7 @@
 #include "CryptoKeyHMAC.h"
 #include "ExceptionCode.h"
 #include <CommonCrypto/CommonHMAC.h>
+#include <wtf/CryptographicUtilities.h>
 
 namespace WebCore {
 
@@ -111,7 +112,8 @@ void CryptoAlgorithmHMAC::platformVerify(const CryptoAlgorithmHmacParams& parame
 
     Vector<uint8_t> signature = calculateSignature(algorithm, key.key(), data);
 
-    bool result = signature.size() == expectedSignature.second && !memcmp(signature.data(), expectedSignature.first, signature.size());
+    // Using a constant time comparison to prevent timing attacks.
+    bool result = signature.size() == expectedSignature.second && !constantTimeMemcmp(signature.data(), expectedSignature.first, signature.size());
 
     callback(result);
 }