From 9b4afad6f4be212474809533ec2c280cce86501a Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sun, 13 Mar 2022 18:03:17 +0100 Subject: [PATCH] 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 --- .../java/com/gitblit/StoredUserConfig.java | 45 +++++- .../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")); + } + } -- 2.39.5