From b453703aa83f9e3b1605190aed3356fec9d46155 Mon Sep 17 00:00:00 2001 From: Rodrigo Andrade Date: Mon, 15 Aug 2016 18:20:28 -0300 Subject: removing duplicated code for cookie genaration and adding random bytes to generate user cookies --- src/main/java/com/gitblit/ConfigUserService.java | 2 +- src/main/java/com/gitblit/auth/AuthenticationProvider.java | 2 +- src/main/java/com/gitblit/client/EditUserDialog.java | 2 +- src/main/java/com/gitblit/models/UserModel.java | 4 ++++ src/main/java/com/gitblit/wicket/pages/EditUserPage.java | 2 +- 5 files changed, 8 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/main/java/com/gitblit/ConfigUserService.java b/src/main/java/com/gitblit/ConfigUserService.java index 6d7230f7..025b1d8c 100644 --- a/src/main/java/com/gitblit/ConfigUserService.java +++ b/src/main/java/com/gitblit/ConfigUserService.java @@ -898,7 +898,7 @@ public class ConfigUserService implements IUserService { user.countryCode = config.getString(USER, username, COUNTRYCODE); user.cookie = config.getString(USER, username, COOKIE); if (StringUtils.isEmpty(user.cookie) && !StringUtils.isEmpty(user.password)) { - user.cookie = StringUtils.getSHA1(user.username + user.password); + user.cookie = user.createCookie(); } // preferences diff --git a/src/main/java/com/gitblit/auth/AuthenticationProvider.java b/src/main/java/com/gitblit/auth/AuthenticationProvider.java index 0bfe2351..6c098859 100644 --- a/src/main/java/com/gitblit/auth/AuthenticationProvider.java +++ b/src/main/java/com/gitblit/auth/AuthenticationProvider.java @@ -81,7 +81,7 @@ public abstract class AuthenticationProvider { protected void setCookie(UserModel user, char [] password) { // create a user cookie if (StringUtils.isEmpty(user.cookie) && !ArrayUtils.isEmpty(password)) { - user.cookie = StringUtils.getSHA1(user.username + new String(password)); + user.cookie = user.createCookie(); } } diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index 676916b2..4b01ff04 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -330,7 +330,7 @@ public class EditUserDialog extends JDialog { } // change the cookie - user.cookie = StringUtils.getSHA1(user.username + password); + user.cookie = user.createCookie(); String type = settings.get(Keys.realm.passwordStorage).getString("md5"); if (type.equalsIgnoreCase("md5")) { diff --git a/src/main/java/com/gitblit/models/UserModel.java b/src/main/java/com/gitblit/models/UserModel.java index e1522748..d411e504 100644 --- a/src/main/java/com/gitblit/models/UserModel.java +++ b/src/main/java/com/gitblit/models/UserModel.java @@ -660,4 +660,8 @@ public class UserModel implements Principal, Serializable, Comparable String projectPath = StringUtils.getFirstPathElement(repository); return !StringUtils.isEmpty(projectPath) && projectPath.equalsIgnoreCase(getPersonalPath()); } + + public String createCookie() { + return StringUtils.getSHA1(String.valueOf(Math.random())); + } } diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java index 220bee3f..72dee6b6 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java +++ b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java @@ -156,7 +156,7 @@ public class EditUserPage extends RootSubPage { } // change the cookie - userModel.cookie = StringUtils.getSHA1(userModel.username + password); + userModel.cookie = userModel.createCookie(); // Optionally store the password MD5 digest. String type = app().settings().getString(Keys.realm.passwordStorage, "md5"); -- cgit v1.2.3 From 4365c8f0b0410f540118868bbfc30f6974db3008 Mon Sep 17 00:00:00 2001 From: Rodrigo Andrade Date: Mon, 15 Aug 2016 18:24:24 -0300 Subject: removing unecessary user cookie code --- src/main/java/com/gitblit/auth/AuthenticationProvider.java | 4 ++-- src/main/java/com/gitblit/auth/HtpasswdAuthProvider.java | 2 +- src/main/java/com/gitblit/auth/LdapAuthProvider.java | 2 +- src/main/java/com/gitblit/auth/PAMAuthProvider.java | 2 +- src/main/java/com/gitblit/auth/RedmineAuthProvider.java | 2 +- src/main/java/com/gitblit/auth/SalesforceAuthProvider.java | 2 +- src/main/java/com/gitblit/auth/WindowsAuthProvider.java | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/main/java/com/gitblit/auth/AuthenticationProvider.java b/src/main/java/com/gitblit/auth/AuthenticationProvider.java index 6c098859..e359fd7e 100644 --- a/src/main/java/com/gitblit/auth/AuthenticationProvider.java +++ b/src/main/java/com/gitblit/auth/AuthenticationProvider.java @@ -78,9 +78,9 @@ public abstract class AuthenticationProvider { public abstract AuthenticationType getAuthenticationType(); - protected void setCookie(UserModel user, char [] password) { + protected void setCookie(UserModel user) { // create a user cookie - if (StringUtils.isEmpty(user.cookie) && !ArrayUtils.isEmpty(password)) { + if (StringUtils.isEmpty(user.cookie)) { user.cookie = user.createCookie(); } } diff --git a/src/main/java/com/gitblit/auth/HtpasswdAuthProvider.java b/src/main/java/com/gitblit/auth/HtpasswdAuthProvider.java index 2cdabf6f..3a6cb8ec 100644 --- a/src/main/java/com/gitblit/auth/HtpasswdAuthProvider.java +++ b/src/main/java/com/gitblit/auth/HtpasswdAuthProvider.java @@ -196,7 +196,7 @@ public class HtpasswdAuthProvider extends UsernamePasswordAuthenticationProvider } // create a user cookie - setCookie(user, password); + setCookie(user); // Set user attributes, hide password from backing user service. user.password = Constants.EXTERNAL_ACCOUNT; diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java index cc772e7b..b7efd4a0 100644 --- a/src/main/java/com/gitblit/auth/LdapAuthProvider.java +++ b/src/main/java/com/gitblit/auth/LdapAuthProvider.java @@ -360,7 +360,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider { } // create a user cookie - setCookie(user, password); + setCookie(user); if (!supportsTeamMembershipChanges()) { getTeamsFromLdap(ldapConnection, simpleUsername, loggingInUser, user); diff --git a/src/main/java/com/gitblit/auth/PAMAuthProvider.java b/src/main/java/com/gitblit/auth/PAMAuthProvider.java index 46f4dd6a..b38d49df 100644 --- a/src/main/java/com/gitblit/auth/PAMAuthProvider.java +++ b/src/main/java/com/gitblit/auth/PAMAuthProvider.java @@ -122,7 +122,7 @@ public class PAMAuthProvider extends UsernamePasswordAuthenticationProvider { } // create a user cookie - setCookie(user, password); + setCookie(user); // update user attributes from UnixUser user.accountType = getAccountType(); diff --git a/src/main/java/com/gitblit/auth/RedmineAuthProvider.java b/src/main/java/com/gitblit/auth/RedmineAuthProvider.java index 27cece29..364aff04 100644 --- a/src/main/java/com/gitblit/auth/RedmineAuthProvider.java +++ b/src/main/java/com/gitblit/auth/RedmineAuthProvider.java @@ -139,7 +139,7 @@ public class RedmineAuthProvider extends UsernamePasswordAuthenticationProvider } // create a user cookie - setCookie(user, password); + setCookie(user); // update user attributes from Redmine user.accountType = getAccountType(); diff --git a/src/main/java/com/gitblit/auth/SalesforceAuthProvider.java b/src/main/java/com/gitblit/auth/SalesforceAuthProvider.java index df033c27..79c3a0c4 100644 --- a/src/main/java/com/gitblit/auth/SalesforceAuthProvider.java +++ b/src/main/java/com/gitblit/auth/SalesforceAuthProvider.java @@ -66,7 +66,7 @@ public class SalesforceAuthProvider extends UsernamePasswordAuthenticationProvid user = new UserModel(simpleUsername); } - setCookie(user, password); + setCookie(user); setUserAttributes(user, info); updateUser(user); diff --git a/src/main/java/com/gitblit/auth/WindowsAuthProvider.java b/src/main/java/com/gitblit/auth/WindowsAuthProvider.java index aee51008..4c31fb15 100644 --- a/src/main/java/com/gitblit/auth/WindowsAuthProvider.java +++ b/src/main/java/com/gitblit/auth/WindowsAuthProvider.java @@ -153,7 +153,7 @@ public class WindowsAuthProvider extends UsernamePasswordAuthenticationProvider } // create a user cookie - setCookie(user, password); + setCookie(user); // update user attributes from Windows identity user.accountType = getAccountType(); -- cgit v1.2.3 From a1fc7e7228d7b8de05bc2cf074f112af757401d0 Mon Sep 17 00:00:00 2001 From: rcaa Date: Sun, 11 Dec 2016 19:12:27 -0300 Subject: changing Math.random to SecureRandom --- src/main/java/com/gitblit/models/UserModel.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/main/java/com/gitblit/models/UserModel.java b/src/main/java/com/gitblit/models/UserModel.java index d411e504..edbdf028 100644 --- a/src/main/java/com/gitblit/models/UserModel.java +++ b/src/main/java/com/gitblit/models/UserModel.java @@ -17,6 +17,7 @@ package com.gitblit.models; import java.io.Serializable; import java.security.Principal; +import java.security.SecureRandom; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -662,6 +663,9 @@ public class UserModel implements Principal, Serializable, Comparable } public String createCookie() { - return StringUtils.getSHA1(String.valueOf(Math.random())); + SecureRandom random = new SecureRandom(); + byte[] values = new byte[20]; + random.nextBytes(values); + return StringUtils.getSHA1(String.valueOf(values)); } } -- cgit v1.2.3 From 2be2c2c95c9a3747fd200e3ea3623607053d5299 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 10 Dec 2016 01:00:27 +0100 Subject: Introduce SecureRandom wrapper for properly seeded static instances Introduce our own wrapper `SecureRandom` around `java.security.SecureRandom`. This a) makes sure that the PRNG is seeded on creation and not when random bytes are retrieved, and b) uses a static instance in the `UserModel` so that lags do not occur during operation due to potentially seeding getting blocked on Unix when reading from the system's entropy pool. To keep the random data still secure, the static instance will reseed all 24 hours, also a functionality of the wrapper class. This fixes #1063 and extends and closes PR #1116 --- src/main/java/com/gitblit/models/UserModel.java | 10 +-- src/main/java/com/gitblit/utils/SecureRandom.java | 83 ++++++++++++++++++++++ .../java/com/gitblit/utils/SecureRandomTest.java | 33 +++++++++ 3 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 src/main/java/com/gitblit/utils/SecureRandom.java create mode 100644 src/test/java/com/gitblit/utils/SecureRandomTest.java (limited to 'src') diff --git a/src/main/java/com/gitblit/models/UserModel.java b/src/main/java/com/gitblit/models/UserModel.java index edbdf028..f8f7ed6d 100644 --- a/src/main/java/com/gitblit/models/UserModel.java +++ b/src/main/java/com/gitblit/models/UserModel.java @@ -37,6 +37,7 @@ import com.gitblit.Constants.PermissionType; import com.gitblit.Constants.RegistrantType; import com.gitblit.utils.ArrayUtils; import com.gitblit.utils.ModelUtils; +import com.gitblit.utils.SecureRandom; import com.gitblit.utils.StringUtils; /** @@ -53,6 +54,8 @@ public class UserModel implements Principal, Serializable, Comparable public static final UserModel ANONYMOUS = new UserModel(); + private static final SecureRandom RANDOM = new SecureRandom(); + // field names are reflectively mapped in EditUser page public String username; public String password; @@ -661,11 +664,8 @@ public class UserModel implements Principal, Serializable, Comparable String projectPath = StringUtils.getFirstPathElement(repository); return !StringUtils.isEmpty(projectPath) && projectPath.equalsIgnoreCase(getPersonalPath()); } - + public String createCookie() { - SecureRandom random = new SecureRandom(); - byte[] values = new byte[20]; - random.nextBytes(values); - return StringUtils.getSHA1(String.valueOf(values)); + return StringUtils.getSHA1(RANDOM.randomBytes(32)); } } diff --git a/src/main/java/com/gitblit/utils/SecureRandom.java b/src/main/java/com/gitblit/utils/SecureRandom.java new file mode 100644 index 00000000..119533d4 --- /dev/null +++ b/src/main/java/com/gitblit/utils/SecureRandom.java @@ -0,0 +1,83 @@ +/* + * Copyright 2016 gitblit.com + * + * Licensed 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.gitblit.utils; + +/** + * Wrapper class for java.security.SecureRandom, which will periodically reseed + * the PRNG in case an instance of the class has been running for a long time. + * + * @author Florian Zschocke + */ +public class SecureRandom { + + /** Period (in ms) after which a new SecureRandom will be created in order to get a fresh random seed. */ + private static final long RESEED_PERIOD = 24 * 60 * 60 * 1000; /* 24 hours */ + + + private long last; + private java.security.SecureRandom random; + + + + public SecureRandom() { + // Make sure the SecureRandom is seeded right from the start. + // This also lets any blocks during seeding occur at creation + // and prevents it from happening when getting next random bytes. + seed(); + } + + + + public byte[] randomBytes(int num) { + byte[] bytes = new byte[num]; + nextBytes(bytes); + return bytes; + } + + + public void nextBytes(byte[] bytes) { + random.nextBytes(bytes); + reseed(false); + } + + + void reseed(boolean forced) { + long ts = System.currentTimeMillis(); + if (forced || (ts - last) > RESEED_PERIOD) { + last = ts; + runReseed(); + } + } + + + + private void seed() { + random = new java.security.SecureRandom(); + random.nextBytes(new byte[0]); + last = System.currentTimeMillis(); + } + + + private void runReseed() { + // Have some other thread hit the penalty potentially incurred by reseeding, + // so that we can immediately return and not block the operation in progress. + new Thread() { + public void run() { + seed(); + } + }.start(); + } +} diff --git a/src/test/java/com/gitblit/utils/SecureRandomTest.java b/src/test/java/com/gitblit/utils/SecureRandomTest.java new file mode 100644 index 00000000..c4098c2f --- /dev/null +++ b/src/test/java/com/gitblit/utils/SecureRandomTest.java @@ -0,0 +1,33 @@ +package com.gitblit.utils; + +import static org.junit.Assert.*; + +import java.util.Arrays; + +import org.junit.Test; + +public class SecureRandomTest { + + @Test + public void testRandomBytes() { + SecureRandom sr = new SecureRandom(); + byte[] bytes1 = sr.randomBytes(10); + assertEquals(10, bytes1.length); + byte[] bytes2 = sr.randomBytes(10); + assertEquals(10, bytes2.length); + assertFalse(Arrays.equals(bytes1, bytes2)); + + assertEquals(0, sr.randomBytes(0).length); + assertEquals(200, sr.randomBytes(200).length); + } + + @Test + public void testNextBytes() { + SecureRandom sr = new SecureRandom(); + byte[] bytes1 = new byte[32]; + sr.nextBytes(bytes1); + byte[] bytes2 = new byte[32]; + sr.nextBytes(bytes2); + assertFalse(Arrays.equals(bytes1, bytes2)); + } +} -- cgit v1.2.3