Skip to content

MD5 passwords broken after upgrade to 1.9 #1335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
darmbrust opened this issue Apr 4, 2020 · 5 comments
Closed

MD5 passwords broken after upgrade to 1.9 #1335

darmbrust opened this issue Apr 4, 2020 · 5 comments

Comments

@darmbrust
Copy link

darmbrust commented Apr 4, 2020

The upgrade process which changes password encryption from MD5 to PBKDF2 is broken, and breaks existing passwords after the first login.

If you log in, and log out, you will never log in again.
If you log in and change your password, it works ok.

The bug happens here in the authentication manager:

    protected UserModel authenticateLocal(UserModel user, char [] password) {
		UserModel returnedUser = null;

		PasswordHash pwdHash = PasswordHash.instanceFor(user.password);
		if (pwdHash != null) {
			if (pwdHash.matches(user.password, password, user.username)) {
				returnedUser = user;
			}
		} else if (user.password.equals(new String(password))) {
			// plain-text password
			returnedUser = user;
		} 
		
		// validate user
		returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
		
		// try to upgrade the stored password hash to a stronger hash, if necessary
		upgradeStoredPassword(returnedUser, password, pwdHash);

		return returnedUser;
	}

The upgradeStoredPassword resets the password using the 'password' char array. However, the
pwdHash.matches implementation in PasswordHash blanks out the char array:

    public boolean matches(String hashedEntry, char[] password, String username) {
		if (hashedEntry == null || type != PasswordHash.getEntryType(hashedEntry)) return false;
		if (password == null) return false;

		String hashed = toHashedEntry(password, username);
		Arrays.fill(password, Character.MIN_VALUE);
		return hashed.equalsIgnoreCase(hashedEntry);
	}
@flaix
Copy link
Member

flaix commented Apr 4, 2020

🙀Well, that is embarrassing. I am so sorry.
Thanks for analyzing this!

And for reporting it. Makes me wonder why no one stumbled over this earlier. Maybe no one uses Gitblit anymore. 😟

@darmbrust
Copy link
Author

I know I had looked periodically, but with the website not being updated, I just assumed there were no updates. Not sure how I even stumbled across the git release.

I appreciate that it is still maintained :)

flaix added a commit to flaix/gitblit that referenced this issue Apr 4, 2020
The upgrade of a MD5 stored password hash to a PBKDF password hash
destroys the stored password. The has check zeroes out the password that
is tested, so that the new hash is built over the zeroed out value.

This fix prevents that an also adds a check to the test.

Fixes gitblit-org#1335
@flaix flaix added this to the 1.9.1 milestone Apr 4, 2020
@flaix
Copy link
Member

flaix commented Apr 4, 2020

I am interested to learn more about how you use Gitblit in Docker. Did you build your own image or use the provided ones? How do you run it? How did you update?

flaix added a commit to flaix/gitblit that referenced this issue Apr 5, 2020
The upgrade of a MD5 stored password hash to a PBKDF password hash
destroys the stored password. The has check zeroes out the password that
is tested, so that the new hash is built over the zeroed out value.

This fix prevents that an also adds a check to the test.

Fixes gitblit-org#1335
flaix added a commit to flaix/gitblit that referenced this issue Apr 5, 2020
The upgrade of a MD5 stored password hash to a PBKDF password hash
destroys the stored password. The has check zeroes out the password that
is tested, so that the new hash is built over the zeroed out value.

This fix prevents that an also adds a check to the test.

Fixes gitblit-org#1335
@flaix flaix closed this as completed in e47647b Apr 5, 2020
@darmbrust
Copy link
Author

I work on a terminology solution (based on solor.io) which creates changeset file that contain terminology. We utilize GIT to store and distribute the changeset files across instances. While we can use services like github, for many deployments, we prefer to have a locally controlled git server to store the change sets (which aren't always public) depending on the customer.

I also use the REST APIs to be able to programmatically create new repositories.

The usage of these git servers are typically system to system - hence the need to control the credentials centrally.

Our deployments are primarily in the cloud these days, so having each of the components packaged into docker images makes management much simpler.

For GitBlit, I utilize an image I maintain that starts with Alpine linux, adds on OpenJDK 11, and then Tomcat (these base images I reuse for other components as well). The GitBlit image is simply a deployment of the gitblit war file into the tomcat container, along with config files (users, properties) that make it come up in a known state. The gitblit_data folder is mounted as a volume (so it persists). When I upgraded from 1.8 to 1.9, I just changed my pointer for where to get the war file from, and updated my checksums that validate the downloads. The initial upgrade went so smoothly, I didn't notice the password issue until a while later, because I don't typically log into the GUI (and I don't believe the passwords are being upgraded-in-place due to GIT protocol access)

While my docker config files aren't public at the moment, they are heavily adapted from other public docker files that provide Alpine Linux + Java + Tomcat.

@flaix
Copy link
Member

flaix commented Apr 5, 2020

Thank you for your input. I recently put some work into the Gitblit Docker images, as the old ones were rather outdated and not very usable, in my opinion. Hence my question. I did not base the official Gitblit Docker images on Tomcat, though. These are rather stand-alone, but I was hoping to make updates easier.

I have released a fixed version 1.9.1. You can give that another run. Again, I'm sorry for the trouble. Thanks for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants