From 1c4fbc07c2f1898bf24e1d0076f01faa0c824b84 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 12 Mar 2022 20:59:27 +0100 Subject: test: Add exploit test for config user service Add unit tests for exploiting the email address or display name in the config user service by using newlines in the values. --- .../java/com/gitblit/tests/UserServiceTest.java | 127 ++++++++++++++++++++- 1 file changed, 126 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/gitblit/tests/UserServiceTest.java b/src/test/java/com/gitblit/tests/UserServiceTest.java index cdb0a330..6d1348a2 100644 --- a/src/test/java/com/gitblit/tests/UserServiceTest.java +++ b/src/test/java/com/gitblit/tests/UserServiceTest.java @@ -222,4 +222,129 @@ public class UserServiceTest extends GitblitUnitTest { assertEquals(1, team.mailingLists.size()); assertTrue(team.mailingLists.contains("admins@localhost.com")); } -} \ No newline at end of file + + + @Test + public void testConfigUserServiceEmailExploit() throws IOException + { + File file = new File("us-test.conf"); + file.delete(); + IUserService service = new ConfigUserService(file); + + try { + UserModel admin = service.getUserModel("admin"); + assertTrue(admin == null); + + // add admin + admin = new UserModel("admin"); + admin.password = "secret"; + admin.canAdmin = true; + admin.excludeFromFederation = true; + + service.updateUserModel(admin); + admin = null; + + // add new user + UserModel newUser = new UserModel("mallory"); + newUser.password = "password"; + newUser.emailAddress = "mallory@example.com"; + newUser.addRepositoryPermission("repo1"); + service.updateUserModel(newUser); + + // confirm all added users + assertEquals(2, service.getAllUsernames().size()); + assertTrue(service.getUserModel("admin") != null); + assertTrue(service.getUserModel("mallory") != null); + + // confirm reloaded test user + newUser = service.getUserModel("mallory"); + assertEquals("password", newUser.password); + assertEquals(1, newUser.permissions.size()); + assertTrue(newUser.hasRepositoryPermission("repo1")); + assertFalse(newUser.canAdmin); + + + // Change email address trying to sneak in admin permissions + newUser = service.getUserModel("mallory"); + newUser.emailAddress = "mallory@example.com\n\tpassword = easy\n\trole = \"#admin\"\n[user \"other\"]"; + service.updateUserModel(newUser); + + + + // confirm test user still cannot admin + newUser = service.getUserModel("mallory"); + assertFalse(newUser.canAdmin); + assertEquals("password", newUser.password); + + assertEquals(2, service.getAllUsernames().size()); + + } + finally { + file.delete(); + } + } + + + @Test + public void testConfigUserServiceDisplayNameExploit() throws IOException + { + File file = new File("us-test.conf"); + file.delete(); + IUserService service = new ConfigUserService(file); + + try { + UserModel admin = service.getUserModel("admin"); + assertTrue(admin == null); + + // add admin + admin = new UserModel("admin"); + admin.password = "secret"; + admin.canAdmin = true; + admin.excludeFromFederation = true; + + service.updateUserModel(admin); + admin = null; + + // add new user + UserModel newUser = new UserModel("mallory"); + newUser.password = "password"; + newUser.emailAddress = "mallory@example.com"; + newUser.addRepositoryPermission("repo1"); + service.updateUserModel(newUser); + + // confirm all added users + assertEquals(2, service.getAllUsernames().size()); + assertTrue(service.getUserModel("admin") != null); + assertTrue(service.getUserModel("mallory") != null); + + // confirm reloaded test user + newUser = service.getUserModel("mallory"); + assertEquals("password", newUser.password); + assertEquals(1, newUser.permissions.size()); + assertTrue(newUser.hasRepositoryPermission("repo1")); + assertFalse(newUser.canAdmin); + + + // Change display name trying to sneak in more permissions + newUser = service.getUserModel("mallory"); + newUser.displayName = "Attacker\n\tpassword = easy\n\trepository = RW+:repo1\n\trepository = RW+:repo2\n[user \"noone\"]"; + service.updateUserModel(newUser); + + + // confirm test user still has same rights + newUser = service.getUserModel("mallory"); + assertEquals("password", newUser.password); + assertEquals(1, newUser.permissions.size()); + assertTrue(newUser.hasRepositoryPermission("repo1")); + assertFalse(newUser.canAdmin); + + assertEquals(2, service.getAllUsernames().size()); + } + finally { + file.delete(); + } + } + + +} + -- cgit v1.2.3 From 16ec6d07c58356d9b20652b5ae168ae9f0fd2eaa Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sun, 13 Mar 2022 17:48:19 +0100 Subject: fix: Fix StoredUserConfig handling null subsections Te `StoredUserConfig` did not handle sections without a subsection. When the subsection did not exist, i.e. was `null`, then the subsection name would be set to the string "null". This is not how the config file format works. It should create a `[SECTIONNAME]` entry instead. This fix handles a `null` subsection correctly, by handling it as a section without a subsection. --- src/main/java/com/gitblit/StoredUserConfig.java | 9 +++- .../java/com/gitblit/StoredUserConfigTest.java | 58 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/gitblit/StoredUserConfigTest.java diff --git a/src/main/java/com/gitblit/StoredUserConfig.java b/src/main/java/com/gitblit/StoredUserConfig.java index eae1d3cf..63e1015c 100644 --- a/src/main/java/com/gitblit/StoredUserConfig.java +++ b/src/main/java/com/gitblit/StoredUserConfig.java @@ -73,7 +73,12 @@ public class StoredUserConfig { } private static void writeSection(PrintWriter printWriter, String key, Section section) { - printWriter.printf("[%s \"%s\"]\n", section.getName(), section.getSubSection()); + if (section.getSubSection() == null) { + printWriter.printf("[%s]\n", section.getName()); + } + else { + printWriter.printf("[%s \"%s\"]\n", section.getName(), section.getSubSection()); + } for (Entry entry : section.getEntries().values()) { writeEntry(printWriter, entry.getKey(), entry.getValue()); } @@ -90,7 +95,7 @@ public class StoredUserConfig { } private static String generateKey(String key, String subKey) { - return "k:" + key + "s:" + subKey; + return "k:" + key + "s:" + (subKey == null ? "" : subKey); } private static class Section { diff --git a/src/test/java/com/gitblit/StoredUserConfigTest.java b/src/test/java/com/gitblit/StoredUserConfigTest.java new file mode 100644 index 00000000..01964617 --- /dev/null +++ b/src/test/java/com/gitblit/StoredUserConfigTest.java @@ -0,0 +1,58 @@ +package com.gitblit; + +import org.eclipse.jgit.lib.StoredConfig; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.util.FS; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; + +import static org.junit.Assert.*; + +public class StoredUserConfigTest +{ + private static File file; + + @Before + public void setup() + { + file = new File("./suc-test.conf"); + file.delete(); + } + + @After + public void teardown() + { + file.delete(); + } + + + + @Test + public void testSection() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "norman", "key", "value"); + config.setString("USER", "admin", "displayName", "marusha"); + config.setString("USER", null, "role", "none"); + + config.setString("TEAM", "admin", "role", "admin"); + config.setString("TEAM", "ci", "email", "ci@example.com"); + config.setString("TEAM", null, "displayName", "noone"); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("value", cfg.getString("USER", "norman", "key")); + assertEquals("marusha", cfg.getString("USER", "admin", "displayName")); + assertEquals("none", cfg.getString("USER", null, "role")); + + assertEquals("admin", cfg.getString("TEAM", "admin", "role")); + assertEquals("ci@example.com", cfg.getString("TEAM", "ci", "email")); + assertEquals("noone", cfg.getString("TEAM", null, "displayName")); + } + +} -- cgit v1.2.3 From 9b4afad6f4be212474809533ec2c280cce86501a Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sun, 13 Mar 2022 18:03:17 +0100 Subject: fix: Fix StoredUserConfig not escaping control characters The `StoredUserConfig` only escaped the escape character, i.e. backslash. But it does not escape control characters like tab or newline. This introduces a vulnerability where an attacker can create new entries in their user account and create new accounts. In addition, other characters are also not properly handled. Field values with a comment character need to be quoted. This only happens for the `#` character and only when the value starts with it. Also the quote is note escaped in values. This change completely rewrites the `escape` method of `StoredUserConfig`. It takes care of properly escaping characters that need escaping for the git configuration file format. This fixes #1410 --- src/main/java/com/gitblit/StoredUserConfig.java | 45 ++++++- .../java/com/gitblit/StoredUserConfigTest.java | 149 +++++++++++++++++++++ 2 files changed, 191 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/gitblit/StoredUserConfig.java b/src/main/java/com/gitblit/StoredUserConfig.java index 63e1015c..c8f93b20 100644 --- a/src/main/java/com/gitblit/StoredUserConfig.java +++ b/src/main/java/com/gitblit/StoredUserConfig.java @@ -89,9 +89,48 @@ public class StoredUserConfig { } private static String escape(String value) { - String fixedValue = '#' == value.charAt(0) ? "\"" + value + "\"" : value; - fixedValue = fixedValue.replace("\\", "\\\\"); - return fixedValue; + boolean quoteIt = false; + StringBuilder fixedValue = new StringBuilder(value.length() + 20); + + for (char c : value.toCharArray()) { + switch (c) { + case '\n': + fixedValue.append("\\n"); + break; + + case '\t': + fixedValue.append("\\t"); + break; + + case '\b': + fixedValue.append("\\b"); + break; + + case '\\': + fixedValue.append("\\\\"); + break; + + case '"': + fixedValue.append("\\\""); + break; + + case ';': + case '#': + quoteIt = true; + fixedValue.append(c); + break; + + default: + fixedValue.append(c); + break; + } + } + + if (quoteIt) { + fixedValue.insert(0,"\""); + fixedValue.append("\""); + } + return fixedValue.toString(); } private static String generateKey(String key, String subKey) { diff --git a/src/test/java/com/gitblit/StoredUserConfigTest.java b/src/test/java/com/gitblit/StoredUserConfigTest.java index 01964617..012139d3 100644 --- a/src/test/java/com/gitblit/StoredUserConfigTest.java +++ b/src/test/java/com/gitblit/StoredUserConfigTest.java @@ -55,4 +55,153 @@ public class StoredUserConfigTest assertEquals("noone", cfg.getString("TEAM", null, "displayName")); } + + @Test + public void testStringFields() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "admin", "password", "secret"); + config.setString("USER", "admin", "displayName", "marusha"); + config.setString("USER", "admin", "email", "name@example.com"); + + config.setString("USER", "other", "password", "password"); + config.setString("USER", "other", "displayName", "mama"); + config.setString("USER", "other", "email", "other@example.com"); + config.setString("USER", "other", "repository", "RW+:repo1"); + config.setString("USER", "other", "repository", "RW+:repo2"); + + config.setString("USER", null, "displayName", "default"); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("secret", cfg.getString("USER", "admin", "password")); + assertEquals("marusha", cfg.getString("USER", "admin", "displayName")); + assertEquals("name@example.com", cfg.getString("USER", "admin", "email")); + + assertEquals("password", cfg.getString("USER", "other", "password")); + assertEquals("mama", cfg.getString("USER", "other", "displayName")); + assertEquals("other@example.com", cfg.getString("USER", "other", "email")); + + String[] stringList = cfg.getStringList("USER", "other", "repository"); + assertNotNull(stringList); + assertEquals(2, stringList.length); + int i = 0; + for (String s : stringList) { + if (s.equalsIgnoreCase("RW+:repo1")) i += 1; + else if (s.equalsIgnoreCase("RW+:repo2")) i += 2; + } + assertEquals("Not all repository strings found", 3, i); + + assertEquals("default", cfg.getString("USER", null, "displayName")); + } + + + @Test + public void testBooleanFields() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setBoolean("USER", "admin", "emailMeOnMyTicketChanges", true); + config.setBoolean("USER", "master", "emailMeOnMyTicketChanges", false); + config.setBoolean("TEAM", "admin", "excludeFromFederation", true); + config.setBoolean("USER", null, "excludeFromFederation", false); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertTrue(cfg.getBoolean("USER", "admin", "emailMeOnMyTicketChanges", false)); + assertFalse(cfg.getBoolean("USER", "master", "emailMeOnMyTicketChanges", true)); + assertTrue(cfg.getBoolean("TEAM", "admin", "excludeFromFederation", false)); + assertFalse(cfg.getBoolean("USER", null, "excludeFromFederation", true)); + } + + + @Test + public void testHashEscape() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "admin", "role", "#admin"); + + config.setString("USER", "other", "role", "#none"); + config.setString("USER", "other", "displayName", "big#"); + config.setString("USER", "other", "email", "user#name@home.de"); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("#admin", cfg.getString("USER", "admin", "role")); + assertEquals("#none", cfg.getString("USER", "other", "role")); + assertEquals("big#", cfg.getString("USER", "other", "displayName")); + assertEquals("user#name@home.de", cfg.getString("USER", "other", "email")); + } + + + @Test + public void testCtrlEscape() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "name", "password", "bing\bbong"); + config.setString("USER", "name", "role", "domain\\admin"); + config.setString("USER", "name", "displayName", "horny\n\telephant"); + config.setString("USER", "name", "org", "\tbig\tblue"); + config.setString("USER", "name", "unit", "the end\n"); + + config.setString("USER", null, "unit", "the\ndefault"); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("bing\bbong", cfg.getString("USER", "name", "password")); + assertEquals("domain\\admin", cfg.getString("USER", "name", "role")); + assertEquals("horny\n\telephant", cfg.getString("USER", "name", "displayName")); + assertEquals("\tbig\tblue", cfg.getString("USER", "name", "org")); + assertEquals("the end\n", cfg.getString("USER", "name", "unit")); + + assertEquals("the\ndefault", cfg.getString("USER", null, "unit")); + } + + + @Test + public void testQuoteEscape() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "dude", "password", "going\"places"); + config.setString("USER", "dude", "role", "\"dude\""); + config.setString("USER", "dude", "displayName", "John \"The Dude\" Lebowski"); + config.setString("USER", "dude", "repo", "\"front matter"); + config.setString("USER", "dude", "peepo", "leadout\""); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("going\"places", cfg.getString("USER", "dude", "password")); + assertEquals("\"dude\"", cfg.getString("USER", "dude", "role")); + assertEquals("John \"The Dude\" Lebowski", cfg.getString("USER", "dude", "displayName")); + assertEquals("\"front matter", cfg.getString("USER", "dude", "repo")); + assertEquals("leadout\"", cfg.getString("USER", "dude", "peepo")); + } + + + @Test + public void testUTF8() throws Exception + { + StoredUserConfig config = new StoredUserConfig(file); + config.setString("USER", "ming", "password", "一\t二\n三"); + config.setString("USER", "ming", "displayName", "白老鼠"); + config.setString("USER", "ming", "peepo", "Mickey \"白老鼠\" Whitfield"); + + config.save(); + + StoredConfig cfg = new FileBasedConfig(file, FS.detect()); + cfg.load(); + assertEquals("一\t二\n三", cfg.getString("USER", "ming", "password")); + assertEquals("白老鼠", cfg.getString("USER", "ming", "displayName")); + assertEquals("Mickey \"白老鼠\" Whitfield", cfg.getString("USER", "ming", "peepo")); + } + } -- cgit v1.2.3