From 9b4e39e837af498599859c4a6687eb8bf9f8ad89 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Ritschard Date: Wed, 14 Jan 2015 11:27:35 +0100 Subject: [PATCH 1/2] Use constant-time comparison functions when checking signatures This limits the likeliness of timing attacks against the API. See http://codahale.com/a-lesson-in-timing-attacks/ for the full rationale. Conflicts: server/src/com/cloud/api/ApiServer.java server/src/com/cloud/user/AccountManagerImpl.java --- server/src/com/cloud/api/ApiServer.java | 4 ++- .../com/cloud/api/ConstantTimeComparator.java | 36 +++++++++++++++++++ .../cloud/servlet/ConsoleProxyServlet.java | 3 +- .../com/cloud/user/AccountManagerImpl.java | 4 ++- 4 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 server/src/com/cloud/api/ConstantTimeComparator.java diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index e60af3b7d50..357504fd9ed 100644 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -910,9 +910,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final SecretKeySpec keySpec = new SecretKeySpec(secretKey.getBytes(), "HmacSHA1"); mac.init(keySpec); mac.update(unsignedRequest.getBytes()); + final byte[] encryptedBytes = mac.doFinal(); final String computedSignature = Base64.encodeBase64String(encryptedBytes); - final boolean equalSig = signature.equals(computedSignature); + final boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); + if (!equalSig) { s_logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); } else { diff --git a/server/src/com/cloud/api/ConstantTimeComparator.java b/server/src/com/cloud/api/ConstantTimeComparator.java new file mode 100644 index 00000000000..4612eeeb5bb --- /dev/null +++ b/server/src/com/cloud/api/ConstantTimeComparator.java @@ -0,0 +1,36 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.api; + +public class ConstantTimeComparator { + + public static boolean compareBytes(byte[] b1, byte[] b2) { + if (b1.length != b2.length) { + return false; + } + + int result = 0; + for (int i = 0; i < b1.length; i++) { + result |= b1[i] ^ b2[i]; + } + return result == 0; + } + + public static boolean compareStrings(String s1, String s2) { + return compareBytes(s1.getBytes(), s2.getBytes()); + } +} diff --git a/server/src/com/cloud/servlet/ConsoleProxyServlet.java b/server/src/com/cloud/servlet/ConsoleProxyServlet.java index 8cbe82b9401..2e797923204 100644 --- a/server/src/com/cloud/servlet/ConsoleProxyServlet.java +++ b/server/src/com/cloud/servlet/ConsoleProxyServlet.java @@ -45,6 +45,7 @@ import com.google.gson.GsonBuilder; import org.apache.cloudstack.framework.security.keys.KeysManager; +import com.cloud.api.ConstantTimeComparator; import com.cloud.exception.PermissionDeniedException; import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor; @@ -659,7 +660,7 @@ public class ConsoleProxyServlet extends HttpServlet { mac.update(unsignedRequest.getBytes()); byte[] encryptedBytes = mac.doFinal(); String computedSignature = Base64.encodeBase64String(encryptedBytes); - boolean equalSig = signature.equals(computedSignature); + boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); if (!equalSig) { s_logger.debug("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); } diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 983a58a4662..36983cc2e4f 100644 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -63,6 +63,7 @@ import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.region.gslb.GlobalLoadBalancerRuleDao; import com.cloud.api.ApiDBUtils; +import com.cloud.api.ConstantTimeComparator; import com.cloud.api.query.vo.ControlledViewEntity; import com.cloud.configuration.Config; import com.cloud.configuration.ConfigurationManager; @@ -488,6 +489,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override public void checkAccess(Account caller, AccessType accessType, boolean sameOwner, String apiName, ControlledEntity... entities) { + //check for the same owner Long ownerId = null; ControlledEntity prevEntity = null; @@ -2061,7 +2063,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M mac.update(unsignedRequest.getBytes()); byte[] encryptedBytes = mac.doFinal(); String computedSignature = new String(Base64.encodeBase64(encryptedBytes)); - boolean equalSig = signature.equals(computedSignature); + boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); if (!equalSig) { s_logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); } else { From b2393c31ed8f689e45227f12371fc042c9dbd0e4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Ritschard Date: Wed, 14 Jan 2015 12:14:00 +0100 Subject: [PATCH 2/2] move ConstantTimeComparator to utils --- server/src/com/cloud/api/ApiServer.java | 1 + server/src/com/cloud/servlet/ConsoleProxyServlet.java | 2 +- server/src/com/cloud/user/AccountManagerImpl.java | 2 +- .../src/com/cloud/utils}/ConstantTimeComparator.java | 5 ++++- 4 files changed, 7 insertions(+), 3 deletions(-) rename {server/src/com/cloud/api => utils/src/com/cloud/utils}/ConstantTimeComparator.java (97%) diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index 357504fd9ed..daf24ef9d44 100644 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -40,6 +40,7 @@ import com.cloud.user.DomainManager; import com.cloud.user.User; import com.cloud.user.UserAccount; import com.cloud.user.UserVO; +import com.cloud.utils.ConstantTimeComparator; import com.cloud.utils.HttpUtils; import com.cloud.utils.NumbersUtil; import com.cloud.utils.Pair; diff --git a/server/src/com/cloud/servlet/ConsoleProxyServlet.java b/server/src/com/cloud/servlet/ConsoleProxyServlet.java index 2e797923204..d08bcdb2760 100644 --- a/server/src/com/cloud/servlet/ConsoleProxyServlet.java +++ b/server/src/com/cloud/servlet/ConsoleProxyServlet.java @@ -45,7 +45,6 @@ import com.google.gson.GsonBuilder; import org.apache.cloudstack.framework.security.keys.KeysManager; -import com.cloud.api.ConstantTimeComparator; import com.cloud.exception.PermissionDeniedException; import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor; @@ -55,6 +54,7 @@ import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.User; import com.cloud.uservm.UserVm; +import com.cloud.utils.ConstantTimeComparator; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.db.EntityManager; diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 36983cc2e4f..a681c902ece 100644 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -63,7 +63,6 @@ import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.region.gslb.GlobalLoadBalancerRuleDao; import com.cloud.api.ApiDBUtils; -import com.cloud.api.ConstantTimeComparator; import com.cloud.api.query.vo.ControlledViewEntity; import com.cloud.configuration.Config; import com.cloud.configuration.ConfigurationManager; @@ -136,6 +135,7 @@ import com.cloud.user.Account.State; import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.UserAccountDao; import com.cloud.user.dao.UserDao; +import com.cloud.utils.ConstantTimeComparator; import com.cloud.utils.NumbersUtil; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; diff --git a/server/src/com/cloud/api/ConstantTimeComparator.java b/utils/src/com/cloud/utils/ConstantTimeComparator.java similarity index 97% rename from server/src/com/cloud/api/ConstantTimeComparator.java rename to utils/src/com/cloud/utils/ConstantTimeComparator.java index 4612eeeb5bb..4d4a595309a 100644 --- a/server/src/com/cloud/api/ConstantTimeComparator.java +++ b/utils/src/com/cloud/utils/ConstantTimeComparator.java @@ -1,3 +1,4 @@ +// // Licensed to the Apache Software Foundation (ASF) under one // or more contributor license agreements. See the NOTICE file // distributed with this work for additional information @@ -14,7 +15,9 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -package com.cloud.api; +// + +package com.cloud.utils; public class ConstantTimeComparator {